Re: [PATCHv2 net] bonding: use permanent address for MAC swapping if device address is same

[Jay Vosburgh <jv@jvosburgh.net>]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>On Tue, Apr 15, 2025 at 06:15:12PM -0700, Jay Vosburgh wrote:
>> >> 
>> >> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> >> index 950d8e4d86f8..0d4e1ddd900d 100644
>> >> --- a/drivers/net/bonding/bond_main.c
>> >> +++ b/drivers/net/bonding/bond_main.c
>> >> @@ -2120,6 +2120,24 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>> >>  			slave_err(bond_dev, slave_dev, "Error %d calling set_mac_address\n", res);
>> >>  			goto err_restore_mtu;
>> >>  		}
>> >> +	} 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 current active slave's permanent mac address to
>> >> +		 * avoid duplicate mac address.
>> >> +		 */
>> >> +		curr_active_slave = rcu_dereference(bond->curr_active_slave);
>> >> +		if (curr_active_slave) {
>> >> +			memcpy(ss.__data, curr_active_slave->perm_hwaddr,
>> >> +			       curr_active_slave->dev->addr_len);
>> >> +			ss.ss_family = slave_dev->type;
>> >> +			res = dev_set_mac_address(slave_dev, (struct sockaddr *)&ss,
>> >> +					extack);
>> >> +			if (res) {
>> >> +				slave_err(bond_dev, slave_dev, "Error %d calling set_mac_address\n", res);
>> >> +				goto err_restore_mtu;
>> >> +			}
>> >> +		}
>> 
>> 	Is this in replacement of the prior patch (that does stuff
>> during failover), or in addition to?
>> 
>> 	I'm asking because in the above, if there is no
>> curr_active_slave, e.g., all interfaces in the bond are down, the above
>> would permit MAC conflict in the absence of logic in failover to resolve
>> things.
>
>Hmm, then how about use bond_for_each_slave() and find out the link
>that has same MAC address with bond/new_slave?

	But even if we find it, aren't we stuck at that point?  The
situation would be that the bond and one backup interface have MAC#1.
MAC#1 may or may not be that backup interface's permanent MAC address,
and we're adding another interface, also with MAC#1, which might be the
newly added interface's permanent MAC.  The MAC swap gyrations to
guarantee this would work correctly in all cases seem to be rather
involved.

	Wouldn't it be equally effective to, when the conflicting
interface is added, give it a random MAC to avoid the conflict?  That
random MAC shouldn't end up as the bond's MAC, so it would exist only as
a placeholder of sorts.

	I'm unsure if there are many (any?) devices in common use today
that actually have issues with multiple ports using the same MAC, so I
don't think we need an overly complicated solution.

	-J

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