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>
---
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

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

On Mon, Oct 21, 2024 at 06:05:11PM +0200, Jay Vosburgh wrote:
> 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.

You are right. Limit the mcast list only on backup salve would be less
involved.

So I will do:

1. Add mcast list to all backup salves when setting NS targets.
2. Add mcast to new backup slave, remove the list on new active slave on
   bond_hw_addr_swap()
3. Remove all mcast list when release slave
4. All the changed need to be with arp_validate and NS targets configured.

Thanks
Hangbin