Re: [PATCH net-next] bonding: report duplicate MAC address in all situations

[Hangbin Liu <liuhangbin@gmail.com>]

Hi Jakub,
On Mon, Feb 24, 2025 at 02:10:08PM -0800, Jakub Kicinski wrote:
> On Wed, 19 Feb 2025 07:55:15 +0000 Hangbin Liu wrote:
> > Normally, a bond uses the MAC address of the first added slave as the
> > bond’s MAC address. And the bond will set active slave’s MAC address to
> > bond’s address if fail_over_mac is set to none (0) or follow (2).
> > 
> > When the first slave is removed, the bond will still use the removed
> > slave’s MAC address, which can lead to a duplicate MAC address and
> > potentially cause issues with the switch. To avoid confusion, let's warn
> > the user in all situations, including when fail_over_mac is set to 2 or
> > in active-backup mode.
> 
> Makes sense, thanks for the high quality commit message.
> 
> False positive warnings are annoying to users (especially users who
> monitor all warnings in their fleet). Could we stick to filtering out
> the BOND_FOM_ACTIVE case? Looks like this condition:
> 
> 	if (bond->params.fail_over_mac != BOND_FOM_ACTIVE ||
> 	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
> 
> exists a few lines later in __bond_release_one()


You are right. The bond will change the mac address later if
bond mode is active_backup and fail_over_mac is active

        if (oldcurrent == slave)
                bond_change_active_slave(bond, NULL);

With the upper mode and parameter, during the slave_warn(), bond is still
using the same mac addr with the first join and released slave's mac address
and cause false positive warn.

I will update the if condition. Thanks for your review.

Regards
Hangbin
> 
> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > index e45bba240cbc..ca66107776cc 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -2551,13 +2551,11 @@ static int __bond_release_one(struct net_device *bond_dev,
> >  
> >  	RCU_INIT_POINTER(bond->current_arp_slave, NULL);
> >  
> > -	if (!all && (!bond->params.fail_over_mac ||
> > -		     BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP)) {
> > -		if (ether_addr_equal_64bits(bond_dev->dev_addr, slave->perm_hwaddr) &&
> > -		    bond_has_slaves(bond))
> > -			slave_warn(bond_dev, slave_dev, "the permanent HWaddr of slave - %pM - is still in use by bond - set the HWaddr of slave to a different address to avoid conflicts\n",
> > -				   slave->perm_hwaddr);
> > -	}
> > +	if (!all &&
> > +	    ether_addr_equal_64bits(bond_dev->dev_addr, slave->perm_hwaddr) &&
> > +	    bond_has_slaves(bond))
> > +		slave_warn(bond_dev, slave_dev, "the permanent HWaddr of slave - %pM - is still in use by bond - set the HWaddr of slave to a different address to avoid conflicts\n",
> > +			   slave->perm_hwaddr);
> -- 
> pw-bot: cr