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

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

[David J Wilder]

This is a followup to this discussion:
  https://www.spinics.net/lists/netdev/msg1085428.html

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 gathering tags.
When provided the list of tags circumvents the process of gathering tags by
using the supplied list. An empty list can be provided to simply skip the the
process of gathering tags.

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

Examples:
arp_ip_target=10.0.0.1,10.0.0.2 (Determine tags automatically for both targets)
arp_ip_target=10.0.0.1[]        (Don't add any tags, don't gather tags)
arp_ip_target=10.0.0.1[100/200] (Don't gather tags, use supplied list of tags)
arp_ip_target=10.0.0.1,10.0.0.2[100] (add vlan 100 tag for 10.0.0.2.
                                      Gather tags for 10.0.0.1.)

This is a work in process. I am requesting feedback on my approach.
This patch allows the extended format only when setting arp_ip_target values with
modules parameters.

TBD: Add support for sysfs, netlink and procfs.
TBD: Kernel self tests.
TBD: Documentation.

David J Wilder (2):
  bonding: Adding struct arp_target
  bonding: Extend arp_ip_target format to allow for a list of vlan tags.

 drivers/net/bonding/bond_main.c    | 163 ++++++++++++++++++++++++-----
 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/bonding.h              |  15 ++-
 6 files changed, 161 insertions(+), 47 deletions(-)

-- 
2.43.5

[PATCH net-next v1 1/2] bonding: Adding struct arp_target

[David J Wilder]

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

struct arp_target {
        __be32 target_ip;
        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 J 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/bonding.h              | 15 ++++++++++-----
 6 files changed, 41 insertions(+), 33 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index d05226484c64..ab388dab218a 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3151,26 +3151,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 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;
 		}
@@ -3188,15 +3191,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);
 	}
 }
