Re: [PATCH net-next v3 04/13] net: move promiscuity handling into dev_rx_mode_work

On 03/20, Loktionov, Aleksandr wrote:

quoted

-----Original Message-----
From: Stanislav Fomichev <sdf@fomichev.me>
Sent: Friday, March 20, 2026 2:25 AM
To: <redacted>
Cc: davem@davemloft.net; <redacted>; kuba@kernel.org;
pabeni@redhat.com; horms@kernel.org; corbet@lwn.net;
skhan@linuxfoundation.org; andrew+netdev@lunn.ch;
michael.chan@broadcom.com; pavan.chebbi@broadcom.com; Nguyen, Anthony
L [off-list ref]; Kitszel, Przemyslaw
[off-list ref]; saeedm@nvidia.com; tariqt@nvidia.com;
mbloch@nvidia.com; alexanderduyck@fb.com; kernel-team@meta.com;
johannes@sipsolutions.net; sd@queasysnail.net; jianbol@nvidia.com;
dtatulea@nvidia.com; sdf@fomichev.me; mohsin.bashr@gmail.com; Keller,
Jacob E [off-list ref]; willemb@google.com;
skhawaja@google.com; bestswngs@gmail.com; Loktionov, Aleksandr
[off-list ref]; kees@kernel.org; linux-
doc@vger.kernel.org; linux-kernel@vger.kernel.org; intel-wired-
lan@lists.osuosl.org; linux-rdma@vger.kernel.org; linux-
wireless@vger.kernel.org; linux-kselftest@vger.kernel.org;
leon@kernel.org
Subject: [PATCH net-next v3 04/13] net: move promiscuity handling into
dev_rx_mode_work

Move unicast promiscuity tracking into dev_rx_mode_work so it runs
under netdev_ops_lock instead of under the addr_lock spinlock. This is
required because __dev_set_promiscuity calls dev_change_rx_flags and
__dev_notify_flags, both of which may need to sleep.

Change ASSERT_RTNL() to netdev_ops_assert_locked() in
__dev_set_promiscuity, netif_set_allmulti and __dev_change_flags since
these are now called from the work queue under the ops lock.

Reviewed-by: Aleksandr Loktionov <redacted>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 Documentation/networking/netdevices.rst |  4 ++
 net/core/dev.c                          | 79 +++++++++++++++++-------
-
 2 files changed, 57 insertions(+), 26 deletions(-)

diff --git a/Documentation/networking/netdevices.rst

b/Documentation/networking/netdevices.rst
index dc83d78d3b27..5cdaa1a3dcc8 100644

--- a/Documentation/networking/netdevices.rst
+++ b/Documentation/networking/netdevices.rst

@@ -298,6 +298,10 @@ struct net_device synchronization rules
 	Notes: Sleepable version of ndo_set_rx_mode. Receives snapshots
 	of the unicast and multicast address lists.

+ndo_change_rx_flags:
+	Synchronization: rtnl_lock() semaphore. In addition, netdev

instance
+	lock if the driver implements queue management or shaper API.
+
 ndo_setup_tc:
 	``TC_SETUP_BLOCK`` and ``TC_SETUP_FT`` are running under NFT
locks
 	(i.e. no ``rtnl_lock`` and no device instance lock). The rest
of diff --git a/net/core/dev.c b/net/core/dev.c index
fedc423306fc..fc5c9b14faa0 100644

--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -9574,7 +9574,7 @@ static int __dev_set_promiscuity(struct

net_device *dev, int inc, bool notify)
 	kuid_t uid;
 	kgid_t gid;

-	ASSERT_RTNL();
+	netdev_ops_assert_locked(dev);

Can you explain why do you add new hard precondition of ops lock must be held?

The context is that in f792709e0baa ("selftests: net: validate team flags
propagation") I had to add locking around NETDEV_CHANGE notifiers and
add that ugly `if (notify) netdev_ops_assert_locked` check. After this
patch I believe we are consistently calling __dev_set_promiscuity
with the ops lock (for ops locked netdev), so we can cleanup this enforcement
part. 

quoted

 	promiscuity = dev->promiscuity + inc;
 	if (promiscuity == 0) {

@@ -9610,16 +9610,8 @@ static int __dev_set_promiscuity(struct

net_device *dev, int inc, bool notify)

 		dev_change_rx_flags(dev, IFF_PROMISC);
 	}

...

quoted

 	__hw_addr_init(&uc_snap);

@@ -9704,16 +9720,29 @@ static void dev_rx_mode_work(struct

work_struct *work)
 		if (!err)
 			err = __hw_addr_list_snapshot(&mc_ref, &dev->mc,
 						      dev->addr_len);
-		netif_addr_unlock_bh(dev);

 		if (err) {
 			netdev_WARN(dev, "failed to sync uc/mc
addresses\n");
 			__hw_addr_flush(&uc_snap);
 			__hw_addr_flush(&uc_ref);
 			__hw_addr_flush(&mc_snap);
+			netif_addr_unlock_bh(dev);
 			goto out;
 		}

+		promisc_inc = dev_uc_promisc_update(dev);
+
+		netif_addr_unlock_bh(dev);
+	} else {
+		netif_addr_lock_bh(dev);
+		promisc_inc = dev_uc_promisc_update(dev);
+		netif_addr_unlock_bh(dev);
+	}
+
+	if (promisc_inc)
+		__dev_set_promiscuity(dev, promisc_inc, false);

But it's being called here without any netdev_lock_ops(dev) ?

We have the following at the start of dev_rx_mode_work:
  rtnl_lock();
  netdev_lock_ops(dev);

Or am I looking at something else?