Re: [PATCH net] bonding: add ns target multicast address to slave device

[Jay Vosburgh <jv@jvosburgh.net>]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>Commit 4598380f9c54 ("bonding: fix ns validation on backup slaves")
>tried to resolve the issue where backup slaves couldn't be brought up when
>receiving IPv6 Neighbor Solicitation (NS) messages. However, this fix only
>worked for drivers that receive all multicast messages, such as the veth
>interface.
>
>For standard drivers, the NS multicast message is silently dropped because
>the slave device is not a member of the NS target multicast group.
>
>To address this, we need to make the slave device join the NS target
>multicast group, ensuring it can receive these IPv6 NS messages to validate
>the slave’s status properly.
>
>Fixes: 4e24be018eb9 ("bonding: add new parameter ns_targets")
>Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

	This seems fairly involved; would it be simpler to have
bond_hw_addr_swap() and/or bond_change_active_slave() insure that the
MAC multicast list is configured in the backup interface if arp_validate
is set appropriately and there's a NS target configured?  That will make
the MAC multicast list more inclusive than necessary, but I think the
implementation will be much less involved.

	-J

>---
>Another way is to set IFF_ALLMULTI flag for slaves. But I think that
>would affect too much.
>---
> drivers/net/bonding/bond_main.c    | 11 ++++++++
> drivers/net/bonding/bond_options.c | 44 +++++++++++++++++++++++++++++-
> include/net/bond_options.h         |  2 ++
> 3 files changed, 56 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index b1bffd8e9a95..04ccbd41fb0c 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -2350,6 +2350,11 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
> 	if (bond_mode_can_use_xmit_hash(bond))
> 		bond_update_slave_arr(bond, NULL);
> 
>+#if IS_ENABLED(CONFIG_IPV6)
>+	if (slave_dev->flags & IFF_MULTICAST)
>+		/* set target NS maddrs for new slave */
>+		slave_set_ns_maddr(bond, slave_dev, true);
>+#endif
> 
> 	if (!slave_dev->netdev_ops->ndo_bpf ||
> 	    !slave_dev->netdev_ops->ndo_xdp_xmit) {
>@@ -2503,6 +2508,12 @@ static int __bond_release_one(struct net_device *bond_dev,
> 	/* recompute stats just before removing the slave */
> 	bond_get_stats(bond->dev, &bond->bond_stats);
> 
>+#if IS_ENABLED(CONFIG_IPV6)
>+	if (slave_dev->flags & IFF_MULTICAST)
>+		/* clear all target NS maddrs */
>+		slave_set_ns_maddr(bond, slave_dev, false);
>+#endif
>+
> 	if (bond->xdp_prog) {
> 		struct netdev_bpf xdp = {
> 			.command = XDP_SETUP_PROG,
>diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>index 95d59a18c022..823cb93d2853 100644
>--- a/drivers/net/bonding/bond_options.c
>+++ b/drivers/net/bonding/bond_options.c
>@@ -1234,17 +1234,41 @@ static int bond_option_arp_ip_targets_set(struct bonding *bond,
> }
> 
> #if IS_ENABLED(CONFIG_IPV6)
>+/* convert IPv6 address to link-local solicited-node multicast mac address */
>+static void ipv6_addr_to_solicited_mac(const struct in6_addr *addr,
>+				       unsigned char mac[ETH_ALEN])
>+{
>+	mac[0] = 0x33;
>+	mac[1] = 0x33;
>+	mac[2] = 0xFF;
>+	mac[3] = addr->s6_addr[13];
>+	mac[4] = addr->s6_addr[14];
>+	mac[5] = addr->s6_addr[15];
>+}
>+
> static void _bond_options_ns_ip6_target_set(struct bonding *bond, int slot,
> 					    struct in6_addr *target,
> 					    unsigned long last_rx)
> {
>+	unsigned char target_maddr[ETH_ALEN], slot_maddr[ETH_ALEN];
> 	struct in6_addr *targets = bond->params.ns_targets;
> 	struct list_head *iter;
> 	struct slave *slave;
> 
>+	if (!ipv6_addr_any(target))
>+		ipv6_addr_to_solicited_mac(target, target_maddr);
> 	if (slot >= 0 && slot < BOND_MAX_NS_TARGETS) {
>-		bond_for_each_slave(bond, slave, iter)
>+		if (!ipv6_addr_any(&targets[slot]))
>+			ipv6_addr_to_solicited_mac(&targets[slot], slot_maddr);
>+		bond_for_each_slave(bond, slave, iter) {
> 			slave->target_last_arp_rx[slot] = last_rx;
>+			/* remove the previous maddr on salve */
>+			if (!ipv6_addr_any(&targets[slot]))
>+				dev_mc_del(slave->dev, slot_maddr);
>+			/* add new maddr on slave if target is set */
>+			if (!ipv6_addr_any(target))
>+				dev_mc_add(slave->dev, target_maddr);
>+		}
> 		targets[slot] = *target;
> 	}
> }
>@@ -1290,6 +1314,24 @@ static int bond_option_ns_ip6_targets_set(struct bonding *bond,
> 
> 	return 0;
> }
>+
>+void slave_set_ns_maddr(struct bonding *bond, struct net_device *slave_dev,
>+			bool add)
>+{
>+	struct in6_addr *targets = bond->params.ns_targets;
>+	unsigned char slot_maddr[ETH_ALEN];
>+	int i;
>+
>+	for (i = 0; i < BOND_MAX_NS_TARGETS; i++) {
>+		if (!ipv6_addr_any(&targets[i])) {
>+			ipv6_addr_to_solicited_mac(&targets[i], slot_maddr);
>+			if (add)
>+				dev_mc_add(slave_dev, slot_maddr);
>+			else
>+				dev_mc_del(slave_dev, slot_maddr);
>+		}
>+	}
>+}
> #else
> static int bond_option_ns_ip6_targets_set(struct bonding *bond,
> 					  const struct bond_opt_value *newval)
>diff --git a/include/net/bond_options.h b/include/net/bond_options.h
>index 473a0147769e..c6c5c1333f37 100644
>--- a/include/net/bond_options.h
>+++ b/include/net/bond_options.h
>@@ -160,6 +160,8 @@ static inline void __bond_opt_init(struct bond_opt_value *optval,
> void bond_option_arp_ip_targets_clear(struct bonding *bond);
> #if IS_ENABLED(CONFIG_IPV6)
> void bond_option_ns_ip6_targets_clear(struct bonding *bond);
>+void slave_set_ns_maddr(struct bonding *bond, struct net_device *slave_dev,
>+			bool add);
> #endif
> 
> #endif /* _NET_BOND_OPTIONS_H */
>-- 
>2.46.0
>

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