netdev - Re: [PATCH net-next v1] net: bonding: move bond_should_notify

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

Message-ID: <4dcae50b-42f8-4adb-b154-5974f5aec19d@redhat.com>
Date: Thu, 20 Nov 2025 13:53:00 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Tonghao Zhang <tonghao@...aicloud.com>, netdev@...r.kernel.org
Cc: Jay Vosburgh <jv@...sburgh.net>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Simon Horman <horms@...nel.org>, Jonathan Corbet <corbet@....net>,
 Andrew Lunn <andrew+netdev@...n.ch>,
 Nikolay Aleksandrov <razor@...ckwall.org>, Hangbin Liu <liuhangbin@...il.com>
Subject: Re: [PATCH net-next v1] net: bonding: move bond_should_notify_peers,
 e.g. into rtnl lock block

On 11/18/25 10:04 AM, Tonghao Zhang wrote:
> In bond_mii_monitor()/bond_activebackup_arp_mon(), when we hold the rtnl lock:
> 
> - check send_peer_notif again to avoid unconditionally reducing this value.
> - send_peer_notif may have been reset. Therefore, it is necessary to check
>   whether to send peer notify via bond_should_notify_peers() to avoid the
>   loss of notification events.

This looks strictly related to:

https://patchwork.kernel.org/project/netdevbpf/patch/20251118090305.35558-1-tonghao@bamaicloud.com/

you probably should bundle both in a series.

> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index b7370c918978..6f0fa78fa3f3 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -2810,11 +2810,10 @@ static void bond_mii_monitor(struct work_struct *work)
>  {
>  	struct bonding *bond = container_of(work, struct bonding,
>  					    mii_work.work);
> -	bool should_notify_peers;
> -	bool commit;
> -	unsigned long delay;
> -	struct slave *slave;
>  	struct list_head *iter;
> +	struct slave *slave;
> +	unsigned long delay;
> +	bool commit;
>  
>  	delay = msecs_to_jiffies(bond->params.miimon);
>  
> @@ -2823,7 +2822,6 @@ static void bond_mii_monitor(struct work_struct *work)
>  
>  	rcu_read_lock();
>  
> -	should_notify_peers = bond_should_notify_peers(bond);
>  	commit = !!bond_miimon_inspect(bond);
>  
>  	rcu_read_unlock();
> @@ -2844,10 +2842,10 @@ static void bond_mii_monitor(struct work_struct *work)
>  		}
>  
>  		if (bond->send_peer_notif) {

The first `bond->send_peer_notif` access is outside the lock. I think
the compiler could do funny things and read the field only outside the
lock: I guess you need additional ONCE annotation, and that could be a
separate patch.

> -			bond->send_peer_notif--;
> -			if (should_notify_peers)
> +			if (bond_should_notify_peers(bond))
>  				call_netdevice_notifiers(NETDEV_NOTIFY_PEERS,
>  							 bond->dev);
> +			bond->send_peer_notif--;
>  		}
>  
>  		rtnl_unlock();	/* might sleep, hold no other locks */
> @@ -3759,8 +3757,7 @@ static bool bond_ab_arp_probe(struct bonding *bond)
>  
>  static void bond_activebackup_arp_mon(struct bonding *bond)
>  {
> -	bool should_notify_peers = false;
> -	bool should_notify_rtnl = false;
> +	bool should_notify_rtnl;
>  	int delta_in_ticks;
>  
>  	delta_in_ticks = msecs_to_jiffies(bond->params.arp_interval);
> @@ -3770,15 +3767,12 @@ static void bond_activebackup_arp_mon(struct bonding *bond)
>  
>  	rcu_read_lock();
>  
> -	should_notify_peers = bond_should_notify_peers(bond);
> -
>  	if (bond_ab_arp_inspect(bond)) {
>  		rcu_read_unlock();
>  
>  		/* Race avoidance with bond_close flush of workqueue */
>  		if (!rtnl_trylock()) {
>  			delta_in_ticks = 1;
> -			should_notify_peers = false;
>  			goto re_arm;
>  		}
>  
> @@ -3791,18 +3785,15 @@ static void bond_activebackup_arp_mon(struct bonding *bond)
>  	should_notify_rtnl = bond_ab_arp_probe(bond);
>  	rcu_read_unlock();
>  
> -re_arm:
> -	if (bond->params.arp_interval)
> -		queue_delayed_work(bond->wq, &bond->arp_work, delta_in_ticks);
> -
> -	if (should_notify_peers || should_notify_rtnl) {
> +	if (bond->send_peer_notif || should_notify_rtnl) {
>  		if (!rtnl_trylock())
>  			return;

The above skips the 2nd trylock attempt when the first one fail, which
IMHO makes sense, but its unrelated from the rest of the change here. I
think this specific bits should go in a separate patch.

/P