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 <[email protected]> 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?
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 <[email protected]>
> > >Signed-off-by: Hangbin Liu <[email protected]>
> > >---
> > > 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, [email protected]
> >