@@ -6102,7 +6105,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 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;
@@ -6296,7 +6299,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 ac5e402c34bc..1a3d17754c0a 100644
--- a/drivers/net/bonding/bond_netlink.c
+++ b/drivers/net/bonding/bond_netlink.c
@@ -688,8 +688,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 91893c29b899..54940950079e 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -1090,7 +1090,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) {
@@ -1118,20 +1118,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 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 arp_target *targets = bond->params.arp_targets;
 	int ind;
 
 	if (!bond_is_ip_target_ok(target)) {
@@ -1166,7 +1166,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 arp_target *targets = bond->params.arp_targets;
 	struct list_head *iter;
 	struct slave *slave;
 	unsigned long *targets_rx;
@@ -1185,20 +1185,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 1e13bb170515..d7c09e0a14dd 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/bonding.h b/include/net/bonding.h
index 95f67b308c19..709ef9a302dd 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -115,6 +115,11 @@ static inline int is_netpoll_tx_blocked(struct net_device *dev)
 #define is_netpoll_tx_blocked(dev) (0)
 #endif
 
+struct arp_target {
+	__be32 target_ip;
+	struct bond_vlan_tag *tags;
+};
+
 struct bond_params {
 	int mode;
 	int xmit_policy;
@@ -135,7 +140,7 @@ struct bond_params {
 	int ad_select;
 	char primary[IFNAMSIZ];
 	int primary_reselect;
-	__be32 arp_targets[BOND_MAX_ARP_TARGETS];
+	struct arp_target arp_targets[BOND_MAX_ARP_TARGETS];
 	int tx_queues;
 	int all_slaves_active;
 	int resend_igmp;
@@ -522,7 +527,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];
 
@@ -760,14 +765,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 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.43.5

Re: [PATCH net-next v1 1/2] bonding: Adding struct arp_target

[Jay Vosburgh]

David J Wilder <wilder@us.ibm.com> wrote:

>Replacing the definition of bond_params.arp_targets (__be32 arp_targets[])
>with:
>
>struct arp_target {
>        __be32 target_ip;
>        struct bond_vlan_tag *tags;
>};

	Since this struct is only for bonding, perhaps "struct
bond_arp_target"?

>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 J 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/bonding.h              | 15 ++++++++++-----
> 6 files changed, 41 insertions(+), 33 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index d05226484c64..ab388dab218a 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -3151,26 +3151,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 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);

	Perhaps add the tags to the debug message?  It might be kind of
a pain to format, but seems like it would be useful.

	-J

> 
> 		/* 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;
> 		}
>@@ -3188,15 +3191,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);
> 	}
> }
>@@ -6102,7 +6105,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 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;
>@@ -6296,7 +6299,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 ac5e402c34bc..1a3d17754c0a 100644
>--- a/drivers/net/bonding/bond_netlink.c
>+++ b/drivers/net/bonding/bond_netlink.c
>@@ -688,8 +688,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 91893c29b899..54940950079e 100644
>--- a/drivers/net/bonding/bond_options.c
>+++ b/drivers/net/bonding/bond_options.c
>@@ -1090,7 +1090,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) {
>@@ -1118,20 +1118,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 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 arp_target *targets = bond->params.arp_targets;
> 	int ind;
> 
> 	if (!bond_is_ip_target_ok(target)) {
>@@ -1166,7 +1166,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 arp_target *targets = bond->params.arp_targets;
> 	struct list_head *iter;
> 	struct slave *slave;
> 	unsigned long *targets_rx;
>@@ -1185,20 +1185,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 1e13bb170515..d7c09e0a14dd 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/bonding.h b/include/net/bonding.h
>index 95f67b308c19..709ef9a302dd 100644
>--- a/include/net/bonding.h
>+++ b/include/net/bonding.h
>@@ -115,6 +115,11 @@ static inline int is_netpoll_tx_blocked(struct net_device *dev)
> #define is_netpoll_tx_blocked(dev) (0)
> #endif
> 
>+struct arp_target {
>+	__be32 target_ip;
>+	struct bond_vlan_tag *tags;
>+};
>+
> struct bond_params {
> 	int mode;
> 	int xmit_policy;
>@@ -135,7 +140,7 @@ struct bond_params {
> 	int ad_select;
> 	char primary[IFNAMSIZ];
> 	int primary_reselect;
>-	__be32 arp_targets[BOND_MAX_ARP_TARGETS];
>+	struct arp_target arp_targets[BOND_MAX_ARP_TARGETS];
> 	int tx_queues;
> 	int all_slaves_active;
> 	int resend_igmp;
>@@ -522,7 +527,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];
> 
>@@ -760,14 +765,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 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.43.5
>

---
	-Jay Vosburgh, jv@jvosburgh.net

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

[David J Wilder]

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 gathering tags.  When provided the list of tags circumvents
the process of gathering tags by using the supplied list. An empty list
can be provided to simply skip the process of gathering tags.

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

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index ab388dab218a..12195e60c7de 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2672,6 +2672,17 @@ static int __bond_release_one(struct net_device *bond_dev,
 	return 0;
 }
 
+/* helper to free arp_target.tags */
+static void free_tags(struct net_device *bond_dev)
+{
+	struct bonding *bond = netdev_priv(bond_dev);
+	struct arp_target *targets = bond->params.arp_targets;
+	int i;
+
+	for (i = 0; i < BOND_MAX_ARP_TARGETS && targets[i].tags; i++)
+		kfree(targets[i].tags);
+}
+
 /* A wrapper used because of ndo_del_link */
 int bond_release(struct net_device *bond_dev, struct net_device *slave_dev)
 {
@@ -3159,9 +3170,6 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 		target_ip = targets[i].target_ip;
 		tags = targets[i].tags;
 
-		slave_dbg(bond->dev, slave->dev, "%s: target %pI4\n",
-			  __func__, &target_ip);
-
 		/* Find out through which dev should the packet go */
 		rt = ip_route_output(dev_net(bond->dev), target_ip, 0, 0, 0,
 				     RT_SCOPE_LINK);
@@ -3182,9 +3190,13 @@ 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);
+			/* cache the tags */
+			targets[i].tags = tags;
+			rcu_read_unlock();
+		}
 
 		if (!IS_ERR_OR_NULL(tags))
 			goto found;
@@ -3200,7 +3212,6 @@ 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);
 	}
 }
 
