[PATCH net-next v13 0/7] bonding: Extend arp_ip_target format to allow for a list of vlan tags.

[PATCH net-next v13 0/7] bonding: Extend arp_ip_target format to allow for a list of vlan tags.

[David Wilder]

The current implementation of the arp monitor builds a list of vlan-tags by
following the chain of net_devices above the bond. See bond_verify_device_path().
Unfortunately, with some configurations, this is not possible. One example is
when an ovs switch is configured above the bond.

This change extends the "arp_ip_target" parameter format to allow for a list of
vlan tags to be included for each arp target. This new list of tags is optional
and may be omitted to preserve the current format and process of discovering
vlans.

The new format for arp_ip_target is:
arp_ip_target ipv4-address[vlan-tag\...],...

For example:
arp_ip_target 10.0.0.1[10/20]
arp_ip_target 10.0.0.1[] (used to disable vlan discovery)

Changes since V12
Fixed uninitialized variable in bond_option_arp_ip_targets_set() (patch 4)
causing a CI failure.

Changes since V11
No Change.

Changes since V10
Thanks Paolo:
- 1/7 Changed the layout of struct bond_arp_target to reduce size of the struct.
- 3/7 Fixed format 'size-num' -> 'size - num'
- 7/7 Updated selftest (bond-arp-ip-target.sh). Removed sleep 10 in check_failure_count().
      Added call to tc to verify arp probes are reaching the target interface. Then I verify that
      the Link Failure counts are not increasing over "time".  Arp probes are sent every 100ms,
      two missed probes will trigger a Link failure. A one second wait between checking counts
      should be be more than sufficient.  This speeds up the execution of the test.

Thanks Nikolay:
- 4/7 In bond_option_arp_ip_targets_clear() I changed the definition of empty_target to empty_target = {}.
-     bond_validate_tags() now verifies input is a multiple of sizeof(struct bond_vlan_tag).
      Updated VID validity check to use: !tags->vlan_id || tags->vlan_id >= VLAN_VID_MASK) as suggested.
-     In bond_option_arp_ip_targets_set() removed the redundant length check of target.target_ip.
-     Added kfree(target.tags) when bond_option_arp_ip_target_add() results in an error.
-     Removed the caching of struct bond_vlan_tag returned by bond_verify_device_path(), Nikolay
      pointed out that caching tags prevented the detection of VLAN configuration changes. 
      Added a kfree(tags) for tags allocated in bond_verify_device_path().

Jay, Nikolay and I had a discussion regarding locking when adding, deleting or changing vlan tags.
Jay pointed out that user supplied tags that are stashed in the bond configuration and can only be
changed via user space this can be done safely in an RCU manner as netlink always operates with RTNL
held. If user space provided tags and then replumbs things, it'll be on user space to update the tags
in a safe manor.  

I was concerned about changing options on a configured bond,  I found that attempting to change
a bonds configuration (using "ip set") will abort the attempt to make a change if the bond's state is
"UP" or has slaves configured. Therefor the configuration and operational side of a bond is separated.
I agree with Jay that the existing locking scheme is sufficient.

Change since V9
Fix kdoc build error.

Changes since V8:
Moved the #define BOND_MAX_VLAN_TAGS from patch 6 to patch 3.
Thanks Simon for catching the bisection break.

Changes since V7:
These changes should eliminate the CI failures I have been seeing.
1) patch 2, changed type of bond_opt_value.extra_len to size_t.
2) Patch 4, added bond_validate_tags() to validate the array of bond_vlan_tag provided by
 the user.

Changes since V6:
1) I made a number of changes to fix the failure seen in the
kernel CI.  I am still unable to reproduce the this failure, hopefully I
have fixed it.  These change are in patch #4 to functions:
bond_option_arp_ip_targets_clear() and
bond_option_arp_ip_targets_set()

Changes since V5: Only the last 2 patches have changed since V5.
1) Fixed sparse warning in bond_fill_info().
2) Also in bond_fill_info() I resolved data.addr uninitialized when if condition is not met.
Thank you Simon for catching this. Note: The change is different that what I shared earlier.
3) Fixed shellcheck warnings in test script: Blocked source warning, Ignored specific unassigned
references and exported ALL_TESTS to resolve a reference warning.

Changes since V4:
1)Dropped changes to proc and sysfs APIs to bonding.  These APIs 
do not need to be updated to support new functionality.  Netlink
and iproute2 have been updated to do the right thing, but the
other APIs are more or less frozen in the past.

2)Jakub reported a warning triggered in bond_info_seq_show() during
testing.  I was unable to reproduce this warning or identify
it with code inspection.  However, all my changes to bond_info_seq_show()
have been dropped as unnecessary (see above).
Hopefully this will resolve the issue. 

3)Selftest script has been updated based on the results of shellcheck.
Two unresolved references that are not possible to resolve are all
that remain.

4)A patch was added updating bond_info_fill()
to support "ip -d show <bond-device>" command.

The inclusion of a list of vlan tags is optional. The new logic
preserves both forward and backward compatibility with the kernel
and iproute2 versions.

Changes since V3:
1) Moved the parsing of the extended arp_ip_target out of the kernel and into
   userspace (ip command). A separate patch to iproute2 to follow shortly.
2) Split up the patch set to make review easier.

Please see iproute changes in a separate posting.

Thank you for your time and reviews.

Signed-off-by: David Wilder <wilder@us.ibm.com>
David Wilder (7):
  bonding: Adding struct bond_arp_target
  bonding: Adding extra_len field to struct bond_opt_value.
  bonding: arp_ip_target helpers.
  bonding: Processing extended arp_ip_target from user space.
  bonding: Update to bond_arp_send_all() to use supplied vlan tags
  bonding: Update for extended arp_ip_target format.
  bonding: Selftest and documentation for the arp_ip_target parameter.

 Documentation/networking/bonding.rst          |  11 +
 drivers/net/bonding/bond_main.c               |  48 ++--
 drivers/net/bonding/bond_netlink.c            |  35 ++-
 drivers/net/bonding/bond_options.c            | 140 +++++++++---
 drivers/net/bonding/bond_procfs.c             |   4 +-
 drivers/net/bonding/bond_sysfs.c              |   4 +-
 include/net/bond_options.h                    |  29 ++-
 include/net/bonding.h                         |  65 +++++-
 .../selftests/drivers/net/bonding/Makefile    |   1 +
 .../drivers/net/bonding/bond-arp-ip-target.sh | 205 ++++++++++++++++++
 10 files changed, 467 insertions(+), 75 deletions(-)
 create mode 100755 tools/testing/selftests/drivers/net/bonding/bond-arp-ip-target.sh

-- 
2.50.1

[PATCH net-next v13 1/7] bonding: Adding struct bond_arp_target

[David Wilder]

Replacing the definition of bond_params.arp_targets (__be32 arp_targets[])
with:

struct bond_arp_target {
	__be32			target_ip;
	u32                     flags;
	struct bond_vlan_tag	*tags;
};

To provide storage for a list of vlan tags for each target.

All references to arp_target are change to use the new structure.

Signed-off-by: David Wilder <wilder@us.ibm.com>
---
 drivers/net/bonding/bond_main.c    | 29 ++++++++++++++++-------------
 drivers/net/bonding/bond_netlink.c |  4 ++--
 drivers/net/bonding/bond_options.c | 18 +++++++++---------
 drivers/net/bonding/bond_procfs.c  |  4 ++--
 drivers/net/bonding/bond_sysfs.c   |  4 ++--
 include/net/bond_options.h         | 20 ++++++++++++++++++++
 include/net/bonding.h              | 15 +++++----------
 7 files changed, 56 insertions(+), 38 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 4da619210c1f..3f35303b4920 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3066,26 +3066,29 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 {
 	struct rtable *rt;
 	struct bond_vlan_tag *tags;
-	__be32 *targets = bond->params.arp_targets, addr;
+	struct bond_arp_target *targets = bond->params.arp_targets;
+	__be32 target_ip, addr;
 	int i;
 
-	for (i = 0; i < BOND_MAX_ARP_TARGETS && targets[i]; i++) {
+	for (i = 0; i < BOND_MAX_ARP_TARGETS && targets[i].target_ip; i++) {
+		target_ip = targets[i].target_ip;
+		tags = targets[i].tags;
+
 		slave_dbg(bond->dev, slave->dev, "%s: target %pI4\n",
-			  __func__, &targets[i]);
-		tags = NULL;
+			  __func__, &target_ip);
 
 		/* Find out through which dev should the packet go */
-		rt = ip_route_output(dev_net(bond->dev), targets[i], 0, 0, 0,
+		rt = ip_route_output(dev_net(bond->dev), target_ip, 0, 0, 0,
 				     RT_SCOPE_LINK);
 		if (IS_ERR(rt)) {
-			/* there's no route to target - try to send arp
+			/* there's no route to target_ip - try to send arp
 			 * probe to generate any traffic (arp_validate=0)
 			 */
 			if (bond->params.arp_validate)
 				pr_warn_once("%s: no route to arp_ip_target %pI4 and arp_validate is set\n",
 					     bond->dev->name,
-					     &targets[i]);
-			bond_arp_send(slave, ARPOP_REQUEST, targets[i],
+					     &target_ip);
+			bond_arp_send(slave, ARPOP_REQUEST, target_ip,
 				      0, tags);
 			continue;
 		}
@@ -3103,15 +3106,15 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 
 		/* Not our device - skip */
 		slave_dbg(bond->dev, slave->dev, "no path to arp_ip_target %pI4 via rt.dev %s\n",
-			   &targets[i], rt->dst.dev ? rt->dst.dev->name : "NULL");
+			   &target_ip, rt->dst.dev ? rt->dst.dev->name : "NULL");
 
 		ip_rt_put(rt);
 		continue;
 
 found:
-		addr = bond_confirm_addr(rt->dst.dev, targets[i], 0);
+		addr = bond_confirm_addr(rt->dst.dev, target_ip, 0);
 		ip_rt_put(rt);
-		bond_arp_send(slave, ARPOP_REQUEST, targets[i], addr, tags);
+		bond_arp_send(slave, ARPOP_REQUEST, target_ip, addr, tags);
 		kfree(tags);
 	}
 }
@@ -6066,7 +6069,7 @@ static int __init bond_check_params(struct bond_params *params)
 	int arp_all_targets_value = 0;
 	u16 ad_actor_sys_prio = 0;
 	u16 ad_user_port_key = 0;
