[v1] bonding: report duplicate MAC address in all situations

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

Posted by Hangbin Liu 10 months ago

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.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/bonding/bond_main.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

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);
 
 	if (rtnl_dereference(bond->primary_slave) == slave)
 		RCU_INIT_POINTER(bond->primary_slave, NULL);
-- 
2.39.5 (Apple Git-154)

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

Posted by Jakub Kicinski 9 months, 3 weeks ago

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

> 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

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

Posted by Hangbin Liu 9 months, 3 weeks ago

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