| 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
| ||
|
Message-ID: <aKvhk8Cq3ZdWeH_7@fedora>
Date: Mon, 25 Aug 2025 04:07:47 +0000
From: Hangbin Liu <liuhangbin@...il.com>
To: Jay Vosburgh <jv@...sburgh.net>
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>, Shuah Khan <shuah@...nel.org>,
linux-kselftest@...r.kernel.org, Qiuling Ren <qren@...hat.com>
Subject: Re: [PATCH net 1/2] bonding: set random address only when slaves
already exist
On Fri, Aug 22, 2025 at 05:21:30PM -0700, Jay Vosburgh wrote:
> Hangbin Liu <liuhangbin@...il.com> wrote:
>
> >Commit 5c3bf6cba791 ("bonding: assign random address if device address is
> >same as bond") fixed an issue where, after releasing the first slave and
> >re-adding it to the bond with fail_over_mac=follow, both the active and
> >backup slaves could end up with duplicate MAC addresses. To avoid this,
> >the new slave was assigned a random address.
> >
> >However, if this happens when adding the very first slave, the bond’s
> >hardware address is set to match the slave’s. Later, during the
> >fail_over_mac=follow check, the slave’s MAC is randomized because it
> >naturally matches the bond, which is incorrect.
>
> The description here seems confusing to me; what does "this"
> refer to? I don't understand the sequence of events that lead to the
> issue being fixed here.
>
> I wonder if there's another bug somewhere, since nominally when
> releasing the last interface in the bond, __bond_release_one() should
> randomize the bond's MAC address, so it shouldn't match when adding (or
> re-adding ?) the first interface to the bond.
>
Sorry I didn't make it clear. A easy reproducer would describe the issue. e.g.
(omit the lo interface)
[root@...tme-ng net]# ip link add type veth
[root@...tme-ng net]# ip link add bond0 type bond mode 1 miimon 100 fail_over_mac 2
[root@...tme-ng net]# ip link show
3: veth0@...h1: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
link/ether 02:0a:04:c2:d6:21 brd ff:ff:ff:ff:ff:ff
4: veth1@...h0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
link/ether 82:a8:52:f4:81:4e brd ff:ff:ff:ff:ff:ff
5: bond0: <BROADCAST,MULTICAST,MASTER> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
link/ether 92:5d:9c:47:e7:53 brd ff:ff:ff:ff:ff:ff
[root@...tme-ng net]# ip link set veth0 master bond0
[root@...tme-ng net]# ip link show
3: veth0@...h1: <NO-CARRIER,BROADCAST,MULTICAST,SLAVE,UP,M-DOWN> mtu 1500 qdisc noqueue master bond0 state LOWERLAYERDOWN mode DEFAULT group default qlen 1000
link/ether 4e:b5:4a:b4:03:18 brd ff:ff:ff:ff:ff:ff
4: veth1@...h0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
link/ether 82:a8:52:f4:81:4e brd ff:ff:ff:ff:ff:ff
5: bond0: <BROADCAST,MULTICAST,MASTER> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
link/ether 02:0a:04:c2:d6:21 brd ff:ff:ff:ff:ff:ff
Here we can see the veth0's mac address is randomized. The reason is in
function bond_enslave(), we set the bond mac address to the same as slave's
if it's the first one.
/* If this is the first slave, then we need to set the master's hardware
* address to be the same as the slave's.
*/
if (!bond_has_slaves(bond) &&
bond->dev->addr_assign_type == NET_ADDR_RANDOM) {
res = bond_set_dev_addr(bond->dev, slave_dev);
if (res)
goto err_undo_flags;
}
And later
} else if (bond->params.fail_over_mac == BOND_FOM_FOLLOW &&
BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP &&
memcmp(slave_dev->dev_addr, bond_dev->dev_addr, bond_dev->addr_len) == 0) {
/* Set slave to random address to avoid duplicate mac
* address in later fail over.
*/
eth_random_addr(ss.__data);
} else {
Here we check the bond and slave's mac address, which would be the same
definitely, which cause the first slave's mac got changed.
Thanks
Hangbin
>
> >The issue is normally hidden since the first slave usually becomes the
> >active one, which restores the bond's MAC address. However, if another
> >slave is selected as the initial active interface, the issue becomes visible.
> >
> >Fix this by assigning a random address only when slaves already exist in
> >the bond.
> >
> >Fixes: 5c3bf6cba791 ("bonding: assign random address if device address is same as bond")
> >Reported-by: Qiuling Ren <qren@...hat.com>
> >Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
> >---
> > drivers/net/bonding/bond_main.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> >index 257333c88710..8832bc9f107b 100644
> >--- a/drivers/net/bonding/bond_main.c
> >+++ b/drivers/net/bonding/bond_main.c
> >@@ -2132,6 +2132,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
> > memcpy(ss.__data, bond_dev->dev_addr, bond_dev->addr_len);
> > } else if (bond->params.fail_over_mac == BOND_FOM_FOLLOW &&
> > BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP &&
> >+ bond_has_slaves(bond) &&
> > memcmp(slave_dev->dev_addr, bond_dev->dev_addr, bond_dev->addr_len) == 0) {
> > /* Set slave to random address to avoid duplicate mac
> > * address in later fail over.
> >--
> >2.50.1
> >
>
> ---
> -Jay Vosburgh, jv@...sburgh.net
>
Powered by blists - more mailing lists