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

Hangbin Liu
2024-10-31T03:06:42Z

Hi Jay,
On Wed, Oct 30, 2024 at 05:21:05PM +0100, Jay Vosburgh wrote:
> I suspect the set of multicast addresses involved is likely to
> be small in the usual case, so the question then is whether the
> presumably small amount of traffic that inadvertently passes the filter
> (and is then thrown away by the kernel RX logic) is worth the complexity
> added here.

Yes, while the code and logic may be complex, the "small amount of
traffic", specifically, the IPv6 NS messages, plays a crucial role in
determining whether backup slaves are up or not. Without these messages,
it would be akin to dropping ARP traffic for IPv4, which could lead to
connectivity issues.

>
> That said, I have a few questions below.
>
> > arp_validate doesn't support 3ad, tlb, alb. So let's only do it on ab mode.
> >---
> > drivers/net/bonding/bond_main.c | 18 +++++-
> > drivers/net/bonding/bond_options.c | 95 +++++++++++++++++++++++++++++-
> > include/net/bond_options.h | 1 +
> > 3 files changed, 112 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> >index b1bffd8e9a95..d7c1016619f9 100644
> >--- a/drivers/net/bonding/bond_main.c
> >+++ b/drivers/net/bonding/bond_main.c
> >@@ -1008,6 +1008,9 @@ static void bond_hw_addr_swap(struct bonding *bond, struct slave *new_active,
> >
> > if (bond->dev->flags & IFF_UP)
> > bond_hw_addr_flush(bond->dev, old_active->dev);
> >+
> >+ /* add target NS maddrs for backup slave */
> >+ slave_set_ns_maddrs(bond, old_active, true);
> > }
> >
> > if (new_active) {
> >@@ -1024,6 +1027,9 @@ static void bond_hw_addr_swap(struct bonding *bond, struct slave *new_active,
> > dev_mc_sync(new_active->dev, bond->dev);
> > netif_addr_unlock_bh(bond->dev);
> > }
> >+
> >+ /* clear target NS maddrs for active slave */
> >+ slave_set_ns_maddrs(bond, new_active, false);
> > }
> > }
> >
> >@@ -2341,6 +2347,12 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
> > bond_compute_features(bond);
> > bond_set_carrier(bond);
> >
> >+ /* set target NS maddrs for new slave, need to be called before
> >+ * bond_select_active_slave(), which will remove the maddr if
> >+ * the slave is selected as active slave
> >+ */
> >+ slave_set_ns_maddrs(bond, new_slave, true);
> >+
> > if (bond_uses_primary(bond)) {
> > block_netpoll_tx();
> > bond_select_active_slave(bond);
> >@@ -2350,7 +2362,6 @@ 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 (!slave_dev->netdev_ops->ndo_bpf ||
> > !slave_dev->netdev_ops->ndo_xdp_xmit) {
> > if (bond->xdp_prog) {
> >@@ -2548,6 +2559,11 @@ static int __bond_release_one(struct net_device *bond_dev,
> > if (oldcurrent == slave)
> > bond_change_active_slave(bond, NULL);
> >
> >+ /* clear target NS maddrs, must after bond_change_active_slave()
> >+ * as we need to clear the maddrs on backup slave
> >+ */
> >+ slave_set_ns_maddrs(bond, slave, false);
> >+
> > if (bond_is_lb(bond)) {
> > /* Must be called only after the slave has been
> > * detached from the list and the curr_active_slave
> >diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
> >index 95d59a18c022..2554ba70f092 100644
> >--- a/drivers/net/bonding/bond_options.c
> >+++ b/drivers/net/bonding/bond_options.c
> >@@ -1234,6 +1234,75 @@ 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];
> >+}
>
> Can we make use of ndisc_mc_map() / ipv6_eth_mc_map() to perform
> this step, instead of creating a new function that's almost the same?

Ah, yes, I think so. Thanks for this tips.

> >+void slave_set_ns_maddrs(struct bonding *bond, struct slave *slave, bool add)
> >+{
> >+ if (!bond->params.arp_validate)
> >+ return;
> >+
> >+ _slave_set_ns_maddrs(bond, slave, add);
> >+}
>
> Why does this need a wrapper function vs. having the
> arp_validate test be first in the larger function?

We have 4 places call slave_set_ns_maddrs(). I think with this wrapper could
save some codes. I'm fine to remove this wrapper if you think the code
would be simpler.

Thanks
Hangbin