linux-kernel - Re: [PATCH net-next] bonding: report duplicate MAC address in all situations

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: <20250224141008.3ee3a74b@kernel.org>
Date: Mon, 24 Feb 2025 14:10:08 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: netdev@...r.kernel.org, Jay Vosburgh <jv@...sburgh.net>, Andrew Lunn
 <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>, Eric
 Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Nikolay
 Aleksandrov <razor@...ckwall.org>, Simon Horman <horms@...nel.org>,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] bonding: report duplicate MAC address in all
 situations

On Wed, 19 Feb 2025 07:55:15 +0000 Hangbin Liu wrote:
> Normally, a bond uses the MAC address of the first added slave as the
> bond’s MAC address. And the bond will set active slave’s MAC address to
> bond’s address if fail_over_mac is set to none (0) or follow (2).
> 
> When the first slave is removed, the bond will still use the removed
> slave’s MAC address, which can lead to a duplicate MAC address and
> potentially cause issues with the switch. To avoid confusion, let's warn
> the user in all situations, including when fail_over_mac is set to 2 or
> in active-backup mode.

Makes sense, thanks for the high quality commit message.

False positive warnings are annoying to users (especially users who
monitor all warnings in their fleet). Could we stick to filtering out
the BOND_FOM_ACTIVE case? Looks like this condition:

	if (bond->params.fail_over_mac != BOND_FOM_ACTIVE ||
	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {

exists a few lines later in __bond_release_one()

> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index e45bba240cbc..ca66107776cc 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -2551,13 +2551,11 @@ static int __bond_release_one(struct net_device *bond_dev,
>  
>  	RCU_INIT_POINTER(bond->current_arp_slave, NULL);
>  
> -	if (!all && (!bond->params.fail_over_mac ||
> -		     BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP)) {
> -		if (ether_addr_equal_64bits(bond_dev->dev_addr, slave->perm_hwaddr) &&
> -		    bond_has_slaves(bond))
> -			slave_warn(bond_dev, slave_dev, "the permanent HWaddr of slave - %pM - is still in use by bond - set the HWaddr of slave to a different address to avoid conflicts\n",
> -				   slave->perm_hwaddr);
> -	}
> +	if (!all &&
> +	    ether_addr_equal_64bits(bond_dev->dev_addr, slave->perm_hwaddr) &&
> +	    bond_has_slaves(bond))
> +		slave_warn(bond_dev, slave_dev, "the permanent HWaddr of slave - %pM - is still in use by bond - set the HWaddr of slave to a different address to avoid conflicts\n",
> +			   slave->perm_hwaddr);
-- 
pw-bot: cr