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: <2696885.1742497994@famine>
Date: Thu, 20 Mar 2025 12:13:14 -0700
From: Jay Vosburgh <jv@...sburgh.net>
To: Hangbin Liu <liuhangbin@...il.com>
cc: netdev@...r.kernel.org, Andrew Lunn <andrew+netdev@...n.ch>,
    "David S. Miller" <davem@...emloft.net>,
    Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
    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

Hangbin Liu <liuhangbin@...il.com> wrote:

>Similar with a951bc1e6ba5 ("bonding: correct the MAC address for "follow"
>fail_over_mac policy"). The fail_over_mac follow mode requires the formerly
>active slave to swap MAC addresses with the newly active slave during
>failover. However, the slave's MAC address can be same under certain
>conditions:
>
>1) ip link set eth0 master bond0
>   bond0 adopts eth0's MAC address (MAC0).
>
>1) ip link set eth1 master bond0
>   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.
>
>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.

	Are all of these steps necessary, or does the issue happen if a
new interface (not previously part of the bond) is added to the bond
with its MAC set to whatever the bond's MAC is?

	Did this come up in practise somewhere, or through inspection /
testing?  I'm curious as I'd expect usage of this option today would be
rare, as I hope that current hardware wouldn't have the "MAC assigned to
multiple ports" issues that led to the "follow" logic.  If memory
serves, the issue arose originally in the ehea network device (on IBM
POWER), which I believe is out of production now for some years.

>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.

	An alternative solution could be to disallow adding a new
interface in "follow" mode if its MAC matches the active interface of
the bond.  If this patch is more of an correctness exercise rather than
something found out in the world impacting production deployments, it
might be better to keep the MAC swapping logic in the failover code
simpler.

	-J

>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);
>+}
>+
> #define BOND_PRI_RESELECT_ALWAYS	0
> #define BOND_PRI_RESELECT_BETTER	1
> #define BOND_PRI_RESELECT_FAILURE	2
>-- 
>2.46.0

---
	-Jay Vosburgh, jv@...sburgh.net