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

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

Message-ID: <155385.1744949793@famine>
Date: Thu, 17 Apr 2025 21:16:33 -0700
From: Jay Vosburgh <jv@...sburgh.net>
To: Hangbin Liu <liuhangbin@...il.com>
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>, Cosmin Ratiu <cratiu@...dia.com>,
    linux-kernel@...r.kernel.org
Subject: Re: [PATCHv2 net] bonding: use permanent address for MAC swapping if
 device address is same

Hangbin Liu <liuhangbin@...il.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@...sburgh.net