-	__be32 arp_target[BOND_MAX_ARP_TARGETS] = { 0 };
+	struct bond_arp_target arp_target[BOND_MAX_ARP_TARGETS] = { 0 };
 	int arp_ip_count;
 	int bond_mode	= BOND_MODE_ROUNDROBIN;
 	int xmit_hashtype = BOND_XMIT_POLICY_LAYER2;
@@ -6260,7 +6263,7 @@ static int __init bond_check_params(struct bond_params *params)
 			arp_interval = 0;
 		} else {
 			if (bond_get_targets_ip(arp_target, ip) == -1)
-				arp_target[arp_ip_count++] = ip;
+				arp_target[arp_ip_count++].target_ip = ip;
 			else
 				pr_warn("Warning: duplicate address %pI4 in arp_ip_target, skipping\n",
 					&ip);
diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
index 286f11c517f7..566a11bb7f1e 100644
--- a/drivers/net/bonding/bond_netlink.c
+++ b/drivers/net/bonding/bond_netlink.c
@@ -716,8 +716,8 @@ static int bond_fill_info(struct sk_buff *skb,
 
 	targets_added = 0;
 	for (i = 0; i < BOND_MAX_ARP_TARGETS; i++) {
-		if (bond->params.arp_targets[i]) {
-			if (nla_put_be32(skb, i, bond->params.arp_targets[i]))
+		if (bond->params.arp_targets[i].target_ip) {
+			if (nla_put_be32(skb, i, bond->params.arp_targets[i].target_ip))
 				goto nla_put_failure;
 			targets_added = 1;
 		}
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 495a87f2ea7c..91d57ba968d6 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -1125,7 +1125,7 @@ static int bond_option_arp_interval_set(struct bonding *bond,
 			netdev_dbg(bond->dev, "ARP monitoring cannot be used with MII monitoring. Disabling MII monitoring\n");
 			bond->params.miimon = 0;
 		}
-		if (!bond->params.arp_targets[0])
+		if (!bond->params.arp_targets[0].target_ip)
 			netdev_dbg(bond->dev, "ARP monitoring has been set up, but no ARP targets have been specified\n");
 	}
 	if (bond->dev->flags & IFF_UP) {
@@ -1153,20 +1153,20 @@ static void _bond_options_arp_ip_target_set(struct bonding *bond, int slot,
 					    __be32 target,
 					    unsigned long last_rx)
 {
-	__be32 *targets = bond->params.arp_targets;
+	struct bond_arp_target *targets = bond->params.arp_targets;
 	struct list_head *iter;
 	struct slave *slave;
 
 	if (slot >= 0 && slot < BOND_MAX_ARP_TARGETS) {
 		bond_for_each_slave(bond, slave, iter)
 			slave->target_last_arp_rx[slot] = last_rx;
-		targets[slot] = target;
+		targets[slot].target_ip = target;
 	}
 }
 
 static int _bond_option_arp_ip_target_add(struct bonding *bond, __be32 target)
 {
-	__be32 *targets = bond->params.arp_targets;
+	struct bond_arp_target *targets = bond->params.arp_targets;
 	int ind;
 
 	if (!bond_is_ip_target_ok(target)) {
@@ -1201,7 +1201,7 @@ static int bond_option_arp_ip_target_add(struct bonding *bond, __be32 target)
 
 static int bond_option_arp_ip_target_rem(struct bonding *bond, __be32 target)
 {
-	__be32 *targets = bond->params.arp_targets;
+	struct bond_arp_target *targets = bond->params.arp_targets;
 	struct list_head *iter;
 	struct slave *slave;
 	unsigned long *targets_rx;
@@ -1220,20 +1220,20 @@ static int bond_option_arp_ip_target_rem(struct bonding *bond, __be32 target)
 		return -EINVAL;
 	}
 
-	if (ind == 0 && !targets[1] && bond->params.arp_interval)
+	if (ind == 0 && !targets[1].target_ip && bond->params.arp_interval)
 		netdev_warn(bond->dev, "Removing last arp target with arp_interval on\n");
 
 	netdev_dbg(bond->dev, "Removing ARP target %pI4\n", &target);
 
 	bond_for_each_slave(bond, slave, iter) {
 		targets_rx = slave->target_last_arp_rx;
-		for (i = ind; (i < BOND_MAX_ARP_TARGETS-1) && targets[i+1]; i++)
+		for (i = ind; (i < BOND_MAX_ARP_TARGETS - 1) && targets[i + 1].target_ip; i++)
 			targets_rx[i] = targets_rx[i+1];
 		targets_rx[i] = 0;
 	}
-	for (i = ind; (i < BOND_MAX_ARP_TARGETS-1) && targets[i+1]; i++)
+	for (i = ind; (i < BOND_MAX_ARP_TARGETS - 1) && targets[i + 1].target_ip; i++)
 		targets[i] = targets[i+1];
-	targets[i] = 0;
+	targets[i].target_ip = 0;
 
 	return 0;
 }
diff --git a/drivers/net/bonding/bond_procfs.c b/drivers/net/bonding/bond_procfs.c
index 7edf72ec816a..94e6fd7041ee 100644
--- a/drivers/net/bonding/bond_procfs.c
+++ b/drivers/net/bonding/bond_procfs.c
@@ -121,11 +121,11 @@ static void bond_info_show_master(struct seq_file *seq)
 		seq_printf(seq, "ARP IP target/s (n.n.n.n form):");
 
 		for (i = 0; (i < BOND_MAX_ARP_TARGETS); i++) {
-			if (!bond->params.arp_targets[i])
+			if (!bond->params.arp_targets[i].target_ip)
 				break;
 			if (printed)
 				seq_printf(seq, ",");
-			seq_printf(seq, " %pI4", &bond->params.arp_targets[i]);
+			seq_printf(seq, " %pI4", &bond->params.arp_targets[i].target_ip);
 			printed = 1;
 		}
 		seq_printf(seq, "\n");
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 9a75ad3181ab..7114bd4d7735 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -290,9 +290,9 @@ static ssize_t bonding_show_arp_targets(struct device *d,
 	int i, res = 0;
 
 	for (i = 0; i < BOND_MAX_ARP_TARGETS; i++) {
-		if (bond->params.arp_targets[i])
+		if (bond->params.arp_targets[i].target_ip)
 			res += sysfs_emit_at(buf, res, "%pI4 ",
-					     &bond->params.arp_targets[i]);
+					     &bond->params.arp_targets[i].target_ip);
 	}
 	if (res)
 		buf[res-1] = '\n'; /* eat the leftover space */
diff --git a/include/net/bond_options.h b/include/net/bond_options.h
index e6eedf23aea1..dea58a07e4cc 100644
--- a/include/net/bond_options.h
+++ b/include/net/bond_options.h
@@ -121,6 +121,26 @@ struct bond_option {
 	int (*set)(struct bonding *bond, const struct bond_opt_value *val);
 };
 
+struct bond_vlan_tag {
+	__be16		vlan_proto;
+	unsigned short	vlan_id;
+};
+
+/* Value type flags:
+ *  BOND_TARGET_DONTFREE - never free the tags
+ *  BOND_TARGET_USERTAGS - tags have been supplied by the user
+ */
+enum {
+	BOND_TARGET_DONTFREE = BIT(0),
+	BOND_TARGET_USERTAGS = BIT(1),
+};
+
+struct bond_arp_target {
+	__be32			target_ip;
+	u32                     flags;
+	struct bond_vlan_tag	*tags;
+};
+
 int __bond_opt_set(struct bonding *bond, unsigned int option,
 		   struct bond_opt_value *val,
 		   struct nlattr *bad_attr, struct netlink_ext_ack *extack);
diff --git a/include/net/bonding.h b/include/net/bonding.h
index 49edc7da0586..3497e5061f90 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -136,7 +136,7 @@ struct bond_params {
 	int ad_select;
 	char primary[IFNAMSIZ];
 	int primary_reselect;
-	__be32 arp_targets[BOND_MAX_ARP_TARGETS];
+	struct bond_arp_target arp_targets[BOND_MAX_ARP_TARGETS];
 	int tx_queues;
 	int all_slaves_active;
 	int resend_igmp;
@@ -276,11 +276,6 @@ struct bonding {
 void bond_queue_slave_event(struct slave *slave);
 void bond_lower_state_changed(struct slave *slave);
 
-struct bond_vlan_tag {
-	__be16		vlan_proto;
-	unsigned short	vlan_id;
-};
-
 /*
  * Returns NULL if the net_device does not belong to any of the bond's slaves
  *
@@ -524,7 +519,7 @@ static inline unsigned long slave_oldest_target_arp_rx(struct bonding *bond,
 	int i = 1;
 	unsigned long ret = slave->target_last_arp_rx[0];
 
-	for (; (i < BOND_MAX_ARP_TARGETS) && bond->params.arp_targets[i]; i++)
+	for (; (i < BOND_MAX_ARP_TARGETS) && bond->params.arp_targets[i].target_ip; i++)
 		if (time_before(slave->target_last_arp_rx[i], ret))
 			ret = slave->target_last_arp_rx[i];
 
@@ -763,14 +758,14 @@ static inline bool bond_slave_has_mac_rcu(struct bonding *bond, const u8 *mac)
 /* Check if the ip is present in arp ip list, or first free slot if ip == 0
  * Returns -1 if not found, index if found
  */
-static inline int bond_get_targets_ip(__be32 *targets, __be32 ip)
+static inline int bond_get_targets_ip(struct bond_arp_target *targets, __be32 ip)
 {
 	int i;
 
 	for (i = 0; i < BOND_MAX_ARP_TARGETS; i++)
-		if (targets[i] == ip)
+		if (targets[i].target_ip == ip)
 			return i;
-		else if (targets[i] == 0)
+		else if (targets[i].target_ip == 0)
 			break;
 
 	return -1;
-- 
2.50.1

[PATCH net-next v13 2/7] bonding: Adding extra_len field to struct bond_opt_value.

[David Wilder]

Used to record the size of the extra array.

__bond_opt_init() is updated to set extra_len.
BOND_OPT_EXTRA_MAXLEN is increased from 16 to 64.

This is needed for the extended  arp_ip_target option.
The ip command will now pass a variable length value when
setting arp_ip_target.

Signed-off-by: David Wilder <wilder@us.ibm.com>
---
 include/net/bond_options.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/net/bond_options.h b/include/net/bond_options.h
index dea58a07e4cc..e3eb5fe6d1e8 100644
--- a/include/net/bond_options.h
+++ b/include/net/bond_options.h
@@ -87,14 +87,15 @@ enum {
  * - if value != ULLONG_MAX -> parse value
  * - if string != NULL -> parse string
  * - if the opt is RAW data and length less than maxlen,
- *   copy the data to extra storage
+ *   copy the data to extra storage, extra_len is set to the size of data copied.
  */
 
-#define BOND_OPT_EXTRA_MAXLEN 16
+#define BOND_OPT_EXTRA_MAXLEN 64
 struct bond_opt_value {
 	char *string;
 	u64 value;
 	u32 flags;
+	size_t extra_len;
 	union {
 		char extra[BOND_OPT_EXTRA_MAXLEN];
 		struct net_device *slave_dev;
@@ -169,8 +170,10 @@ static inline void __bond_opt_init(struct bond_opt_value *optval,
 	else if (string)
 		optval->string = string;
 
-	if (extra && extra_len <= BOND_OPT_EXTRA_MAXLEN)
+	if (extra && extra_len <= BOND_OPT_EXTRA_MAXLEN) {
 		memcpy(optval->extra, extra, extra_len);
+		optval->extra_len = extra_len;
+	}
 }
 #define bond_opt_initval(optval, value) __bond_opt_init(optval, NULL, value, NULL, 0)
 #define bond_opt_initstr(optval, str) __bond_opt_init(optval, str, ULLONG_MAX, NULL, 0)
-- 
2.50.1

[PATCH net-next v13 3/7] bonding: arp_ip_target helpers.

[David Wilder]

Adding helpers and defines needed for extending the
arp_ip_target parameters.

Signed-off-by: David Wilder <wilder@us.ibm.com>
---
 include/net/bonding.h | 50 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/include/net/bonding.h b/include/net/bonding.h
index 3497e5061f90..62359b34b69c 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -809,4 +809,54 @@ static inline netdev_tx_t bond_tx_drop(struct net_device *dev, struct sk_buff *s
 	return NET_XMIT_DROP;
 }
 
+/* Helpers for handling arp_ip_target */
+#define BOND_OPTION_STRING_MAX_SIZE 64
+#define BOND_MAX_VLAN_TAGS 5
+#define BOND_VLAN_PROTO_NONE cpu_to_be16(0xffff)
+
+static inline char *bond_arp_target_to_string(struct bond_arp_target *target,
+					      char *buf, int size)
+{
+	struct bond_vlan_tag *tags = target->tags;
+	int i, num = 0;
+
+	if (!(target->flags & BOND_TARGET_USERTAGS)) {
+		num = snprintf(&buf[0], size, "%pI4", &target->target_ip);
+		return buf;
+	}
+
+	num = snprintf(&buf[0], size, "%pI4[", &target->target_ip);
+	if (tags) {
+		for (i = 0; (tags[i].vlan_proto != BOND_VLAN_PROTO_NONE); i++) {
+			if (!tags[i].vlan_id)
+				continue;
+			if (i != 0)
+				num = num + snprintf(&buf[num], size - num, "/");
+			num = num + snprintf(&buf[num], size - num, "%u",
+					     tags[i].vlan_id);
+		}
+	}
+	snprintf(&buf[num], size - num, "]");
+	return buf;
+}
+
+static inline void bond_free_vlan_tag(struct bond_arp_target *target)
+{
+	struct bond_vlan_tag *tags = target->tags;
+
+	target->tags = NULL;
+	target->flags = 0;
+	kfree(tags);
+}
+
+static inline void __bond_free_vlan_tags(struct bond_arp_target *targets)
+{
+	int i;
+
+	for (i = 0; i < BOND_MAX_ARP_TARGETS && targets[i].tags; i++)
+		bond_free_vlan_tag(&targets[i]);
+}
+
+#define bond_free_vlan_tags(targets)  __bond_free_vlan_tags(targets)
+
 #endif /* _NET_BONDING_H */
-- 
2.50.1

[PATCH net-next v13 4/7] bonding: Processing extended arp_ip_target from user space.

[David Wilder]

Changes to bond_netlink and bond_options to process extended
format arp_ip_target option sent from user space via the ip
command.

The extended format adds a list of vlan tags to the ip target address.

Signed-off-by: David Wilder <wilder@us.ibm.com>
---
 drivers/net/bonding/bond_netlink.c |   5 +-
 drivers/net/bonding/bond_options.c | 126 +++++++++++++++++++++++------
 2 files changed, 104 insertions(+), 27 deletions(-)

diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
index 566a11bb7f1e..15782745fa4d 100644
--- a/drivers/net/bonding/bond_netlink.c
+++ b/drivers/net/bonding/bond_netlink.c
@@ -307,9 +307,10 @@ static int bond_changelink(struct net_device *bond_dev, struct nlattr *tb[],
 			if (nla_len(attr) < sizeof(target))
 				return -EINVAL;
 
-			target = nla_get_be32(attr);
+			bond_opt_initextra(&newval,
+					   (__force void *)nla_data(attr),
+					   nla_len(attr));
 
-			bond_opt_initval(&newval, (__force u64)target);
 			err = __bond_opt_set(bond, BOND_OPT_ARP_TARGETS,
 					     &newval,
 					     data[IFLA_BOND_ARP_IP_TARGET],
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 91d57ba968d6..fb31d0b800b0 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -13,6 +13,7 @@
 #include <linux/ctype.h>
 #include <linux/inet.h>
 #include <linux/sched/signal.h>
+#include <linux/if_vlan.h>
 
 #include <net/bonding.h>
 #include <net/ndisc.h>
@@ -31,8 +32,10 @@ static int bond_option_use_carrier_set(struct bonding *bond,
 				       const struct bond_opt_value *newval);
 static int bond_option_arp_interval_set(struct bonding *bond,
 					const struct bond_opt_value *newval);
-static int bond_option_arp_ip_target_add(struct bonding *bond, __be32 target);
-static int bond_option_arp_ip_target_rem(struct bonding *bond, __be32 target);
+static int bond_option_arp_ip_target_add(struct bonding *bond,
+					 struct bond_arp_target target);
+static int bond_option_arp_ip_target_rem(struct bonding *bond,
+					 struct bond_arp_target target);
 static int bond_option_arp_ip_targets_set(struct bonding *bond,
 					  const struct bond_opt_value *newval);
 static int bond_option_ns_ip6_targets_set(struct bonding *bond,
@@ -1150,7 +1153,7 @@ static int bond_option_arp_interval_set(struct bonding *bond,
 }
 
 static void _bond_options_arp_ip_target_set(struct bonding *bond, int slot,
-					    __be32 target,
+					    struct bond_arp_target target,
 					    unsigned long last_rx)
 {
 	struct bond_arp_target *targets = bond->params.arp_targets;
@@ -1160,24 +1163,26 @@ static void _bond_options_arp_ip_target_set(struct bonding *bond, int slot,
 	if (slot >= 0 && slot < BOND_MAX_ARP_TARGETS) {
 		bond_for_each_slave(bond, slave, iter)
 			slave->target_last_arp_rx[slot] = last_rx;
-		targets[slot].target_ip = target;
+		memcpy(&targets[slot], &target, sizeof(target));
 	}
 }
 
-static int _bond_option_arp_ip_target_add(struct bonding *bond, __be32 target)
+static int _bond_option_arp_ip_target_add(struct bonding *bond,
+					  struct bond_arp_target target)
 {
 	struct bond_arp_target *targets = bond->params.arp_targets;
+	char pbuf[BOND_OPTION_STRING_MAX_SIZE];
 	int ind;
 
-	if (!bond_is_ip_target_ok(target)) {
+	if (!bond_is_ip_target_ok(target.target_ip)) {
 		netdev_err(bond->dev, "invalid ARP target %pI4 specified for addition\n",
-			   &target);
+			   &target.target_ip);
 		return -EINVAL;
 	}
 
-	if (bond_get_targets_ip(targets, target) != -1) { /* dup */
+	if (bond_get_targets_ip(targets, target.target_ip) != -1) { /* dup */
 		netdev_err(bond->dev, "ARP target %pI4 is already present\n",
-			   &target);
+			   &target.target_ip);
 		return -EINVAL;
 	}
 
@@ -1187,43 +1192,46 @@ static int _bond_option_arp_ip_target_add(struct bonding *bond, __be32 target)
 		return -EINVAL;
 	}
 
-	netdev_dbg(bond->dev, "Adding ARP target %pI4\n", &target);
+	netdev_dbg(bond->dev, "Adding ARP target %s\n",
+		   bond_arp_target_to_string(&target, pbuf, sizeof(pbuf)));
 
 	_bond_options_arp_ip_target_set(bond, ind, target, jiffies);
 
 	return 0;
 }
 
-static int bond_option_arp_ip_target_add(struct bonding *bond, __be32 target)
+static int bond_option_arp_ip_target_add(struct bonding *bond,
+					 struct bond_arp_target target)
 {
 	return _bond_option_arp_ip_target_add(bond, target);
 }
 
-static int bond_option_arp_ip_target_rem(struct bonding *bond, __be32 target)
+static int bond_option_arp_ip_target_rem(struct bonding *bond,
+					 struct bond_arp_target target)
 {
 	struct bond_arp_target *targets = bond->params.arp_targets;
+	unsigned long *targets_rx;
 	struct list_head *iter;
 	struct slave *slave;
-	unsigned long *targets_rx;
 	int ind, i;
 
-	if (!bond_is_ip_target_ok(target)) {
+	if (!bond_is_ip_target_ok(target.target_ip)) {
 		netdev_err(bond->dev, "invalid ARP target %pI4 specified for removal\n",
-			   &target);
+			   &target.target_ip);
 		return -EINVAL;
 	}
 
-	ind = bond_get_targets_ip(targets, target);
+	ind = bond_get_targets_ip(targets, target.target_ip);
 	if (ind == -1) {
 		netdev_err(bond->dev, "unable to remove nonexistent ARP target %pI4\n",
-			   &target);
+			   &target.target_ip);
 		return -EINVAL;
 	}
 
 	if (ind == 0 && !targets[1].target_ip && bond->params.arp_interval)
 		netdev_warn(bond->dev, "Removing last arp target with arp_interval on\n");
 
-	netdev_dbg(bond->dev, "Removing ARP target %pI4\n", &target);
+	netdev_dbg(bond->dev, "Removing ARP target %pI4\n", &target.target_ip);
 
 	bond_for_each_slave(bond, slave, iter) {
 		targets_rx = slave->target_last_arp_rx;
@@ -1231,30 +1239,77 @@ static int bond_option_arp_ip_target_rem(struct bonding *bond, __be32 target)
 			targets_rx[i] = targets_rx[i+1];
 		targets_rx[i] = 0;
 	}
-	for (i = ind; (i < BOND_MAX_ARP_TARGETS - 1) && targets[i + 1].target_ip; i++)
-		targets[i] = targets[i+1];
+
+	bond_free_vlan_tag(&targets[ind]);
+
+	for (i = ind; (i < BOND_MAX_ARP_TARGETS - 1) && targets[i + 1].target_ip; i++) {
+		targets[i].target_ip = targets[i + 1].target_ip;
+		targets[i].tags = targets[i + 1].tags;
+		targets[i].flags = targets[i + 1].flags;
+	}
 	targets[i].target_ip = 0;
+	targets[i].flags = 0;
+	targets[i].tags = NULL;
 
 	return 0;
 }
 
 void bond_option_arp_ip_targets_clear(struct bonding *bond)
 {
+	struct bond_arp_target empty_target = {};
 	int i;
 
 	for (i = 0; i < BOND_MAX_ARP_TARGETS; i++)
-		_bond_options_arp_ip_target_set(bond, i, 0, 0);
+		_bond_options_arp_ip_target_set(bond, i, empty_target, 0);
+}
+
+/**
+ * bond_validate_tags - validate an array of bond_vlan_tag.
+ * @tags: the array to validate
+ * @len: the length in bytes of @tags
+ *
+ * Validate that @tags points to a valid array of struct bond_vlan_tag.
+ * Returns: the length of the validated bytes in the array or -1 if no
+ * valid list is found.
+ */
+static int bond_validate_tags(struct bond_vlan_tag *tags, size_t len)
+{
+	size_t i, ntags = 0;
+
+	if (len == 0 || !tags)
+		return 0;
+
+	if (len % sizeof(struct bond_vlan_tag) != 0)
+		return -1;
+
+	for (i = 0; i <= len; i = i + sizeof(struct bond_vlan_tag)) {
+		if (ntags > BOND_MAX_VLAN_TAGS)
+			break;
+
+		if (tags->vlan_proto == BOND_VLAN_PROTO_NONE)
+			return i + sizeof(struct bond_vlan_tag);
+
+		if (!tags->vlan_id || tags->vlan_id >= VLAN_VID_MASK)
+			break;
+		tags++;
+		ntags++;
+	}
+	return -1;
 }
 
 static int bond_option_arp_ip_targets_set(struct bonding *bond,
 					  const struct bond_opt_value *newval)
 {
-	int ret = -EPERM;
-	__be32 target;
+	size_t len = (size_t)newval->extra_len;
+	char *extra = (char *)newval->extra;
+	struct bond_arp_target target = {};
+	int size, ret = -EPERM;
 
 	if (newval->string) {
+		/* Adding or removing arp_ip_target from sysfs */
 		if (strlen(newval->string) < 1 ||
-		    !in4_pton(newval->string + 1, -1, (u8 *)&target, -1, NULL)) {
+		    !in4_pton(newval->string + 1, -1, (u8 *)&target.target_ip,
+			      -1, NULL)) {
 			netdev_err(bond->dev, "invalid ARP target specified\n");
 			return ret;
 		}
@@ -1265,8 +1320,29 @@ static int bond_option_arp_ip_targets_set(struct bonding *bond,
 		else
 			netdev_err(bond->dev, "no command found in arp_ip_targets file - use +<addr> or -<addr>\n");
 	} else {
-		target = newval->value;
+		/* Adding arp_ip_target from netlink. aka: ip command */
+		memcpy(&target.target_ip, newval->extra, sizeof(__be32));
+		len = len - sizeof(target.target_ip);
+		extra = extra + sizeof(target.target_ip);
+
+		size = bond_validate_tags((struct bond_vlan_tag *)extra, len);
+
+		if (size > 0) {
+			target.tags = kmalloc((size_t)size, GFP_ATOMIC);
+			if (!target.tags)
+				return -ENOMEM;
+			memcpy(target.tags, extra, size);
+			target.flags |= BOND_TARGET_USERTAGS;
+		} else if (size == -1) {
+			netdev_err(bond->dev, "Invalid list of vlans provided with %pI4\n",
+				   &target.target_ip);
+			return -EINVAL;
+		}
+
 		ret = bond_option_arp_ip_target_add(bond, target);
+
+		if (ret)
+			kfree(target.tags);
 	}
 
 	return ret;
-- 
2.50.1

[PATCH net-next v13 5/7] bonding: Update to bond_arp_send_all() to use supplied vlan tags

[David Wilder]

bond_arp_send_all() will pass the vlan tags supplied by
the user to bond_arp_send(). If vlan tags have not been
supplied the vlans in the path to the target will be
discovered by bond_verify_device_path().

bond_uninit() is also updated to free vlan tags when a
bond is destroyed.

Signed-off-by: David Wilder <wilder@us.ibm.com>
---
 drivers/net/bonding/bond_main.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 3f35303b4920..5c2584ec544b 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3064,18 +3064,21 @@ struct bond_vlan_tag *bond_verify_device_path(struct net_device *start_dev,
 
 static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 {
-	struct rtable *rt;
-	struct bond_vlan_tag *tags;
 	struct bond_arp_target *targets = bond->params.arp_targets;
+	char pbuf[BOND_OPTION_STRING_MAX_SIZE];
+	struct bond_vlan_tag *tags;
 	__be32 target_ip, addr;
+	struct rtable *rt;
+	u32 flags;
 	int i;
 
 	for (i = 0; i < BOND_MAX_ARP_TARGETS && targets[i].target_ip; i++) {
 		target_ip = targets[i].target_ip;
 		tags = targets[i].tags;
+		flags = targets[i].flags;
 
-		slave_dbg(bond->dev, slave->dev, "%s: target %pI4\n",
-			  __func__, &target_ip);
+		slave_dbg(bond->dev, slave->dev, "%s: target %s\n", __func__,
+			  bond_arp_target_to_string(&targets[i], pbuf, sizeof(pbuf)));
 
 		/* Find out through which dev should the packet go */
 		rt = ip_route_output(dev_net(bond->dev), target_ip, 0, 0, 0,
@@ -3097,9 +3100,11 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 		if (rt->dst.dev == bond->dev)
 			goto found;
 
-		rcu_read_lock();
-		tags = bond_verify_device_path(bond->dev, rt->dst.dev, 0);
-		rcu_read_unlock();
+		if (!tags) {
+			rcu_read_lock();
+			tags = bond_verify_device_path(bond->dev, rt->dst.dev, 0);
+			rcu_read_unlock();
+		}
 
 		if (!IS_ERR_OR_NULL(tags))
 			goto found;
@@ -3115,7 +3120,8 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 		addr = bond_confirm_addr(rt->dst.dev, target_ip, 0);
 		ip_rt_put(rt);
 		bond_arp_send(slave, ARPOP_REQUEST, target_ip, addr, tags);
-		kfree(tags);
+		if  (!(flags & BOND_TARGET_USERTAGS))
+			kfree(tags);
 	}
 }
 
@@ -6047,6 +6053,7 @@ static void bond_uninit(struct net_device *bond_dev)
 	bond_for_each_slave(bond, slave, iter)
 		__bond_release_one(bond_dev, slave->dev, true, true);
 	netdev_info(bond_dev, "Released all slaves\n");
+	bond_free_vlan_tags(bond->params.arp_targets);
 
 #ifdef CONFIG_XFRM_OFFLOAD
 	mutex_destroy(&bond->ipsec_lock);
-- 
2.50.1

[PATCH net-next v13 6/7] bonding: Update for extended arp_ip_target format.

[David Wilder]

Updated bond_fill_info() to support extended arp_ip_target format.

Forward and backward compatibility between the kernel and iproute2 is
preserved.

Signed-off-by: David Wilder <wilder@us.ibm.com>
---
 drivers/net/bonding/bond_netlink.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
index 15782745fa4d..d1946d387e95 100644
--- a/drivers/net/bonding/bond_netlink.c
+++ b/drivers/net/bonding/bond_netlink.c
@@ -678,6 +678,7 @@ static int bond_fill_info(struct sk_buff *skb,
 			  const struct net_device *bond_dev)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
+	struct bond_arp_target *arptargets;
 	unsigned int packets_per_slave;
 	int ifindex, i, targets_added;
 	struct nlattr *targets;
@@ -716,12 +717,31 @@ static int bond_fill_info(struct sk_buff *skb,
 		goto nla_put_failure;
 
 	targets_added = 0;
-	for (i = 0; i < BOND_MAX_ARP_TARGETS; i++) {
-		if (bond->params.arp_targets[i].target_ip) {
-			if (nla_put_be32(skb, i, bond->params.arp_targets[i].target_ip))
-				goto nla_put_failure;
-			targets_added = 1;
+
+	arptargets = bond->params.arp_targets;
+	for (i = 0; i < BOND_MAX_ARP_TARGETS && arptargets[i].target_ip ; i++) {
+		struct data {
+			__be32 addr;
+			struct bond_vlan_tag vlans[BOND_MAX_VLAN_TAGS + 1];
+		} __packed data;
+		int level, size;
+
+		data.addr = arptargets[i].target_ip;
+		size = sizeof(__be32);
+		targets_added = 1;
+
+		if (arptargets[i].flags & BOND_TARGET_USERTAGS) {
+			for (level = 0; level < BOND_MAX_VLAN_TAGS + 1 ; level++) {
+				data.vlans[level].vlan_proto = arptargets[i].tags[level].vlan_proto;
+				data.vlans[level].vlan_id = arptargets[i].tags[level].vlan_id;
+				size = size + sizeof(struct bond_vlan_tag);
+				if (arptargets[i].tags[level].vlan_proto == BOND_VLAN_PROTO_NONE)
+					break;
+				}
 		}
+
+		if (nla_put(skb, i, size, &data))
+			goto nla_put_failure;
 	}
 
 	if (targets_added)
-- 
2.50.1

[PATCH net-next v13 7/7] bonding: Selftest and documentation for the arp_ip_target parameter.

[David Wilder]

Selftest provided as a functional test for the arp_ip_target parameter
both with and without user supplied vlan tags. Bonding documentation
has been updated for the arp_ip_target option.

Signed-off-by: David Wilder <wilder@us.ibm.com>
---
 Documentation/networking/bonding.rst          |  11 +
 .../selftests/drivers/net/bonding/Makefile    |   1 +
 .../drivers/net/bonding/bond-arp-ip-target.sh | 205 ++++++++++++++++++
 3 files changed, 217 insertions(+)
 create mode 100755 tools/testing/selftests/drivers/net/bonding/bond-arp-ip-target.sh

diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
index e700bf1d095c..08a3191a0322 100644
--- a/Documentation/networking/bonding.rst
+++ b/Documentation/networking/bonding.rst
@@ -330,6 +330,17 @@ arp_ip_target
 	maximum number of targets that can be specified is 16.  The
 	default value is no IP addresses.
 
+        When an arp_ip_target is configured the bonding driver will
+        attempt to automatically determine what vlans the arp probe will
+        pass through. This process of gathering vlan tags is required
+        for the arp probe to be sent. However, in some configurations
+        this process may fail. In these cases you may manually
+        supply a list of vlan tags. To specify a list of vlan tags
+        append the ipv4 address with [tag1/tag2...]. For example:
+        arp_ip_target=10.0.0.1[10]. If you simply need to disable the
+        vlan discovery process you may provide an empty list, for example:
+        arp_ip_target=10.0.0.1[].
+
 ns_ip6_target
 
 	Specifies the IPv6 addresses to use as IPv6 monitoring peers when
diff --git a/tools/testing/selftests/drivers/net/bonding/Makefile b/tools/testing/selftests/drivers/net/bonding/Makefile
index 402d4ee84f2e..b9594790e6df 100644
--- a/tools/testing/selftests/drivers/net/bonding/Makefile
+++ b/tools/testing/selftests/drivers/net/bonding/Makefile
@@ -3,6 +3,7 @@
 
 TEST_PROGS := \
 	bond-arp-interval-causes-panic.sh \
+	bond-arp-ip-target.sh \
 	bond-break-lacpdu-tx.sh \
 	bond-eth-type-change.sh \
 	bond-lladdr-target.sh \
diff --git a/tools/testing/selftests/drivers/net/bonding/bond-arp-ip-target.sh b/tools/testing/selftests/drivers/net/bonding/bond-arp-ip-target.sh
new file mode 100755
index 000000000000..40cbd56c00f9
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/bonding/bond-arp-ip-target.sh
@@ -0,0 +1,205 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# Test bonding arp_ip_target.
+# Topology for Bond mode 1,5,6 testing
+#
+#  +-------------------------+
+#  |                         | Server
+#  |        bond0.10.20      | 192.20.2.1/24
+#  |            |            |
+#  |         bond0.10        | 192.10.2.1/24
+#  |            |            |
+#  |          bond0          | 192.0.2.1/24
+#  |            |            |
+#  |            +            |
+#  |      eth0  |  eth1      |
+#  |        +---+---+        |
+#  |        |       |        |
+#  +-------------------------+
+#           |       |
+#  +-------------------------+
+#  |        |       |        |
+#  |    +---+-------+---+    |  Gateway
+#  |    |      br0      |    |
+#  |    +-------+-------+    |
+#  |            |            |
+#  +-------------------------+
+#               |
+#  +-------------------------+
+#  |            |            |  Client
+#  |          eth0           | 192.0.0.2/24
+#  |            |            |
+#  |         eth0.10         | 192.10.10.2/24
+#  |            |            |
+#  |        eth0.10.20       | 192.20.20.2/24
+#  +-------------------------+
+
+# shellcheck disable=SC2317
+
+lib_dir=$(dirname "$0")
+
+# shellcheck source=/dev/null # Ignore source warning.
+source "${lib_dir}"/bond_topo_2d1c.sh
+
+# shellcheck disable=SC2154 # Ignore unassigned referenced warning.
+echo "${c_ns}" "${s_ns}" > /dev/null
+
+DEBUG=${DEBUG:-0}
+test "${DEBUG}" -ne 0 && set -x
+
+# vlan subnets
+c_ip4="192.0.2.10"
+c_ip4v10="192.10.2.10"
+c_ip4v20="192.20.2.10"
+
+export ALL_TESTS="
+    no_vlan_hints
+    with_vlan_hints
+"
+
+# Build stacked vlans on top of an interface.
+stack_vlans()
+{
+    RET=0
+    local interface="$1"
+    local ns=$2
+    local last="$interface"
+    local tags="10 20"
+
+    if ! ip -n "${ns}" link show "${interface}" > /dev/null; then
+        RET=1
+        msg="Failed to create ${interface}"
+        return 1
+    fi
+
+    if [ "$ns" == "${s_ns}" ]; then host=1; else host=10;fi
+
+    for tag in $tags; do
+        ip -n "${ns}" link add link "$last" name "$last"."$tag" type vlan id "$tag"
+        ip -n "${ns}" address add 192."$tag".2."$host"/24 dev "$last"."$tag"
+        ip -n "${ns}" link set up dev "$last"."$tag"
+        last=$last.$tag
+    done
+}
+
+wait_for_arp_request()
+{
+	local target=$1
+	local ip
+	local interface
+
+	ip=$(echo "${target}" | awk -F "[" '{print $1}')
+	interface="$(ip -n "${c_ns}" -br addr show | grep "${ip}" | awk -F @ '{print $1}')"
+
+	tc -n "${c_ns}" qdisc add dev "${interface}" clsact
+	tc -n "${c_ns}" filter add dev "${interface}" ingress protocol arp \
+                handle 101 flower skip_hw arp_op request arp_tip "${ip}" action pass
+
+	slowwait_for_counter 5 5 tc_rule_handle_stats_get \
+                "dev ${interface} ingress" 101 ".packets" "-n ${c_ns}" &> /dev/null || RET=1
+
+	tc -n "${c_ns}" filter del dev "${interface}" ingress
+	tc -n "${c_ns}" qdisc del dev "${interface}" clsact
+
+	if [ "$RET" -ne 0 ]; then
+		msg="Arp probe not received by ${interface}"
+		return 1
+	fi
+}
+
+# Check for link flapping.
+# First verify the arp requests are being received
+# by the target.  Then verify that the Link Failure
+# Counts are not increasing over time.
+# Arp probes are sent every 100ms, two probes must
+# be missed to trigger a slave failure. A one second
+# wait should be sufficient.
+check_failure_count()
+{
+    local bond=$1
+    local target=$2
+    local proc_file=/proc/net/bonding/${bond}
+
+    wait_for_arp_request "${target}" || return 1
+
+    LinkFailureCount1=$(ip netns exec "${s_ns}" grep -F "Link Failure Count" "${proc_file}" \
+            | awk -F: '{ sum += $2 } END { print sum }')
+    sleep 1
+    LinkFailureCount2=$(ip netns exec "${s_ns}" grep -F "Link Failure Count" "${proc_file}" \
+            | awk -F: '{ sum += $2 } END { print sum }')
+
+    [ "$LinkFailureCount1" != "$LinkFailureCount2" ] && RET=1
+}
+
+setup_bond_topo()
+{
+    setup_prepare
+    setup_wait
+    stack_vlans bond0 "${s_ns}"
+    stack_vlans eth0 "${c_ns}"
+}
+
+skip_with_vlan_hints()
+{
+    # check if iproute supports arp_ip_target with vlans option.
+    if ! ip -n "${s_ns}" link add bond2 type bond arp_ip_target 10.0.0.1[10]; then
+        ip -n "${s_ns}" link del bond2 2> /dev/null
+        return 0
+    fi
+    return 1
+}
+
+no_vlan_hints()
+{
+        RET=0
+        local targets="${c_ip4} ${c_ip4v10} ${c_ip4v20}"
+        local target
+        msg=""
+
+        for target in $targets; do
+                bond_reset "mode $mode arp_interval 100 arp_ip_target ${target}"
+		stack_vlans bond0 "${s_ns}"
+                if [ "$RET" -ne 0 ]; then
+                    log_test "no_vlan_hints" "${msg}"
+                    return
+                fi
+                check_failure_count bond0 "${target}"
+		log_test "arp_ip_target=${target} ${msg}"
+        done
+}
+
+with_vlan_hints()
+{
+        RET=0
+        local targets="${c_ip4}[] ${c_ip4v10}[10] ${c_ip4v20}[10/20]"
+        local target
+        msg=""
+
+        if skip_with_vlan_hints; then
+            log_test_skip "skip_with_vlan_hints" \
+	          "Installed iproute doesn't support extended arp_ip_target options."
+            return 0
+        fi
+
+        for target in $targets; do
+                bond_reset "mode $mode arp_interval 100 arp_ip_target ${target}"
+                stack_vlans bond0 "${s_ns}"
+                if [ "$RET" -ne 0 ]; then
+                    log_test "no_vlan_hints" "${msg}"
+                    return
+                fi
+
+                check_failure_count bond0 "${target}"
+                log_test "arp_ip_target=${target} ${msg}"
+        done
+}
+
+trap cleanup EXIT
+
+mode=active-backup
+
+setup_bond_topo
+tests_run
+
+exit "$EXIT_STATUS"
-- 
2.50.1

Re: [PATCH net-next v13 3/7] bonding: arp_ip_target helpers.

[Paolo Abeni]

Hi,

On 10/14/25 1:52 AM, David Wilder wrote:
> Adding helpers and defines needed for extending the
> arp_ip_target parameters.
> 
> Signed-off-by: David Wilder <wilder@us.ibm.com>
> ---
>  include/net/bonding.h | 50 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/include/net/bonding.h b/include/net/bonding.h
> index 3497e5061f90..62359b34b69c 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -809,4 +809,54 @@ static inline netdev_tx_t bond_tx_drop(struct net_device *dev, struct sk_buff *s
>  	return NET_XMIT_DROP;
>  }
>  
> +/* Helpers for handling arp_ip_target */
> +#define BOND_OPTION_STRING_MAX_SIZE 64
> +#define BOND_MAX_VLAN_TAGS 5
> +#define BOND_VLAN_PROTO_NONE cpu_to_be16(0xffff)
> +
> +static inline char *bond_arp_target_to_string(struct bond_arp_target *target,
> +					      char *buf, int size)

I'm sorry for not catching this before; 'target' argument should be const:

	const struct bond_arp_target *target

> +{
> +	struct bond_vlan_tag *tags = target->tags;
> +	int i, num = 0;
> +
> +	if (!(target->flags & BOND_TARGET_USERTAGS)) {
> +		num = snprintf(&buf[0], size, "%pI4", &target->target_ip);
> +		return buf;
> +	}
> +
> +	num = snprintf(&buf[0], size, "%pI4[", &target->target_ip);
> +	if (tags) {
> +		for (i = 0; (tags[i].vlan_proto != BOND_VLAN_PROTO_NONE); i++) {
> +			if (!tags[i].vlan_id)
> +				continue;
> +			if (i != 0)
> +				num = num + snprintf(&buf[num], size - num, "/");
> +			num = num + snprintf(&buf[num], size - num, "%u",
> +					     tags[i].vlan_id);

The above is safe under the assumption that the provided buffer size is
large enough to fit all the tags. The above limits respect this
constraint, but some build time checks could possibly help.

/P

Re: [PATCH net-next v13 4/7] bonding: Processing extended arp_ip_target from user space.

[Paolo Abeni]

On 10/14/25 1:52 AM, David Wilder wrote:
> @@ -31,8 +32,10 @@ static int bond_option_use_carrier_set(struct bonding *bond,
>  				       const struct bond_opt_value *newval);
>  static int bond_option_arp_interval_set(struct bonding *bond,
>  					const struct bond_opt_value *newval);
> -static int bond_option_arp_ip_target_add(struct bonding *bond, __be32 target);
> -static int bond_option_arp_ip_target_rem(struct bonding *bond, __be32 target);
> +static int bond_option_arp_ip_target_add(struct bonding *bond,
> +					 struct bond_arp_target target);

I guess you passed the 'target' by value instead of using a 'const
struct bond_arp_target *' argument to reduce the code delta?

I think it would be clear the other option, it will make the intent clear.

/P

Re: [PATCH net-next v13 5/7] bonding: Update to bond_arp_send_all() to use supplied vlan tags

[Paolo Abeni]

On 10/14/25 1:52 AM, David Wilder wrote:
> @@ -3097,9 +3100,11 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
>  		if (rt->dst.dev == bond->dev)
>  			goto found;
>  
> -		rcu_read_lock();
> -		tags = bond_verify_device_path(bond->dev, rt->dst.dev, 0);
> -		rcu_read_unlock();
> +		if (!tags) {
> +			rcu_read_lock();
> +			tags = bond_verify_device_path(bond->dev, rt->dst.dev, 0);
> +			rcu_read_unlock();
> +		}
>  
>  		if (!IS_ERR_OR_NULL(tags))
>  			goto found;
> @@ -3115,7 +3120,8 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
>  		addr = bond_confirm_addr(rt->dst.dev, target_ip, 0);
>  		ip_rt_put(rt);
>  		bond_arp_send(slave, ARPOP_REQUEST, target_ip, addr, tags);
> -		kfree(tags);
> +		if  (!(flags & BOND_TARGET_USERTAGS))
> +			kfree(tags);

It's IMHO not obvious from the above code that `tags` is allocated (by
bond_verify_device_path() if and only if !(flags & BOND_TARGET_USERTAGS)

I think it would be more clear if you use the same condition for both
the allocation and the free calls.

/P

Re: [PATCH net-next v13 0/7] bonding: Extend arp_ip_target format to allow for a list of vlan tags.

[Nikolay Aleksandrov]

On 10/14/25 02:52, David Wilder wrote:
> The current implementation of the arp monitor builds a list of vlan-tags by
> following the chain of net_devices above the bond. See bond_verify_device_path().
> Unfortunately, with some configurations, this is not possible. One example is
> when an ovs switch is configured above the bond.
> 
> This change extends the "arp_ip_target" parameter format to allow for a list of
> vlan tags to be included for each arp target. This new list of tags is optional
> and may be omitted to preserve the current format and process of discovering
> vlans.
> 
> The new format for arp_ip_target is:
> arp_ip_target ipv4-address[vlan-tag\...],...
> 
> For example:
> arp_ip_target 10.0.0.1[10/20]
> arp_ip_target 10.0.0.1[] (used to disable vlan discovery)
> 
> Changes since V12
> Fixed uninitialized variable in bond_option_arp_ip_targets_set() (patch 4)
> causing a CI failure.
> 
> Changes since V11
> No Change.
> 
> Changes since V10
> Thanks Paolo:
> - 1/7 Changed the layout of struct bond_arp_target to reduce size of the struct.
> - 3/7 Fixed format 'size-num' -> 'size - num'
> - 7/7 Updated selftest (bond-arp-ip-target.sh). Removed sleep 10 in check_failure_count().
>       Added call to tc to verify arp probes are reaching the target interface. Then I verify that
>       the Link Failure counts are not increasing over "time".  Arp probes are sent every 100ms,
>       two missed probes will trigger a Link failure. A one second wait between checking counts
>       should be be more than sufficient.  This speeds up the execution of the test.
> 
> Thanks Nikolay:
> - 4/7 In bond_option_arp_ip_targets_clear() I changed the definition of empty_target to empty_target = {}.
> -     bond_validate_tags() now verifies input is a multiple of sizeof(struct bond_vlan_tag).
>       Updated VID validity check to use: !tags->vlan_id || tags->vlan_id >= VLAN_VID_MASK) as suggested.
> -     In bond_option_arp_ip_targets_set() removed the redundant length check of target.target_ip.
> -     Added kfree(target.tags) when bond_option_arp_ip_target_add() results in an error.
> -     Removed the caching of struct bond_vlan_tag returned by bond_verify_device_path(), Nikolay
>       pointed out that caching tags prevented the detection of VLAN configuration changes. 
>       Added a kfree(tags) for tags allocated in bond_verify_device_path().
> 
> Jay, Nikolay and I had a discussion regarding locking when adding, deleting or changing vlan tags.
> Jay pointed out that user supplied tags that are stashed in the bond configuration and can only be
> changed via user space this can be done safely in an RCU manner as netlink always operates with RTNL
> held. If user space provided tags and then replumbs things, it'll be on user space to update the tags
> in a safe manor.  
> 
> I was concerned about changing options on a configured bond,  I found that attempting to change
> a bonds configuration (using "ip set") will abort the attempt to make a change if the bond's state is
> "UP" or has slaves configured. Therefor the configuration and operational side of a bond is separated.
> I agree with Jay that the existing locking scheme is sufficient.
> 
> Change since V9
> Fix kdoc build error.
> 
> Changes since V8:
> Moved the #define BOND_MAX_VLAN_TAGS from patch 6 to patch 3.
> Thanks Simon for catching the bisection break.
> 
> Changes since V7:
> These changes should eliminate the CI failures I have been seeing.
> 1) patch 2, changed type of bond_opt_value.extra_len to size_t.
> 2) Patch 4, added bond_validate_tags() to validate the array of bond_vlan_tag provided by
>  the user.
> 
> Changes since V6:
> 1) I made a number of changes to fix the failure seen in the
> kernel CI.  I am still unable to reproduce the this failure, hopefully I
> have fixed it.  These change are in patch #4 to functions:
> bond_option_arp_ip_targets_clear() and
> bond_option_arp_ip_targets_set()
> 
> Changes since V5: Only the last 2 patches have changed since V5.
> 1) Fixed sparse warning in bond_fill_info().
> 2) Also in bond_fill_info() I resolved data.addr uninitialized when if condition is not met.
> Thank you Simon for catching this. Note: The change is different that what I shared earlier.
> 3) Fixed shellcheck warnings in test script: Blocked source warning, Ignored specific unassigned
> references and exported ALL_TESTS to resolve a reference warning.
> 
> Changes since V4:
> 1)Dropped changes to proc and sysfs APIs to bonding.  These APIs 
> do not need to be updated to support new functionality.  Netlink
> and iproute2 have been updated to do the right thing, but the
> other APIs are more or less frozen in the past.
> 
> 2)Jakub reported a warning triggered in bond_info_seq_show() during
> testing.  I was unable to reproduce this warning or identify
> it with code inspection.  However, all my changes to bond_info_seq_show()
> have been dropped as unnecessary (see above).
> Hopefully this will resolve the issue. 
> 
> 3)Selftest script has been updated based on the results of shellcheck.
> Two unresolved references that are not possible to resolve are all
> that remain.
> 
> 4)A patch was added updating bond_info_fill()
> to support "ip -d show <bond-device>" command.
> 
> The inclusion of a list of vlan tags is optional. The new logic
> preserves both forward and backward compatibility with the kernel
> and iproute2 versions.
> 
> Changes since V3:
> 1) Moved the parsing of the extended arp_ip_target out of the kernel and into
>    userspace (ip command). A separate patch to iproute2 to follow shortly.
> 2) Split up the patch set to make review easier.
> 
> Please see iproute changes in a separate posting.
> 
> Thank you for your time and reviews.
> 
> Signed-off-by: David Wilder <wilder@us.ibm.com>
> David Wilder (7):
>   bonding: Adding struct bond_arp_target
>   bonding: Adding extra_len field to struct bond_opt_value.
>   bonding: arp_ip_target helpers.
>   bonding: Processing extended arp_ip_target from user space.
>   bonding: Update to bond_arp_send_all() to use supplied vlan tags
>   bonding: Update for extended arp_ip_target format.
>   bonding: Selftest and documentation for the arp_ip_target parameter.
> 
>  Documentation/networking/bonding.rst          |  11 +
>  drivers/net/bonding/bond_main.c               |  48 ++--
>  drivers/net/bonding/bond_netlink.c            |  35 ++-
>  drivers/net/bonding/bond_options.c            | 140 +++++++++---
>  drivers/net/bonding/bond_procfs.c             |   4 +-
>  drivers/net/bonding/bond_sysfs.c              |   4 +-
>  include/net/bond_options.h                    |  29 ++-
>  include/net/bonding.h                         |  65 +++++-
>  .../selftests/drivers/net/bonding/Makefile    |   1 +
>  .../drivers/net/bonding/bond-arp-ip-target.sh | 205 ++++++++++++++++++
>  10 files changed, 467 insertions(+), 75 deletions(-)
>  create mode 100755 tools/testing/selftests/drivers/net/bonding/bond-arp-ip-target.sh
> 

I remember reviewing previous versions of this patch-set, please CC all
reviewers for future versions.

Thanks,
 Nik

Re: [PATCH net-next v13 6/7] bonding: Update for extended arp_ip_target format.

[Paolo Abeni]

On 10/14/25 1:52 AM, David Wilder wrote:
> diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
> index 15782745fa4d..d1946d387e95 100644
> --- a/drivers/net/bonding/bond_netlink.c
> +++ b/drivers/net/bonding/bond_netlink.c
> @@ -678,6 +678,7 @@ static int bond_fill_info(struct sk_buff *skb,
>  			  const struct net_device *bond_dev)
>  {
>  	struct bonding *bond = netdev_priv(bond_dev);
> +	struct bond_arp_target *arptargets;
>  	unsigned int packets_per_slave;
>  	int ifindex, i, targets_added;
>  	struct nlattr *targets;
> @@ -716,12 +717,31 @@ static int bond_fill_info(struct sk_buff *skb,
>  		goto nla_put_failure;
>  
>  	targets_added = 0;
> -	for (i = 0; i < BOND_MAX_ARP_TARGETS; i++) {
> -		if (bond->params.arp_targets[i].target_ip) {
> -			if (nla_put_be32(skb, i, bond->params.arp_targets[i].target_ip))
> -				goto nla_put_failure;
> -			targets_added = 1;
> +
> +	arptargets = bond->params.arp_targets;
> +	for (i = 0; i < BOND_MAX_ARP_TARGETS && arptargets[i].target_ip ; i++) {
> +		struct data {
> +			__be32 addr;
> +			struct bond_vlan_tag vlans[BOND_MAX_VLAN_TAGS + 1];
> +		} __packed data;
> +		int level, size;
> +
> +		data.addr = arptargets[i].target_ip;
> +		size = sizeof(__be32);
> +		targets_added = 1;
> +
> +		if (arptargets[i].flags & BOND_TARGET_USERTAGS) {
> +			for (level = 0; level < BOND_MAX_VLAN_TAGS + 1 ; level++) {
> +				data.vlans[level].vlan_proto = arptargets[i].tags[level].vlan_proto;
> +				data.vlans[level].vlan_id = arptargets[i].tags[level].vlan_id;
> +				size = size + sizeof(struct bond_vlan_tag);
> +				if (arptargets[i].tags[level].vlan_proto == BOND_VLAN_PROTO_NONE)
> +					break;
> +				}

Apparently there is an extra tab above. This has been already noted in a
previous revision.

>  		}
> +
> +		if (nla_put(skb, i, size, &data))
> +			goto nla_put_failure;
>  	}
>  
>  	if (targets_added)

I guess you should update bond_get_size() accordingly???

Also changing the binary layout of an existing NL type does not feel
safe. @Jakub: is that something we can safely allow?

/P

Re: [PATCH net-next v13 7/7] bonding: Selftest and documentation for the arp_ip_target parameter.

[Paolo Abeni]

On 10/14/25 1:52 AM, David Wilder wrote:
> +# Build stacked vlans on top of an interface.
> +stack_vlans()
> +{
> +    RET=0
> +    local interface="$1"
> +    local ns=$2
> +    local last="$interface"
> +    local tags="10 20"
> +
> +    if ! ip -n "${ns}" link show "${interface}" > /dev/null; then
> +        RET=1
> +        msg="Failed to create ${interface}"
> +        return 1
> +    fi
> +
> +    if [ "$ns" == "${s_ns}" ]; then host=1; else host=10;fi
> +
> +    for tag in $tags; do
> +        ip -n "${ns}" link add link "$last" name "$last"."$tag" type vlan id "$tag"
> +        ip -n "${ns}" address add 192."$tag".2."$host"/24 dev "$last"."$tag"
> +        ip -n "${ns}" link set up dev "$last"."$tag"
> +        last=$last.$tag
> +    done
> +}
> +
> +wait_for_arp_request()
> +{
> +	local target=$1

The indentation is inconsistent. Please always use a single tab.

> +	local ip
> +	local interface
> +
> +	ip=$(echo "${target}" | awk -F "[" '{print $1}')
> +	interface="$(ip -n "${c_ns}" -br addr show | grep "${ip}" | awk -F @ '{print $1}')"
> +
> +	tc -n "${c_ns}" qdisc add dev "${interface}" clsact
> +	tc -n "${c_ns}" filter add dev "${interface}" ingress protocol arp \
> +                handle 101 flower skip_hw arp_op request arp_tip "${ip}" action pass
> +
> +	slowwait_for_counter 5 5 tc_rule_handle_stats_get \
> +                "dev ${interface} ingress" 101 ".packets" "-n ${c_ns}" &> /dev/null || RET=1
> +
> +	tc -n "${c_ns}" filter del dev "${interface}" ingress
> +	tc -n "${c_ns}" qdisc del dev "${interface}" clsact
> +
> +	if [ "$RET" -ne 0 ]; then
> +		msg="Arp probe not received by ${interface}"
> +		return 1
> +	fi
> +}
> +
> +# Check for link flapping.
> +# First verify the arp requests are being received
> +# by the target.  Then verify that the Link Failure
> +# Counts are not increasing over time.
> +# Arp probes are sent every 100ms, two probes must
> +# be missed to trigger a slave failure. A one second
> +# wait should be sufficient.
> +check_failure_count()
> +{
> +    local bond=$1
> +    local target=$2
> +    local proc_file=/proc/net/bonding/${bond}
> +
> +    wait_for_arp_request "${target}" || return 1
> +
> +    LinkFailureCount1=$(ip netns exec "${s_ns}" grep -F "Link Failure Count" "${proc_file}" \
> +            | awk -F: '{ sum += $2 } END { print sum }')
> +    sleep 1
> +    LinkFailureCount2=$(ip netns exec "${s_ns}" grep -F "Link Failure Count" "${proc_file}" \
> +            | awk -F: '{ sum += $2 } END { print sum }')
> +
> +    [ "$LinkFailureCount1" != "$LinkFailureCount2" ] && RET=1
> +}
> +
> +setup_bond_topo()
> +{
> +    setup_prepare
> +    setup_wait
> +    stack_vlans bond0 "${s_ns}"
> +    stack_vlans eth0 "${c_ns}"
> +}
> +
> +skip_with_vlan_hints()
> +{
> +    # check if iproute supports arp_ip_target with vlans option.
> +    if ! ip -n "${s_ns}" link add bond2 type bond arp_ip_target 10.0.0.1[10]; then
> +        ip -n "${s_ns}" link del bond2 2> /dev/null
> +        return 0
> +    fi
> +    return 1
> +}
> +
> +no_vlan_hints()
> +{
> +        RET=0
> +        local targets="${c_ip4} ${c_ip4v10} ${c_ip4v20}"
> +        local target
> +        msg=""
> +
> +        for target in $targets; do
> +                bond_reset "mode $mode arp_interval 100 arp_ip_target ${target}"
> +		stack_vlans bond0 "${s_ns}"
> +                if [ "$RET" -ne 0 ]; then
> +                    log_test "no_vlan_hints" "${msg}"
> +                    return
> +                fi
> +                check_failure_count bond0 "${target}"
> +		log_test "arp_ip_target=${target} ${msg}"

Please fix the alignment above.

/P

Re: [PATCH net-next v13 6/7] bonding: Update for extended arp_ip_target format.

[Jakub Kicinski]

On Thu, 16 Oct 2025 13:50:52 +0200 Paolo Abeni wrote:
> > +		if (nla_put(skb, i, size, &data))
> > +			goto nla_put_failure;
> >  	}
> >  
> >  	if (targets_added)  
> 
> I guess you should update bond_get_size() accordingly???
> 
> Also changing the binary layout of an existing NL type does not feel
> safe. @Jakub: is that something we can safely allow?

In general extending attributes is fine, but going from a scalar 
to a struct is questionable. YNL for example will not allow it.

I haven't looked at the series more closely until now.

Why are there multiple vlan tags per target?

Is this configuration really something we should support in the kernel?
IDK how much we should push "OvS-compatibility" into other parts of the
stack. If user knows that they have to apply this funny configuration
on the bond maybe they should just arp from user space?

RE: [PATCH net-next v13 6/7] bonding: Update for extended arp_ip_target format.

[David Wilder]

________________________________________
From: Jakub Kicinski <kuba@kernel.org>
Sent: Thursday, October 16, 2025 12:49 PM
To: Paolo Abeni
Cc: David Wilder; netdev@vger.kernel.org; jv@jvosburgh.net; Pradeep Satyanarayana; i.maximets@ovn.org; Adrian Moreno Zapata; Hangbin Liu; stephen@networkplumber.org; horms@kernel.org; andrew+netdev@lunn.ch; edumazet@google.com
Subject: [EXTERNAL] Re: [PATCH net-next v13 6/7] bonding: Update for extended arp_ip_target format.

> On Thu, 16 Oct 2025 13:50:52 +0200 Paolo Abeni wrote:
> > > +           if (nla_put(skb, i, size, &data))
> > > +                   goto nla_put_failure;
> > >     }
> > >
> > >     if (targets_added)
> >
> > I guess you should update bond_get_size() accordingly???
> >
> > Also changing the binary layout of an existing NL type does not feel
> > safe. @Jakub: is that something we can safely allow?
>
> In general extending attributes is fine, but going from a scalar
> to a struct is questionable. YNL for example will not allow it.

I am not sure I understand your concern. I have change the
netlink socket payload from a fixed 4 bytes to a variable number of bytes.
4 bytes for ipv4 address followed by some number of bytes with the
list of vlans, could be zero. Netlink sockets just need to be told the
payload size.  Or have I missed the point?

>
> I haven't looked at the series more closely until now.
>
> Why are there multiple vlan tags per target?

You can have a vlan inside a vlan, the original arp_ip_target
option code supported this.

>
> Is this configuration really something we should support in the kernel?
> IDK how much we should push "OvS-compatibility" into other parts of the
> stack. If user knows that they have to apply this funny configuration
> on the bond maybe they should just arp from user space?

This change is not just for compatibility with OVS. Ilya Maximets pointed out:
"..this is true not only for OVS.  You can add various TC qdiscs onto the
interface that will break all those assumptions as well, for example.  Loaded
BPF/XDP programs will too."

When using the arp_ip_target option the bond driver must discover what
vlans are in the path to the target. These special arps must be generated by
the bonding driver to know what bonded slave the packets is to sent and
received on and at what frequency.

When the the arp_ip_target feature was first introduced discovering vlans in the
path to the target was easy by following the linked net_devices. As our
networking code has evolved this is no longer possible with all configurations
as Ilya pointed out.  What I have done is provide alternate way to provide the
list of vlans so this desirable feature can continue to function.

Regards
  David Wilder

RE: [PATCH net-next v13 6/7] bonding: Update for extended arp_ip_target format.

[David Wilder]

________________________________________
From: David Wilder <wilder@us.ibm.com>
Sent: Thursday, October 16, 2025 5:21 PM
To: Jakub Kicinski; Paolo Abeni
Cc: netdev@vger.kernel.org; jv@jvosburgh.net; Pradeep Satyanarayana; i.maximets@ovn.org; Adrian Moreno Zapata; Hangbin Liu; stephen@networkplumber.org; horms@kernel.org; andrew+netdev@lunn.ch; edumazet@google.com; Nikolay Aleksandrov
Subject: Re: [EXTERNAL] Re: [PATCH net-next v13 6/7] bonding: Update for extended arp_ip_target format.



>> On Thu, 16 Oct 2025 13:50:52 +0200 Paolo Abeni wrote:
>> > > +           if (nla_put(skb, i, size, &data))
>> > > +                   goto nla_put_failure;
>> > >     }
>> > >
>> > >     if (targets_added)
>> >
>> > I guess you should update bond_get_size() accordingly???
>> >
>> > Also changing the binary layout of an existing NL type does not feel
>> > safe. @Jakub: is that something we can safely allow?
>>
>> In general extending attributes is fine, but going from a scalar
>> to a struct is questionable. YNL for example will not allow it.

> I am not sure I understand your concern. I have change the
> netlink socket payload from a fixed 4 bytes to a variable number of bytes.
> 4 bytes for ipv4 address followed by some number of bytes with the
> list of vlans, could be zero. Netlink sockets just need to be told the
> payload size.  Or have I missed the point?

Is the concern here the variable size of the payload?

I have updated bond_get_size() to use the maximum size of the payload so the payload allocation should correct :

+struct bond_arp_ip_target_payload {
+       __be32 addr;
+       struct bond_vlan_tag vlans[BOND_MAX_VLAN_TAGS + 1];
+} __packed;
+
                                                /* IFLA_BOND_ARP_IP_TARGET */
                nla_total_size(sizeof(struct nlattr)) +
-               nla_total_size(sizeof(u32)) * BOND_MAX_ARP_TARGETS +
+               nla_total_size(sizeof(struct bond_arp_ip_target_payload)) * (BOND_MAX_ARP_TARGETS + 1) +

>>
>> I haven't looked at the series more closely until now.
>>
>> Why are there multiple vlan tags per target?
>
> You can have a vlan inside a vlan, the original arp_ip_target
> option code supported this.
>
>>
>> Is this configuration really something we should support in the kernel?
>> IDK how much we should push "OvS-compatibility" into other parts of the
>> stack. If user knows that they have to apply this funny configuration
>> on the bond maybe they should just arp from user space?

> This change is not just for compatibility with OVS. Ilya Maximets pointed out:
> "..this is true not only for OVS.  You can add various TC qdiscs onto the
> interface that will break all those assumptions as well, for example.  Loaded
> BPF/XDP programs will too."

> When using the arp_ip_target option the bond driver must discover what
> vlans are in the path to the target. These special arps must be generated by
> the bonding driver to know what bonded slave the packets is to sent and
> received on and at what frequency.
>
> When the the arp_ip_target feature was first introduced discovering vlans in the
> path to the target was easy by following the linked net_devices. As our
> networking code has evolved this is no longer possible with all configurations
> as Ilya pointed out.  What I have done is provide alternate way to provide the
> list of vlans so this desirable feature can continue to function.

Regards
  David Wilder

RE: [PATCH net-next v13 6/7] bonding: Update for extended arp_ip_target format.

[David Wilder]

________________________________________
From: David Wilder <wilder@us.ibm.com>
Sent: Tuesday, October 21, 2025 9:00 AM
To: Jakub Kicinski; Paolo Abeni
Cc: netdev@vger.kernel.org; jv@jvosburgh.net; Pradeep Satyanarayana; i.maximets@ovn.org; Adrian Moreno Zapata; Hangbin Liu; stephen@networkplumber.org; horms@kernel.org; andrew+netdev@lunn.ch; edumazet@google.com; Nikolay Aleksandrov
Subject: Re: [EXTERNAL] Re: [PATCH net-next v13 6/7] bonding: Update for extended arp_ip_target format.



>> On Thu, 16 Oct 2025 13:50:52 +0200 Paolo Abeni wrote:
>> > > +           if (nla_put(skb, i, size, &data))
>> > > +                   goto nla_put_failure;
>> > >     }
>> > >
>> > >     if (targets_added)
>> >
>> > I guess you should update bond_get_size() accordingly???
>> >
>> > Also changing the binary layout of an existing NL type does not feel
>> > safe. @Jakub: is that something we can safely allow?
>>
>> In general extending attributes is fine, but going from a scalar
>> to a struct is questionable. YNL for example will not allow it.

> I am not sure I understand your concern. I have change the
> netlink socket payload from a fixed 4 bytes to a variable number of bytes.
> 4 bytes for ipv4 address followed by some number of bytes with the
> list of vlans, could be zero. Netlink sockets just need to be told the
> payload size.  Or have I missed the point?

> Is the concern here the variable size of the payload?
>
> I have updated bond_get_size() to use the maximum size of the payload so the payload allocation should correct :

** Correction: 
+struct bond_arp_ip_target_payload {
+       __be32 addr;
+       struct bond_vlan_tag vlans[BOND_MAX_VLAN_TAGS + 1];
+} __packed;
+
                                                /* IFLA_BOND_ARP_IP_TARGET */
                nla_total_size(sizeof(struct nlattr)) +
-               nla_total_size(sizeof(u32)) * BOND_MAX_ARP_TARGETS +
+               nla_total_size(sizeof(struct bond_arp_ip_target_payload)) * BOND_MAX_ARP_TARGETS +

>>>
>>> I haven't looked at the series more closely until now.
>>>
>>> Why are there multiple vlan tags per target?
>>
>> You can have a vlan inside a vlan, the original arp_ip_target
>> option code supported this.
>>
>>>
>>> Is this configuration really something we should support in the kernel?
>>> IDK how much we should push "OvS-compatibility" into other parts of the
>>> stack. If user knows that they have to apply this funny configuration
>>> on the bond maybe they should just arp from user space?
>
>> This change is not just for compatibility with OVS. Ilya Maximets pointed out:
>> "..this is true not only for OVS.  You can add various TC qdiscs onto the
>> interface that will break all those assumptions as well, for example.  Loaded
>> BPF/XDP programs will too."
>
>> When using the arp_ip_target option the bond driver must discover what
>> vlans are in the path to the target. These special arps must be generated by
>> the bonding driver to know what bonded slave the packets is to sent and
>> received on and at what frequency.
>>
>> When the the arp_ip_target feature was first introduced discovering vlans in the
>> path to the target was easy by following the linked net_devices. As our
>> networking code has evolved this is no longer possible with all configurations
>> as Ilya pointed out.  What I have done is provide alternate way to provide the
>> list of vlans so this desirable feature can continue to function.
>
> Regards
>  David Wilder

Re: [PATCH net-next v13 6/7] bonding: Update for extended arp_ip_target format.

[Jakub Kicinski]

On Fri, 17 Oct 2025 00:21:02 +0000 David Wilder wrote:
> > > I guess you should update bond_get_size() accordingly???
> > >
> > > Also changing the binary layout of an existing NL type does not feel
> > > safe. @Jakub: is that something we can safely allow?  
> >
> > In general extending attributes is fine, but going from a scalar
> > to a struct is questionable. YNL for example will not allow it.  
> 
> I am not sure I understand your concern. I have change the
> netlink socket payload from a fixed 4 bytes to a variable number of bytes.
> 4 bytes for ipv4 address followed by some number of bytes with the
> list of vlans, could be zero. Netlink sockets just need to be told the
> payload size.  Or have I missed the point?

Are you replacing a line that says nla_put() which outputs raw bytes
or a line which says nla_put_x32() which outputs a scalar?
What I'm saying is that while growing raw byte attrs is pretty common
in Netlink, replacing a scalar with a struct may cause user space
to reject the attrs.

> > I haven't looked at the series more closely until now.
> >
> > Why are there multiple vlan tags per target?  
> 
> You can have a vlan inside a vlan, the original arp_ip_target
> option code supported this.
> 
> > Is this configuration really something we should support in the kernel?
> > IDK how much we should push "OvS-compatibility" into other parts of the
> > stack. If user knows that they have to apply this funny configuration
> > on the bond maybe they should just arp from user space?  
> 
> This change is not just for compatibility with OVS. Ilya Maximets pointed out:
> "..this is true not only for OVS.  You can add various TC qdiscs onto the
> interface that will break all those assumptions as well, for example.  Loaded
> BPF/XDP programs will too."
> 
> When using the arp_ip_target option the bond driver must discover what
> vlans are in the path to the target. These special arps must be generated by
> the bonding driver to know what bonded slave the packets is to sent and
> received on and at what frequency.
> 
> When the the arp_ip_target feature was first introduced discovering vlans in the
> path to the target was easy by following the linked net_devices. As our
> networking code has evolved this is no longer possible with all configurations
> as Ilya pointed out.  What I have done is provide alternate way to provide the
> list of vlans so this desirable feature can continue to function.

I understand your perspective. I'm not convinced that kernel must
support such custom configurations, if it can't infer the correct
behavior from information it already has.

I don't feel strongly about it, if you manage to collect a review 
tag from the bonding maintainers or another netdev maintainer I won't
stand in the way. Otherwise, given that the uAPI is questionable and
there's total of 0 review tags on v13, this series is starting to look
like a dead end.