Message-ID: <ZytEBmPmqHwfCIzo@penguin>
Date: Wed, 6 Nov 2024 12:25:10 +0200
From: Nikolay Aleksandrov <razor@...ckwall.org>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: netdev@...r.kernel.org, Jay Vosburgh <jv@...sburgh.net>,
	Andy Gospodarek <andy@...yhouse.net>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Simon Horman <horms@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCHv3 net 1/2] bonding: add ns target multicast address to
 slave device

On Wed, Nov 06, 2024 at 05:14:41AM +0000, Hangbin Liu 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.
> 
> There are three policies before joining the multicast group:
> 1. All settings must be under active-backup mode (alb and tlb do not support
>    arp_validate), with backup slaves and slaves supporting multicast.
> 2. We can add or remove multicast groups when arp_validate changes.
> 3. Other operations, such as enslaving, releasing, or setting NS targets,
>    need to be guarded by arp_validate.
> 
> Fixes: 4e24be018eb9 ("bonding: add new parameter ns_targets")
> Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
> ---
>  drivers/net/bonding/bond_main.c    | 18 ++++++-
>  drivers/net/bonding/bond_options.c | 85 +++++++++++++++++++++++++++++-
>  include/net/bond_options.h         |  1 +
>  3 files changed, 102 insertions(+), 2 deletions(-)
> 

Hi,
A few minor comments below,

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

So instead of having these redundant comments, maybe add helper functions
to show better what's happening, e.g. slave_ns_maddrs_add()...

> +		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);

... same here slave_ns_maddrs_del()

These true/false arguments for add/del require the reader to go search what
they mean.

>  	}
>  }
>  
> @@ -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
> +	 */

"needs to be called...which will remove the maddr*s*"

> +	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
> +	 */

This should be re-worded. I think there's a word missing as well in
"...must after...".

> +	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..60368cef2704 100644
> --- a/drivers/net/bonding/bond_options.c
> +++ b/drivers/net/bonding/bond_options.c
> @@ -15,6 +15,7 @@
>  #include <linux/sched/signal.h>
>  
>  #include <net/bonding.h>
> +#include <net/ndisc.h>
>  
>  static int bond_option_active_slave_set(struct bonding *bond,
>  					const struct bond_opt_value *newval);
> @@ -1234,6 +1235,64 @@ static int bond_option_arp_ip_targets_set(struct bonding *bond,
>  }
>  
>  #if IS_ENABLED(CONFIG_IPV6)
> +static bool slave_can_set_ns_maddr(struct bonding *bond, struct slave *slave)

const bond/slave

> +{
> +	return BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP &&
> +	       !bond_is_active_slave(slave) &&
> +	       slave->dev->flags & IFF_MULTICAST;
> +}
> +
> +static void _slave_set_ns_maddrs(struct bonding *bond, struct slave *slave, bool add)

Can we please stick to using double underscore for internal helpers?
I see there are a few other places where a single underscore is used in this
file which I've missed before, we can take care of those to make them consistent
with the rest of the bonding code.

> +{
> +	struct in6_addr *targets = bond->params.ns_targets;
> +	char slot_maddr[MAX_ADDR_LEN];
> +	int i;
> +
> +	if (!slave_can_set_ns_maddr(bond, slave))
> +		return;
> +
> +	for (i = 0; i < BOND_MAX_NS_TARGETS; i++) {
> +		if (ipv6_addr_any(&targets[i]))
> +			break;
> +
> +		if (!ndisc_mc_map(&targets[i], slot_maddr, slave->dev, 0)) {
> +			if (add)
> +				dev_mc_add(slave->dev, slot_maddr);
> +			else
> +				dev_mc_del(slave->dev, slot_maddr);
> +		}
> +	}
> +}
> +
> +void slave_set_ns_maddrs(struct bonding *bond, struct slave *slave, bool add)

Usually exported slave functions have the bond_ prefix, e.g. bond_slave...

