Re: [PATCH net] bonding: use permanent address for MAC swapping if device address is same

[Hangbin Liu]

On Tue, Mar 25, 2025 at 06:24:16AM -0700, Jakub Kicinski wrote:
> > 1) ip link set eth1 master bond0
> 
> nit: 2)
> 
> >    eth1 is added as a backup with its own MAC (MAC1).
> > 
> > 3) ip link set eth0 nomaster
> >    eth0 is released and restores its MAC (MAC0).
> >    eth1 becomes the active slave, and bond0 assigns MAC0 to eth1.
> 
> I don't know much about bonding, but this seems like a problem already
> to me. Assuming both eth0 and eth1 are on the same segment we now have
> two interfaces with the same MAC on the network. Shouldn't we override
> the address of eth0 to a random one when it leaves?

Can we change an interface mac to random value after leaving bond's control?
It looks may break user's other configures.

> 
> > 4) ip link set eth0 master bond0
> >    eth0 is re-added to bond0, but both eth0 and eth1 now have MAC0,
> >    breaking the follow policy.
> > 
> > To resolve this issue, we need to swap the new active slave’s permanent
> > MAC address with the old one. The new active slave then uses the old
> > dev_addr, ensuring that it matches the bond address. After the fix:
> > 
> > 5) ip link set bond0 type bond active_slave eth0
> >    dev_addr is the same, swap old active eth1's MAC (MAC0) with eth0.
> >    Swap new active eth0's permanent MAC (MAC0) to eth1.
> >    MAC addresses remain unchanged.
> > 
> > 6) ip link set bond0 type bond active_slave eth1
> >    dev_addr is the same, swap the old active eth0's MAC (MAC0) with eth1.
> >    Swap new active eth1's permanent MAC (MAC1) to eth0.
> >    The MAC addresses are now correctly differentiated.
> > 
> > Fixes: 3915c1e8634a ("bonding: Add "follow" option to fail_over_mac")
> > Reported-by: Liang Li <liali@xxxxxxxxxx>
> > Signed-off-by: Hangbin Liu <liuhangbin@xxxxxxxxx>
> > ---
> >  drivers/net/bonding/bond_main.c | 9 +++++++--
> >  include/net/bonding.h           | 8 ++++++++
> >  2 files changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > index e45bba240cbc..9cc2348d4ee9 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -1107,8 +1107,13 @@ static void bond_do_fail_over_mac(struct bonding *bond,
> >  			old_active = bond_get_old_active(bond, new_active);
> >  
> >  		if (old_active) {
> > -			bond_hw_addr_copy(tmp_mac, new_active->dev->dev_addr,
> > -					  new_active->dev->addr_len);
> > +			if (bond_hw_addr_equal(old_active->dev->dev_addr, new_active->dev->dev_addr,
> > +					       new_active->dev->addr_len))
> > +				bond_hw_addr_copy(tmp_mac, new_active->perm_hwaddr,
> > +						  new_active->dev->addr_len);
> > +			else
> > +				bond_hw_addr_copy(tmp_mac, new_active->dev->dev_addr,
> > +						  new_active->dev->addr_len);
> >  			bond_hw_addr_copy(ss.__data,
> >  					  old_active->dev->dev_addr,
> >  					  old_active->dev->addr_len);
> > diff --git a/include/net/bonding.h b/include/net/bonding.h
> > index 8bb5f016969f..de965c24dde0 100644
> > --- a/include/net/bonding.h
> > +++ b/include/net/bonding.h
> > @@ -463,6 +463,14 @@ static inline void bond_hw_addr_copy(u8 *dst, const u8 *src, unsigned int len)
> >  	memcpy(dst, src, len);
> >  }
> >  
> > +static inline bool bond_hw_addr_equal(const u8 *dst, const u8 *src, unsigned int len)
> > +{
> > +	if (len == ETH_ALEN)
> > +		return ether_addr_equal(dst, src);
> > +	else
> > +		return (memcmp(dst, src, len) == 0);
> 
> looks like this is on ctrl path, just always use memcmp directly ?
> not sure if this helper actually.. helps.

This is just to align with bond_hw_addr_copy(). If you think it's not help.
I can use memcmp() directly.

Thanks
Hangbin