@@ -6082,6 +6093,7 @@ static void bond_uninit(struct net_device *bond_dev)
 	/* Release the bonded slaves */
 	bond_for_each_slave(bond, slave, iter)
 		__bond_release_one(bond_dev, slave->dev, true, true);
+	free_tags(bond_dev);
 	netdev_info(bond_dev, "Released all slaves\n");
 
 #ifdef CONFIG_XFRM_OFFLOAD
@@ -6095,6 +6107,96 @@ static void bond_uninit(struct net_device *bond_dev)
 	bond_debug_unregister(bond);
 }
 
+/* Convert vlan_list into struct bond_vlan_tag.
+ * Inspired by bond_verify_device_path();
+ */
+static struct bond_vlan_tag *vlan_tags_parse(char *vlan_list, int level)
+{
+	struct bond_vlan_tag *tags;
+	char *vlan;
+
+	if (!vlan_list || strlen(vlan_list) == 0) {
+		tags = kcalloc(level + 1, sizeof(*tags), GFP_ATOMIC);
+		if (!tags)
+			return ERR_PTR(-ENOMEM);
+		tags[level].vlan_proto = BOND_VLAN_PROTO_NONE;
+		return tags;
+	}
+
+	for (vlan = strsep(&vlan_list, "/"); (vlan != 0); level++) {
+		tags = vlan_tags_parse(vlan_list, level + 1);
+		if (IS_ERR_OR_NULL(tags)) {
+			if (IS_ERR(tags))
+				return tags;
+			continue;
+		}
+
+		tags[level].vlan_proto = __cpu_to_be16(ETH_P_8021Q);
+		if (kstrtou16(vlan, 0, &tags[level].vlan_id))
+			return ERR_PTR(-EINVAL);
+
+		if (tags[level].vlan_id < 1 || tags[level].vlan_id > 4094) {
+			kfree(tags);
+			return ERR_PTR(-EINVAL);
+		}
+
+		return tags;
+	}
+
+	return NULL;
+}
+
+/**
+ * arp_ip_target_opt_parse - parse a single arp_ip_target option value string
+ * @src: the option value to be parsed
+ * @dest: struct arp_target to place the results.
+ *
+ * This function parses a single arp_ip_target string in the form:
+ * x.x.x.x[tag/....] into a struct arp_target.
+ * Returns 0 on success.
+ */
+static int arp_ip_target_opt_parse(char *src, struct arp_target *dest)
+{
+	char *ipv4, *vlan_list;
+	char target[128], *args;
+	struct bond_vlan_tag *tags = NULL;
+	__be32 ip;
+
+	/* Prevent buffer overflow */
+	if (strlen(src) > 128)
+		return -E2BIG;
+
+	pr_debug("Parsing arp_ip_target (%s)\n", src);
+
+	/* copy arp_ip_target[i] to local array, strsep works
+	 * destructively...
+	 */
+	args = strcpy(target, src);
+	ipv4 = strsep(&args, "[");
+
+	/* not a complete check, but good enough to catch mistakes */
+	if (!in4_pton(ipv4, -1, (u8 *)&ip, -1, NULL) ||
+	    !bond_is_ip_target_ok(ip)) {
+		return -EINVAL;
+	}
+
+	/* extract vlan tags */
+	vlan_list = strsep(&args, "]");
+
+	/* If a vlan list was not supplied skip the processing of the list.
+	 * A value of "[]" is a valid list and should be handled a such.
+	 */
+	if (vlan_list) {
+		tags = vlan_tags_parse(vlan_list, 0);
+		if (IS_ERR(tags))
+			return PTR_ERR(tags);
+	}
+
+	dest->target_ip = ip;
+	dest->tags = tags;
+	return 0;
+}
+
 /*------------------------- Module initialization ---------------------------*/
 
 static int __init bond_check_params(struct bond_params *params)
