netdev - Re: [PATCH net 1/2] bonding: set random address only when slaves already exist

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] [day] [month] [year] [list]

Message-ID: <aMDf0Lj0PzJfY46x@fedora>
Date: Wed, 10 Sep 2025 02:17:52 +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 Tue, Sep 09, 2025 at 01:25:43PM -0700, Jay Vosburgh wrote:
> >> 
> >> 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.
> >
> >Any comments for this?
> 
> 	Sorry, fell off the radar.
> 
> 	I follow what's going on now, and it's actually a lot simpler
> than the description suggests, at least to my reading.  Perhaps language
> like:
> 
> After commit 5c3bf6cba791 ("bonding: assign random address if device
> address is same as bond"), bonding will erroneously randomize the MAC
> address of the first interface added to the bond if fail_over_mac =
> follow.
> 
> Correct this by additionally testing for the bond being empty before
> randomizing the MAC.
> 
> 	Does that sound ok to you?

Sure. As a non native English speaker, I always struggle to organize the patch
description. Thanks for your update.

Regards
Hangbin