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

[Jay Vosburgh <jv@jvosburgh.net>]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>Hi Jay,
>
>On Mon, Aug 25, 2025 at 04:07:55AM +0000, Hangbin Liu wrote:
>> On Fri, Aug 22, 2025 at 05:21:30PM -0700, Jay Vosburgh wrote:
>> > Hangbin Liu <liuhangbin@gmail.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@virtme-ng net]# ip link add type veth
>> [root@virtme-ng net]# ip link add bond0 type bond mode 1 miimon 100 fail_over_mac 2
>> [root@virtme-ng net]# ip link show
>> 3: veth0@veth1: <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@veth0: <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@virtme-ng net]# ip link set veth0 master bond0
>> [root@virtme-ng net]# ip link show
>> 3: veth0@veth1: <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@veth0: <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?

	-J

>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@redhat.com>
>> > >Signed-off-by: Hangbin Liu <liuhangbin@gmail.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@jvosburgh.net
>> > 

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