> +{
> +	if (!bond->params.arp_validate)
> +		return;
> +
> +	_slave_set_ns_maddrs(bond, slave, add);
> +}
> +
> +static void slave_set_ns_maddr(struct bonding *bond, struct slave *slave,
> +			       struct in6_addr *target, struct in6_addr *slot)

> +{
> +	char target_maddr[MAX_ADDR_LEN], slot_maddr[MAX_ADDR_LEN];
> +
> +	if (!bond->params.arp_validate || !slave_can_set_ns_maddr(bond, slave))
> +		return;
> +
> +	/* remove the previous maddr on salve */

s/salve/slave/
s/on/from/

> +	if (!ipv6_addr_any(slot) &&
> +	    !ndisc_mc_map(slot, slot_maddr, slave->dev, 0)) {
> +		dev_mc_del(slave->dev, slot_maddr);
> +	}

drop unnecessary {}

> +
> +	/* add new maddr on slave if target is set */
> +	if (!ipv6_addr_any(target) &&
> +	    !ndisc_mc_map(target, target_maddr, slave->dev, 0)) {
> +		dev_mc_add(slave->dev, target_maddr);
> +	}

drop unnesecary {}

> +}
> +
>  static void _bond_options_ns_ip6_target_set(struct bonding *bond, int slot,
>  					    struct in6_addr *target,
>  					    unsigned long last_rx)
> @@ -1243,8 +1302,10 @@ static void _bond_options_ns_ip6_target_set(struct bonding *bond, int slot,
>  	struct slave *slave;
>  
>  	if (slot >= 0 && slot < BOND_MAX_NS_TARGETS) {
> -		bond_for_each_slave(bond, slave, iter)
> +		bond_for_each_slave(bond, slave, iter) {
>  			slave->target_last_arp_rx[slot] = last_rx;
> +			slave_set_ns_maddr(bond, slave, target, &targets[slot]);
> +		}
>  		targets[slot] = *target;
>  	}
>  }
> @@ -1296,15 +1357,37 @@ static int bond_option_ns_ip6_targets_set(struct bonding *bond,
>  {
>  	return -EPERM;
>  }
> +
> +static void _slave_set_ns_maddrs(struct bonding *bond, struct slave *slave, bool add)
> +{
> +}
> +
> +void slave_set_ns_maddrs(struct bonding *bond, struct slave *slave, bool add)
> +{
> +}
>  #endif
>  
>  static int bond_option_arp_validate_set(struct bonding *bond,
>  					const struct bond_opt_value *newval)
>  {
> +	bool changed = (bond->params.arp_validate == 0 && newval->value != 0) ||
> +		       (bond->params.arp_validate != 0 && newval->value == 0);

!!bond->params.arp_validate != !!newval->value

> +	struct list_head *iter;
> +	struct slave *slave;
> +
>  	netdev_dbg(bond->dev, "Setting arp_validate to %s (%llu)\n",
>  		   newval->string, newval->value);
>  	bond->params.arp_validate = newval->value;
>  
> +	if (changed) {
> +		bond_for_each_slave(bond, slave, iter) {
> +			if (bond->params.arp_validate)
> +				_slave_set_ns_maddrs(bond, slave, true);
> +			else
> +				_slave_set_ns_maddrs(bond, slave, false);
> +		}
> +	}

This block can be written shortly as:
 bond_for_each_slave(bond, slave, iter)
	_slave_set_ns_maddrs(bond, slave, !!bond->params.arp_validate);

> +
>  	return 0;
>  }
>  
> diff --git a/include/net/bond_options.h b/include/net/bond_options.h
> index 473a0147769e..59a91d12cd57 100644
> --- a/include/net/bond_options.h
> +++ b/include/net/bond_options.h
> @@ -161,5 +161,6 @@ 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);
>  #endif
> +void slave_set_ns_maddrs(struct bonding *bond, struct slave *slave, bool add);
>  
>  #endif /* _NET_BOND_OPTIONS_H */
> -- 
> 2.46.0
> 

Cheers,
 Nik

Powered by blists - more mailing lists