@@ -6289,21 +6391,25 @@ static int __init bond_check_params(struct bond_params *params)
 
 	for (arp_ip_count = 0, i = 0;
 	     (arp_ip_count < BOND_MAX_ARP_TARGETS) && arp_ip_target[i]; i++) {
-		__be32 ip;
+		struct arp_target tmp_arp_target;
 
-		/* not a complete check, but good enough to catch mistakes */
-		if (!in4_pton(arp_ip_target[i], -1, (u8 *)&ip, -1, NULL) ||
-		    !bond_is_ip_target_ok(ip)) {
+		if (arp_ip_target_opt_parse(arp_ip_target[i], &tmp_arp_target)) {
 			pr_warn("Warning: bad arp_ip_target module parameter (%s), ARP monitoring will not be performed\n",
 				arp_ip_target[i]);
 			arp_interval = 0;
-		} else {
-			if (bond_get_targets_ip(arp_target, ip) == -1)
-				arp_target[arp_ip_count++].target_ip = ip;
-			else
-				pr_warn("Warning: duplicate address %pI4 in arp_ip_target, skipping\n",
-					&ip);
+			break;
 		}
+
+		if (bond_get_targets_ip(arp_target, tmp_arp_target.target_ip) != -1) {
+			pr_warn("Warning: duplicate address %pI4 in arp_ip_target, skipping\n",
+				&tmp_arp_target.target_ip);
+			kfree(tmp_arp_target.tags);
+			continue;
+		}
+
+		arp_target[i].target_ip = tmp_arp_target.target_ip;
+		arp_target[i].tags = tmp_arp_target.tags;
+		++arp_ip_count;
 	}
 
 	if (arp_interval && !arp_ip_count) {
-- 
2.43.5

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

[Jakub Kicinski]

On Thu,  8 May 2025 11:29:29 -0700 David J Wilder wrote:
> 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 gathering tags.  When provided the list of tags circumvents
> the process of gathering tags by using the supplied list. An empty list
> can be provided to simply skip the process of gathering tags.

drivers/net/bonding/bond_main.c:6126:55: warning: Using plain integer as NULL pointer
drivers/net/bonding/bond_main.c:6159: warning: No description found for return value of 'arp_ip_target_opt_parse'
-- 
pw-bot: cr

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

[kernel test robot]

Hi David,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/David-J-Wilder/bonding-Adding-struct-arp_target/20250509-023255
base:   net-next/main
patch link:    https://lore.kernel.org/r/20250508183014.2554525-3-wilder%40us.ibm.com
patch subject: [PATCH net-next v1 2/2] bonding: Extend arp_ip_target format to allow for a list of vlan tags.
config: csky-randconfig-r123-20250510 (https://download.01.org/0day-ci/archive/20250511/202505110414.ebTXSare-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 14.2.0
reproduce: (https://download.01.org/0day-ci/archive/20250511/202505110414.ebTXSare-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505110414.ebTXSare-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/net/bonding/bond_main.c:6126:55: sparse: sparse: Using plain integer as NULL pointer

vim +6126 drivers/net/bonding/bond_main.c

  6109	
  6110	/* Convert vlan_list into struct bond_vlan_tag.
  6111	 * Inspired by bond_verify_device_path();
  6112	 */
  6113	static struct bond_vlan_tag *vlan_tags_parse(char *vlan_list, int level)
  6114	{
  6115		struct bond_vlan_tag *tags;
  6116		char *vlan;
  6117	
  6118		if (!vlan_list || strlen(vlan_list) == 0) {
  6119			tags = kcalloc(level + 1, sizeof(*tags), GFP_ATOMIC);
  6120			if (!tags)
  6121				return ERR_PTR(-ENOMEM);
  6122			tags[level].vlan_proto = BOND_VLAN_PROTO_NONE;
  6123			return tags;
  6124		}
  6125	
> 6126		for (vlan = strsep(&vlan_list, "/"); (vlan != 0); level++) {
  6127			tags = vlan_tags_parse(vlan_list, level + 1);
  6128			if (IS_ERR_OR_NULL(tags)) {
  6129				if (IS_ERR(tags))
  6130					return tags;
  6131				continue;
  6132			}
  6133	
  6134			tags[level].vlan_proto = __cpu_to_be16(ETH_P_8021Q);
  6135			if (kstrtou16(vlan, 0, &tags[level].vlan_id))
  6136				return ERR_PTR(-EINVAL);
  6137	
  6138			if (tags[level].vlan_id < 1 || tags[level].vlan_id > 4094) {
  6139				kfree(tags);
  6140				return ERR_PTR(-EINVAL);
  6141			}
  6142	
  6143			return tags;
  6144		}
  6145	
  6146		return NULL;
  6147	}
  6148	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Hangbin Liu]

Hi David,
On Thu, May 08, 2025 at 11:29:27AM -0700, David J Wilder wrote:
> This is a followup to this discussion:
>   https://www.spinics.net/lists/netdev/msg1085428.html
> 
> 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 gathering tags.
> When provided the list of tags circumvents the process of gathering tags by
> using the supplied list. An empty list can be provided to simply skip the the
> process of gathering tags.
> 
> The purposed new format for arp_ip_target is:
> arp_ip_target=ipv4-address[vlan-tag\...],...
> 
> Examples:
> arp_ip_target=10.0.0.1,10.0.0.2 (Determine tags automatically for both targets)
> arp_ip_target=10.0.0.1[]        (Don't add any tags, don't gather tags)
> arp_ip_target=10.0.0.1[100/200] (Don't gather tags, use supplied list of tags)
> arp_ip_target=10.0.0.1,10.0.0.2[100] (add vlan 100 tag for 10.0.0.2.
>                                       Gather tags for 10.0.0.1.)

Thanks for the update. What about the IPv6 NS targets? Will you support it
or only the arp targets?

BTW, when add or update the parameter, please also update the bond documents
and iproute2 cmd.

Thanks
Hangbin
> 
> This is a work in process. I am requesting feedback on my approach.
> This patch allows the extended format only when setting arp_ip_target values with
> modules parameters.
> 
> TBD: Add support for sysfs, netlink and procfs.
> TBD: Kernel self tests.
> TBD: Documentation.
> 
> David J Wilder (2):
>   bonding: Adding struct arp_target
>   bonding: Extend arp_ip_target format to allow for a list of vlan tags.
> 
>  drivers/net/bonding/bond_main.c    | 163 ++++++++++++++++++++++++-----
>  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/bonding.h              |  15 ++-
>  6 files changed, 161 insertions(+), 47 deletions(-)
> 
> -- 
> 2.43.5
> 

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

[David Wilder]

>Hi David,
>> 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 gathering tags.
>> When provided the list of tags circumvents the process of gathering tags by
>> using the supplied list. An empty list can be provided to simply skip the the
>> process of gathering tags.
>>
>> The purposed new format for arp_ip_target is:
>> arp_ip_target=ipv4-address[vlan-tag\...],...
>>
>> Examples:
>> arp_ip_target=10.0.0.1,10.0.0.2 (Determine tags automatically for both targets)
>> arp_ip_target=10.0.0.1[]        (Don't add any tags, don't gather tags)
>> arp_ip_target=10.0.0.1[100/200] (Don't gather tags, use supplied list of tags)
>> arp_ip_target=10.0.0.1,10.0.0.2[100] (add vlan 100 tag for 10.0.0.2.
>>                                       Gather tags for 10.0.0.1.)
>
>Thanks for the update. What about the IPv6 NS targets? Will you support it
>or only the arp targets?

Thank you for the input.

My initial plan is to support IPv4, but I will ensure that it won't adversely impact IPv6.

>
>BTW, when add or update the parameter, please also update the bond documents
>and iproute2 cmd.

Will do.

>
>Thanks
>Hangbin

David Wilder (wilder@us.ibm.com)