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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z-PVgs4OIDZx5fZD@fedora>
Date: Wed, 26 Mar 2025 10:22:58 +0000
From: Hangbin Liu <liuhangbin@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, Jay Vosburgh <jv@...sburgh.net>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Nikolay Aleksandrov <razor@...ckwall.org>,
	Simon Horman <horms@...nel.org>, Cosmin Ratiu <cratiu@...dia.com>,
	linux-kernel@...r.kernel.org, Liang Li <liali@...hat.com>
Subject: Re: [PATCH net] bonding: use permanent address for MAC swapping if
 device address is same

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@...hat.com>
> > Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
> > ---
> >  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