[PATCH net v4 00/11] net: hold instance lock during NETDEV_UP/REGISTER

[PATCH net v4 00/11] net: hold instance lock during NETDEV_UP/REGISTER

[Stanislav Fomichev]

Solving the issue reported by Cosmin in [0] requires consistent
lock during NETDEV_UP/REGISTER notifiers. This series
addresses that (along with some other fixes in net/ipv4/devinet.c
and net/ipv6/addrconf.c) and appends the patches from Jakub
that were conditional on consistent locking in NETDEV_UNREGISTER.

0: https://lore.kernel.org/netdev/700fa36b94cbd57cfea2622029b087643c80cbc9.camel@nvidia.com/

v4:
- make sure EXPORT_IPV6_MOD works for !CONFIG_INET configs (Intel LKP)

v3:
- s/EXPORT_SYMBOL/EXPORT_IPV6_MOD/ for netif_disable_lro (Jakub)
- drop netdev_lockdep_set_classes mlx5 patch (Jakub)
- update doc to clarify that instance lock is held for ops-locked
  devices only, also list NETDEV_UNREGISTER as unlocked (Jakub)
- drop patch that unlocks and relocks bonding and teaming
  UNREGISTER notifiers (Jakub)
- lock ops (instead of unconditional instance lock) in addrconf (Jakub)
- rename rtnl_net_debug.c to lock_debug.c and share rtnl_net_debug_event
  (as netdev_debug_event) with netdevsim (Jakub)
- use netdevsim in one of the existing tests that set netns (Jakub)
- add ops locks around xp_clear_dev in xsk_notifier

v2:
- export netdev_get_by_index_lock
- new patch: add netdev_lockdep_set_classes to mlx5
- new patch: exercise notifiers in netdevsim
- ignore specific locked netdev in call_netdevice_register_notifiers,
  not all

Jakub Kicinski (3):
  net: designate XSK pool pointers in queues as "ops protected"
  netdev: add "ops compat locking" helpers
  netdev: don't hold rtnl_lock over nl queue info get when possible

Stanislav Fomichev (8):
  net: switch to netif_disable_lro in inetdev_init
  net: hold instance lock during NETDEV_REGISTER/UP
  net: use netif_disable_lro in ipv6_add_dev
  net: rename rtnl_net_debug to lock_debug
  netdevsim: add dummy device notifiers
  net: dummy: request ops lock
  docs: net: document netdev notifier expectations
  selftests: net: use netdevsim in netns test

 Documentation/networking/netdevices.rst     | 22 ++++++++
 drivers/net/dummy.c                         |  1 +
 drivers/net/netdevsim/netdev.c              | 10 ++++
 drivers/net/netdevsim/netdevsim.h           |  3 +
 include/linux/netdevice.h                   |  3 +-
 include/net/ip.h                            | 16 +++---
 include/net/netdev_lock.h                   | 27 +++++++++
 include/net/netdev_rx_queue.h               |  6 +-
 net/core/Makefile                           |  2 +-
 net/core/dev.c                              | 62 ++++++++++++++++++++-
 net/core/dev.h                              | 15 +++++
 net/core/dev_api.c                          |  4 +-
 net/core/{rtnl_net_debug.c => lock_debug.c} | 14 +++--
 net/core/netdev-genl.c                      | 18 +++---
 net/core/rtnetlink.c                        | 10 ++--
 net/ipv4/devinet.c                          |  2 +-
 net/ipv6/addrconf.c                         | 15 ++++-
 net/xdp/xsk.c                               |  2 +
 net/xdp/xsk_buff_pool.c                     |  7 ++-
 tools/testing/selftests/net/lib.sh          | 25 +++++++++
 tools/testing/selftests/net/netns-name.sh   | 13 +++--
 21 files changed, 229 insertions(+), 48 deletions(-)
 rename net/core/{rtnl_net_debug.c => lock_debug.c} (90%)

-- 
2.48.1

[PATCH net v4 01/11] net: switch to netif_disable_lro in inetdev_init

[Stanislav Fomichev]

Cosmin reports the following deadlock:
dump_stack_lvl+0x62/0x90
print_deadlock_bug+0x274/0x3b0
__lock_acquire+0x1229/0x2470
lock_acquire+0xb7/0x2b0
__mutex_lock+0xa6/0xd20
dev_disable_lro+0x20/0x80
inetdev_init+0x12f/0x1f0
inetdev_event+0x48b/0x870
notifier_call_chain+0x38/0xf0
netif_change_net_namespace+0x72e/0x9f0
do_setlink.isra.0+0xd5/0x1220
rtnl_newlink+0x7ea/0xb50
rtnetlink_rcv_msg+0x459/0x5e0
netlink_rcv_skb+0x54/0x100
netlink_unicast+0x193/0x270
netlink_sendmsg+0x204/0x450

Switch to netif_disable_lro which assumes the caller holds the instance
lock. inetdev_init is called for blackhole device (which sw device and
doesn't grab instance lock) and from REGISTER/UNREGISTER notifiers.
We already hold the instance lock for REGISTER notifier during
netns change and we'll soon hold the lock during other paths.

Reviewed-by: Jakub Kicinski <kuba@kernel.org>
Reported-by: Cosmin Ratiu <cratiu@nvidia.com>
Fixes: ad7c7b2172c3 ("net: hold netdev instance lock during sysfs operations")
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 include/net/ip.h   | 16 ++++++++--------
 net/core/dev.c     |  1 +
 net/ipv4/devinet.c |  2 +-
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/include/net/ip.h b/include/net/ip.h
index 8a48ade24620..47ed6d23853d 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -667,14 +667,6 @@ static inline void ip_ipgre_mc_map(__be32 naddr, const unsigned char *broadcast,
 		memcpy(buf, &naddr, sizeof(naddr));
 }
 
-#if IS_MODULE(CONFIG_IPV6)
-#define EXPORT_IPV6_MOD(X) EXPORT_SYMBOL(X)
-#define EXPORT_IPV6_MOD_GPL(X) EXPORT_SYMBOL_GPL(X)
-#else
-#define EXPORT_IPV6_MOD(X)
-#define EXPORT_IPV6_MOD_GPL(X)
-#endif
-
 #if IS_ENABLED(CONFIG_IPV6)
 #include <linux/ipv6.h>
 #endif
@@ -694,6 +686,14 @@ static __inline__ void inet_reset_saddr(struct sock *sk)
 
 #endif
 
+#if IS_MODULE(CONFIG_IPV6)
+#define EXPORT_IPV6_MOD(X) EXPORT_SYMBOL(X)
+#define EXPORT_IPV6_MOD_GPL(X) EXPORT_SYMBOL_GPL(X)
+#else
+#define EXPORT_IPV6_MOD(X)
+#define EXPORT_IPV6_MOD_GPL(X)
+#endif
+
 static inline unsigned int ipv4_addr_hash(__be32 ip)
 {
 	return (__force unsigned int) ip;
diff --git a/net/core/dev.c b/net/core/dev.c
index be17e0660144..711a946d4bfb 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1771,6 +1771,7 @@ void netif_disable_lro(struct net_device *dev)
 		netdev_unlock_ops(lower_dev);
 	}
 }
+EXPORT_IPV6_MOD(netif_disable_lro);
 
 /**
  *	dev_disable_gro_hw - disable HW Generic Receive Offload on a device
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 754f60fb6e25..77e5705ac799 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -281,7 +281,7 @@ static struct in_device *inetdev_init(struct net_device *dev)
 	if (!in_dev->arp_parms)
 		goto out_kfree;
 	if (IPV4_DEVCONF(in_dev->cnf, FORWARDING))
-		dev_disable_lro(dev);
+		netif_disable_lro(dev);
 	/* Reference in_dev->dev */
 	netdev_hold(dev, &in_dev->dev_tracker, GFP_KERNEL);
 	/* Account for reference dev->ip_ptr (below) */
-- 
2.48.1

[PATCH net v4 02/11] net: hold instance lock during NETDEV_REGISTER/UP

[Stanislav Fomichev]

Callers of inetdev_init can come from several places with inconsistent
expectation about netdev instance lock. Grab instance lock during
REGISTER (plus UP). Also solve the inconsistency with UNREGISTER
where it was locked only during move netns path.

WARNING: CPU: 10 PID: 1479 at ./include/net/netdev_lock.h:54
__netdev_update_features+0x65f/0xca0
__warn+0x81/0x180
__netdev_update_features+0x65f/0xca0
report_bug+0x156/0x180
handle_bug+0x4f/0x90
exc_invalid_op+0x13/0x60
asm_exc_invalid_op+0x16/0x20
__netdev_update_features+0x65f/0xca0
netif_disable_lro+0x30/0x1d0
inetdev_init+0x12f/0x1f0
inetdev_event+0x48b/0x870
notifier_call_chain+0x38/0xf0
register_netdevice+0x741/0x8b0
register_netdev+0x1f/0x40
mlx5e_probe+0x4e3/0x8e0 [mlx5_core]
auxiliary_bus_probe+0x3f/0x90
really_probe+0xc3/0x3a0
__driver_probe_device+0x80/0x150
driver_probe_device+0x1f/0x90
__device_attach_driver+0x7d/0x100
bus_for_each_drv+0x80/0xd0
__device_attach+0xb4/0x1c0
bus_probe_device+0x91/0xa0
device_add+0x657/0x870

Reported-by: Cosmin Ratiu <cratiu@nvidia.com>
Fixes: ad7c7b2172c3 ("net: hold netdev instance lock during sysfs operations")
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 include/linux/netdevice.h |  2 +-
 net/core/dev.c            | 12 +++++++++---
 net/core/dev_api.c        |  4 +---
 net/core/rtnetlink.c      | 10 +++++-----
 4 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index fa79145518d1..cf3b6445817b 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4192,7 +4192,7 @@ int dev_change_flags(struct net_device *dev, unsigned int flags,
 int netif_set_alias(struct net_device *dev, const char *alias, size_t len);
 int dev_set_alias(struct net_device *, const char *, size_t);
 int dev_get_alias(const struct net_device *, char *, size_t);
-int netif_change_net_namespace(struct net_device *dev, struct net *net,
+int __dev_change_net_namespace(struct net_device *dev, struct net *net,
 			       const char *pat, int new_ifindex,
 			       struct netlink_ext_ack *extack);
 int dev_change_net_namespace(struct net_device *dev, struct net *net,
diff --git a/net/core/dev.c b/net/core/dev.c
index 711a946d4bfb..e59eb173900d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1859,7 +1859,9 @@ static int call_netdevice_register_net_notifiers(struct notifier_block *nb,
 	int err;
 
 	for_each_netdev(net, dev) {
+		netdev_lock_ops(dev);
 		err = call_netdevice_register_notifiers(nb, dev);
+		netdev_unlock_ops(dev);
 		if (err)
 			goto rollback;
 	}
@@ -11046,7 +11048,9 @@ int register_netdevice(struct net_device *dev)
 		memcpy(dev->perm_addr, dev->dev_addr, dev->addr_len);
 
 	/* Notify protocols, that a new device appeared. */
+	netdev_lock_ops(dev);
 	ret = call_netdevice_notifiers(NETDEV_REGISTER, dev);
+	netdev_unlock_ops(dev);
 	ret = notifier_to_errno(ret);
 	if (ret) {
 		/* Expect explicit free_netdev() on failure */
@@ -12058,7 +12062,7 @@ void unregister_netdev(struct net_device *dev)
 }
 EXPORT_SYMBOL(unregister_netdev);
 
-int netif_change_net_namespace(struct net_device *dev, struct net *net,
+int __dev_change_net_namespace(struct net_device *dev, struct net *net,
 			       const char *pat, int new_ifindex,
 			       struct netlink_ext_ack *extack)
 {
@@ -12143,11 +12147,12 @@ int netif_change_net_namespace(struct net_device *dev, struct net *net,
 	 * And now a mini version of register_netdevice unregister_netdevice.
 	 */
 
+	netdev_lock_ops(dev);
 	/* If device is running close it first. */
 	netif_close(dev);
-
 	/* And unlink it from device chain */
 	unlist_netdevice(dev);
+	netdev_unlock_ops(dev);
 
 	synchronize_net();
 
@@ -12209,11 +12214,12 @@ int netif_change_net_namespace(struct net_device *dev, struct net *net,
 	err = netdev_change_owner(dev, net_old, net);
 	WARN_ON(err);
 
+	netdev_lock_ops(dev);
 	/* Add the device back in the hashes */
 	list_netdevice(dev);
-
 	/* Notify protocols, that a new device appeared. */
 	call_netdevice_notifiers(NETDEV_REGISTER, dev);
+	netdev_unlock_ops(dev);
 
 	/*
 	 *	Prevent userspace races by waiting until the network
diff --git a/net/core/dev_api.c b/net/core/dev_api.c
index 8dbc60612100..cb3e5807dce8 100644
--- a/net/core/dev_api.c
+++ b/net/core/dev_api.c
@@ -119,9 +119,7 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net,
 {
 	int ret;
 
-	netdev_lock_ops(dev);
-	ret = netif_change_net_namespace(dev, net, pat, 0, NULL);
-	netdev_unlock_ops(dev);
+	ret = __dev_change_net_namespace(dev, net, pat, 0, NULL);
 
 	return ret;
 }
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 334db17be37d..f49665851172 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3025,8 +3025,6 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
 	char ifname[IFNAMSIZ];
 	int err;
 
-	netdev_lock_ops(dev);
-
 	err = validate_linkmsg(dev, tb, extack);
 	if (err < 0)
 		goto errout;
@@ -3042,14 +3040,16 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
 
 		new_ifindex = nla_get_s32_default(tb[IFLA_NEW_IFINDEX], 0);
 
-		err = netif_change_net_namespace(dev, tgt_net, pat,
-						 new_ifindex, extack);
+		err = __dev_change_net_namespace(dev, tgt_net, pat, new_ifindex,
+						 extack);
 		if (err)
-			goto errout;
+			return err;
 
 		status |= DO_SETLINK_MODIFIED;
 	}
 
+	netdev_lock_ops(dev);
+
 	if (tb[IFLA_MAP]) {
 		struct rtnl_link_ifmap *u_map;
 		struct ifmap k_map;
-- 
2.48.1

[PATCH net v4 03/11] net: use netif_disable_lro in ipv6_add_dev

[Stanislav Fomichev]

ipv6_add_dev might call dev_disable_lro which unconditionally grabs
instance lock, so it will deadlock during NETDEV_REGISTER. Switch
to netif_disable_lro.

Make sure all callers hold the instance lock as well.

Cc: Cosmin Ratiu <cratiu@nvidia.com>
Fixes: ad7c7b2172c3 ("net: hold netdev instance lock during sysfs operations")
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 net/ipv6/addrconf.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index ac8cc1076536..35477d494573 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -80,6 +80,7 @@
 #include <net/netlink.h>
 #include <net/pkt_sched.h>
 #include <net/l3mdev.h>
+#include <net/netdev_lock.h>
 #include <linux/if_tunnel.h>
 #include <linux/rtnetlink.h>
 #include <linux/netconf.h>
@@ -377,6 +378,7 @@ static struct inet6_dev *ipv6_add_dev(struct net_device *dev)
 	int err = -ENOMEM;
 
 	ASSERT_RTNL();
+	netdev_ops_assert_locked(dev);
 
 	if (dev->mtu < IPV6_MIN_MTU && dev != blackhole_netdev)
 		return ERR_PTR(-EINVAL);
@@ -402,7 +404,7 @@ static struct inet6_dev *ipv6_add_dev(struct net_device *dev)
 		return ERR_PTR(err);
 	}
 	if (ndev->cnf.forwarding)
-		dev_disable_lro(dev);
+		netif_disable_lro(dev);
 	/* We refer to the device */
 	netdev_hold(dev, &ndev->dev_tracker, GFP_KERNEL);
 
@@ -3152,10 +3154,12 @@ int addrconf_add_ifaddr(struct net *net, void __user *arg)
 
 	rtnl_net_lock(net);
 	dev = __dev_get_by_index(net, ireq.ifr6_ifindex);
+	netdev_lock_ops(dev);
 	if (dev)
 		err = inet6_addr_add(net, dev, &cfg, 0, 0, NULL);
 	else
 		err = -ENODEV;
+	netdev_unlock_ops(dev);
 	rtnl_net_unlock(net);
 	return err;
 }
@@ -5026,9 +5030,10 @@ inet6_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (!dev) {
 		NL_SET_ERR_MSG_MOD(extack, "Unable to find the interface");
 		err = -ENODEV;
-		goto unlock;
+		goto unlock_rtnl;
 	}
 
+	netdev_lock_ops(dev);
 	idev = ipv6_find_idev(dev);
 	if (IS_ERR(idev)) {
 		err = PTR_ERR(idev);
@@ -5065,6 +5070,8 @@ inet6_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	in6_ifa_put(ifa);
 unlock:
+	netdev_unlock_ops(dev);
+unlock_rtnl:
 	rtnl_net_unlock(net);
 
 	return err;
@@ -6503,7 +6510,9 @@ static int addrconf_sysctl_addr_gen_mode(const struct ctl_table *ctl, int write,
 
 			if (idev->cnf.addr_gen_mode != new_val) {
 				WRITE_ONCE(idev->cnf.addr_gen_mode, new_val);
+				netdev_lock_ops(idev->dev);
 				addrconf_init_auto_addrs(idev->dev);
+				netdev_unlock_ops(idev->dev);
 			}
 		} else if (&net->ipv6.devconf_all->addr_gen_mode == ctl->data) {
 			struct net_device *dev;
@@ -6515,7 +6524,9 @@ static int addrconf_sysctl_addr_gen_mode(const struct ctl_table *ctl, int write,
 				    idev->cnf.addr_gen_mode != new_val) {
 					WRITE_ONCE(idev->cnf.addr_gen_mode,
 						  new_val);
+					netdev_lock_ops(idev->dev);
 					addrconf_init_auto_addrs(idev->dev);
+					netdev_unlock_ops(idev->dev);
 				}
 			}
 		}
-- 
2.48.1

[PATCH net v4 04/11] net: rename rtnl_net_debug to lock_debug

[Stanislav Fomichev]

And make it selected by CONFIG_DEBUG_NET. Don't rename any of
the structs/functions. Next patch will use rtnl_net_debug_event in
netdevsim.

Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 net/core/Makefile                           | 2 +-
 net/core/{rtnl_net_debug.c => lock_debug.c} | 0
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename net/core/{rtnl_net_debug.c => lock_debug.c} (100%)

diff --git a/net/core/Makefile b/net/core/Makefile
index a10c3bd96798..b2a76ce33932 100644
--- a/net/core/Makefile
+++ b/net/core/Makefile
@@ -45,5 +45,5 @@ obj-$(CONFIG_BPF_SYSCALL) += bpf_sk_storage.o
 obj-$(CONFIG_OF)	+= of_net.o
 obj-$(CONFIG_NET_TEST) += net_test.o
 obj-$(CONFIG_NET_DEVMEM) += devmem.o
-obj-$(CONFIG_DEBUG_NET_SMALL_RTNL) += rtnl_net_debug.o
+obj-$(CONFIG_DEBUG_NET) += lock_debug.o
 obj-$(CONFIG_FAIL_SKB_REALLOC) += skb_fault_injection.o
diff --git a/net/core/rtnl_net_debug.c b/net/core/lock_debug.c
similarity index 100%
rename from net/core/rtnl_net_debug.c
rename to net/core/lock_debug.c
-- 
2.48.1

[PATCH net v4 05/11] netdevsim: add dummy device notifiers

[Stanislav Fomichev]

In order to exercise and verify notifiers' locking assumptions,
register dummy notifiers (via register_netdevice_notifier_dev_net).
Share notifier event handler that enforces the assumptions with
lock_debug.c (rename and export rtnl_net_debug_event as
netdev_debug_event). Add ops lock asserts to netdev_debug_event.

Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 drivers/net/netdevsim/netdev.c    | 10 ++++++++++
 drivers/net/netdevsim/netdevsim.h |  3 +++
 include/net/netdev_lock.h         | 11 +++++++++++
 net/core/lock_debug.c             | 14 +++++++++-----
 4 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index b67af4651185..6188699aa241 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -939,6 +939,7 @@ static int nsim_init_netdevsim(struct netdevsim *ns)
 	ns->netdev->netdev_ops = &nsim_netdev_ops;
 	ns->netdev->stat_ops = &nsim_stat_ops;
 	ns->netdev->queue_mgmt_ops = &nsim_queue_mgmt_ops;
+	netdev_lockdep_set_classes(ns->netdev);
 
 	err = nsim_udp_tunnels_info_create(ns->nsim_dev, ns->netdev);
 	if (err)
@@ -960,6 +961,11 @@ static int nsim_init_netdevsim(struct netdevsim *ns)
 	if (err)
 		goto err_ipsec_teardown;
 	rtnl_unlock();
+
+	ns->nb.notifier_call = netdev_debug_event;
+	if (register_netdevice_notifier_dev_net(ns->netdev, &ns->nb, &ns->nn))
+		ns->nb.notifier_call = NULL;
+
 	return 0;
 
 err_ipsec_teardown:
@@ -1043,6 +1049,10 @@ void nsim_destroy(struct netdevsim *ns)
 	debugfs_remove(ns->qr_dfs);
 	debugfs_remove(ns->pp_dfs);
 
+	if (ns->nb.notifier_call)
+		unregister_netdevice_notifier_dev_net(ns->netdev, &ns->nb,
+						      &ns->nn);
+
 	rtnl_lock();
 	peer = rtnl_dereference(ns->peer);
 	if (peer)
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index 665020d18f29..d04401f0bdf7 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -144,6 +144,9 @@ struct netdevsim {
 
 	struct nsim_ethtool ethtool;
 	struct netdevsim __rcu *peer;
+
+	struct notifier_block nb;
+	struct netdev_net_notifier nn;
 };
 
 struct netdevsim *
diff --git a/include/net/netdev_lock.h b/include/net/netdev_lock.h
index 1c0c9a94cc22..5f712de5bf8a 100644
--- a/include/net/netdev_lock.h
+++ b/include/net/netdev_lock.h
@@ -98,4 +98,15 @@ static inline int netdev_lock_cmp_fn(const struct lockdep_map *a,
 				  &qdisc_xmit_lock_key);	\
 }
 
+#if IS_ENABLED(CONFIG_DEBUG_NET)
+int netdev_debug_event(struct notifier_block *nb, unsigned long event,
+		       void *ptr);
+#else
+static inline int netdev_debug_event(struct notifier_block *nb,
+				     unsigned long event, void *ptr)
+{
+	return 0;
+}
+#endif
+
 #endif
diff --git a/net/core/lock_debug.c b/net/core/lock_debug.c
index 7ecd28cc1c22..506899164f31 100644
--- a/net/core/lock_debug.c
+++ b/net/core/lock_debug.c
@@ -6,10 +6,11 @@
 #include <linux/notifier.h>
 #include <linux/rtnetlink.h>
 #include <net/net_namespace.h>
+#include <net/netdev_lock.h>
 #include <net/netns/generic.h>
 
-static int rtnl_net_debug_event(struct notifier_block *nb,
-				unsigned long event, void *ptr)
+int netdev_debug_event(struct notifier_block *nb, unsigned long event,
+		       void *ptr)
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 	struct net *net = dev_net(dev);
@@ -17,11 +18,13 @@ static int rtnl_net_debug_event(struct notifier_block *nb,
 
 	/* Keep enum and don't add default to trigger -Werror=switch */
 	switch (cmd) {
+	case NETDEV_REGISTER:
 	case NETDEV_UP:
+		netdev_ops_assert_locked(dev);
+		fallthrough;
 	case NETDEV_DOWN:
 	case NETDEV_REBOOT:
 	case NETDEV_CHANGE:
-	case NETDEV_REGISTER:
 	case NETDEV_UNREGISTER:
 	case NETDEV_CHANGEMTU:
 	case NETDEV_CHANGEADDR:
@@ -66,6 +69,7 @@ static int rtnl_net_debug_event(struct notifier_block *nb,
 
 	return NOTIFY_DONE;
 }
+EXPORT_SYMBOL_GPL(netdev_debug_event);
 
 static int rtnl_net_debug_net_id;
 
@@ -74,7 +78,7 @@ static int __net_init rtnl_net_debug_net_init(struct net *net)
 	struct notifier_block *nb;
 
 	nb = net_generic(net, rtnl_net_debug_net_id);
-	nb->notifier_call = rtnl_net_debug_event;
+	nb->notifier_call = netdev_debug_event;
 
 	return register_netdevice_notifier_net(net, nb);
 }
@@ -95,7 +99,7 @@ static struct pernet_operations rtnl_net_debug_net_ops __net_initdata = {
 };
 
 static struct notifier_block rtnl_net_debug_block = {
-	.notifier_call = rtnl_net_debug_event,
+	.notifier_call = netdev_debug_event,
 };
 
 static int __init rtnl_net_debug_init(void)
-- 
2.48.1

[PATCH net v4 06/11] net: dummy: request ops lock

[Stanislav Fomichev]

Even though dummy device doesn't really need an instance lock,
a lot of selftests use dummy so it's useful to have extra
expose to the instance lock on NIPA. Request the instance/ops
locking.

Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 drivers/net/dummy.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c
index a4938c6a5ebb..d6bdad4baadd 100644
--- a/drivers/net/dummy.c
+++ b/drivers/net/dummy.c
@@ -105,6 +105,7 @@ static void dummy_setup(struct net_device *dev)
 	dev->netdev_ops = &dummy_netdev_ops;
 	dev->ethtool_ops = &dummy_ethtool_ops;
 	dev->needs_free_netdev = true;
+	dev->request_ops_lock = true;
 
 	/* Fill in device structure with ethernet-generic values. */
 	dev->flags |= IFF_NOARP;
-- 
2.48.1

[PATCH net v4 07/11] docs: net: document netdev notifier expectations

[Stanislav Fomichev]

We don't have a consistent state yet, but document where we think
we are and where we wanna be.

Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 Documentation/networking/netdevices.rst | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/networking/netdevices.rst b/Documentation/networking/netdevices.rst
index ebb868f50ac2..381243c002c1 100644
--- a/Documentation/networking/netdevices.rst
+++ b/Documentation/networking/netdevices.rst
@@ -343,6 +343,28 @@ there are two sets of interfaces: ``dev_xxx`` and ``netif_xxx`` (e.g.,
 acquiring the instance lock themselves, while the ``netif_xxx`` functions
 assume that the driver has already acquired the instance lock.
 
+Notifiers and netdev instance lock
+==================================
+
+For device drivers that implement shaping or queue management APIs,
+some of the notifiers (``enum netdev_cmd``) are running under the netdev
+instance lock.
+
+For devices with locked ops, currently only the following notifiers are
+running under the lock:
+* ``NETDEV_REGISTER``
+* ``NETDEV_UP``
+
+The following notifiers are running without the lock (so the ops-locked
+devices need to manually grab the lock if needed):
+* ``NETDEV_UNREGISTER``
+
+There are no clear expectations for the remaining notifiers. Notifiers not on
+the list may run with or without the instance lock, potentially even invoking
+the same notifier type with and without the lock from different code paths.
+The goal is to eventually ensure that all (or most, with a few documented
+exceptions) notifiers run under the instance lock.
+
 NETDEV_INTERNAL symbol namespace
 ================================
 
-- 
2.48.1

[PATCH net v4 08/11] selftests: net: use netdevsim in netns test

[Stanislav Fomichev]

Netdevsim has extra register_netdevice_notifier_dev_net notifiers,
use netdevim instead of dummy device to test them out.

Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/net/lib.sh        | 25 +++++++++++++++++++++++
 tools/testing/selftests/net/netns-name.sh | 13 ++++++++----
 2 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
index 975be4fdbcdb..701905eeff66 100644
--- a/tools/testing/selftests/net/lib.sh
+++ b/tools/testing/selftests/net/lib.sh
@@ -222,6 +222,31 @@ setup_ns()
 	NS_LIST+=("${ns_list[@]}")
 }
 
+# Create netdevsim with given id and net namespace.
+create_netdevsim() {
+    local id="$1"
+    local ns="$2"
+
+    modprobe netdevsim &> /dev/null
+    udevadm settle
+
+    echo "$id 1" | ip netns exec $ns tee /sys/bus/netdevsim/new_device >/dev/null
+    local dev=$(ip netns exec $ns ls /sys/bus/netdevsim/devices/netdevsim$id/net)
+    ip -netns $ns link set dev $dev name nsim$id
+    ip -netns $ns link set dev nsim$id up
+
+    echo nsim$id
+}
+
+# Remove netdevsim with given id.
+cleanup_netdevsim() {
+    local id="$1"
+
+    if [ -d "/sys/bus/netdevsim/devices/netdevsim$id/net" ]; then
+        echo "$id" > /sys/bus/netdevsim/del_device
+    fi
+}
+
 tc_rule_stats_get()
 {
 	local dev=$1; shift
diff --git a/tools/testing/selftests/net/netns-name.sh b/tools/testing/selftests/net/netns-name.sh
index 0be1905d1f2f..38871bdef67f 100755
--- a/tools/testing/selftests/net/netns-name.sh
+++ b/tools/testing/selftests/net/netns-name.sh
@@ -7,10 +7,12 @@ set -o pipefail
 DEV=dummy-dev0
 DEV2=dummy-dev1
 ALT_NAME=some-alt-name
+NSIM_ADDR=2025
 
 RET_CODE=0
 
 cleanup() {
+    cleanup_netdevsim $NSIM_ADDR
     cleanup_ns $NS $test_ns
 }
 
@@ -25,12 +27,15 @@ setup_ns NS test_ns
 
 #
 # Test basic move without a rename
+# Use netdevsim because it has extra asserts for notifiers.
 #
-ip -netns $NS link add name $DEV type dummy || fail
-ip -netns $NS link set dev $DEV netns $test_ns ||
+
+nsim=$(create_netdevsim $NSIM_ADDR $NS)
+ip -netns $NS link set dev $nsim netns $test_ns ||
     fail "Can't perform a netns move"
-ip -netns $test_ns link show dev $DEV >> /dev/null || fail "Device not found after move"
-ip -netns $test_ns link del $DEV || fail
+ip -netns $test_ns link show dev $nsim >> /dev/null ||
+    fail "Device not found after move"
+cleanup_netdevsim $NSIM_ADDR
 
 #
 # Test move with a conflict
-- 
2.48.1

[PATCH net v4 09/11] net: designate XSK pool pointers in queues as "ops protected"

[Stanislav Fomichev]

From: Jakub Kicinski <kuba@kernel.org>

Read accesses go via xsk_get_pool_from_qid(), the call coming
from the core and gve look safe (other "ops locked" drivers
don't support XSK).

Write accesses go via xsk_reg_pool_at_qid() and xsk_clear_pool_at_qid().
Former is already under the ops lock, latter needs to be locked when
coming from the workqueue via xp_clear_dev().

Acked-by: Stanislav Fomichev <sdf@fomichev.me>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 include/linux/netdevice.h     | 1 +
 include/net/netdev_rx_queue.h | 6 +++---
 net/xdp/xsk.c                 | 2 ++
 net/xdp/xsk_buff_pool.c       | 7 ++++++-
 4 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index cf3b6445817b..9fb03a292817 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -688,6 +688,7 @@ struct netdev_queue {
 	/* Subordinate device that the queue has been assigned to */
 	struct net_device	*sb_dev;
 #ifdef CONFIG_XDP_SOCKETS
+	/* "ops protected", see comment about net_device::lock */
 	struct xsk_buff_pool    *pool;
 #endif
 
diff --git a/include/net/netdev_rx_queue.h b/include/net/netdev_rx_queue.h
index b2238b551dce..8cdcd138b33f 100644
--- a/include/net/netdev_rx_queue.h
+++ b/include/net/netdev_rx_queue.h
@@ -20,12 +20,12 @@ struct netdev_rx_queue {
 	struct net_device		*dev;
 	netdevice_tracker		dev_tracker;
 
+	/* All fields below are "ops protected",
+	 * see comment about net_device::lock
+	 */
 #ifdef CONFIG_XDP_SOCKETS
 	struct xsk_buff_pool            *pool;
 #endif
-	/* NAPI instance for the queue
-	 * "ops protected", see comment about net_device::lock
-	 */
 	struct napi_struct		*napi;
 	struct pp_memory_provider_params mp_params;
 } ____cacheline_aligned_in_smp;
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index e5d104ce7b82..98a38d21b9b7 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -1651,7 +1651,9 @@ static int xsk_notifier(struct notifier_block *this,
 				xsk_unbind_dev(xs);
 
 				/* Clear device references. */
+				netdev_lock_ops(dev);
 				xp_clear_dev(xs->pool);
+				netdev_unlock_ops(dev);
 			}
 			mutex_unlock(&xs->mutex);
 		}
diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
index 25a76c5ce0f1..c7e50fd86c6a 100644
--- a/net/xdp/xsk_buff_pool.c
+++ b/net/xdp/xsk_buff_pool.c
@@ -279,9 +279,14 @@ static void xp_release_deferred(struct work_struct *work)
 {
 	struct xsk_buff_pool *pool = container_of(work, struct xsk_buff_pool,
 						  work);
+	struct net_device *netdev = pool->netdev;
 
 	rtnl_lock();
-	xp_clear_dev(pool);
+	if (netdev) {
+		netdev_lock_ops(netdev);
+		xp_clear_dev(pool);
+		netdev_unlock_ops(netdev);
+	}
 	rtnl_unlock();
 
 	if (pool->fq) {
-- 
2.48.1

[PATCH net v4 10/11] netdev: add "ops compat locking" helpers

[Stanislav Fomichev]

From: Jakub Kicinski <kuba@kernel.org>

Add helpers to "lock a netdev in a backward-compatible way",
which for ops-locked netdevs will mean take the instance lock.
For drivers which haven't opted into the ops locking we'll take
rtnl_lock.

The scoped foreach is dropping and re-taking the lock for each
device, even if prev and next are both under rtnl_lock.
I hope that's fine since we expect that netdev nl to be mostly
supported by modern drivers, and modern drivers should also
opt into the instance locking.

Note that these helpers are mostly needed for queue related state,
because drivers modify queue config in their ops in a non-atomic
way. Or differently put, queue changes don't have a clear-cut API
like NAPI configuration. Any state that can should just use the
instance lock directly, not the "compat" hacks.

Acked-by: Stanislav Fomichev <sdf@fomichev.me>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 include/net/netdev_lock.h | 16 +++++++++++++
 net/core/dev.c            | 49 +++++++++++++++++++++++++++++++++++++++
 net/core/dev.h            | 15 ++++++++++++
 3 files changed, 80 insertions(+)

diff --git a/include/net/netdev_lock.h b/include/net/netdev_lock.h
index 5f712de5bf8a..8ab108a4e2cf 100644
--- a/include/net/netdev_lock.h
+++ b/include/net/netdev_lock.h
@@ -64,6 +64,22 @@ netdev_ops_assert_locked_or_invisible(const struct net_device *dev)
 		netdev_ops_assert_locked(dev);
 }
 
+static inline void netdev_lock_ops_compat(struct net_device *dev)
+{
+	if (netdev_need_ops_lock(dev))
+		netdev_lock(dev);
+	else
+		rtnl_lock();
+}
+
+static inline void netdev_unlock_ops_compat(struct net_device *dev)
+{
+	if (netdev_need_ops_lock(dev))
+		netdev_unlock(dev);
+	else
+		rtnl_unlock();
+}
+
 static inline int netdev_lock_cmp_fn(const struct lockdep_map *a,
 				     const struct lockdep_map *b)
 {
diff --git a/net/core/dev.c b/net/core/dev.c
index e59eb173900d..87cba93fa59f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1051,6 +1051,18 @@ struct net_device *__netdev_put_lock(struct net_device *dev)
 	return dev;
 }
 
+static struct net_device *__netdev_put_lock_ops_compat(struct net_device *dev)
+{
+	netdev_lock_ops_compat(dev);
+	if (dev->reg_state > NETREG_REGISTERED) {
+		netdev_unlock_ops_compat(dev);
+		dev_put(dev);
+		return NULL;
+	}
+	dev_put(dev);
+	return dev;
+}
+
 /**
  *	netdev_get_by_index_lock() - find a device by its ifindex
  *	@net: the applicable net namespace
@@ -1073,6 +1085,18 @@ struct net_device *netdev_get_by_index_lock(struct net *net, int ifindex)
 	return __netdev_put_lock(dev);
 }
 
+struct net_device *
+netdev_get_by_index_lock_ops_compat(struct net *net, int ifindex)
+{
+	struct net_device *dev;
+
+	dev = dev_get_by_index(net, ifindex);
+	if (!dev)
+		return NULL;
+
+	return __netdev_put_lock_ops_compat(dev);
+}
+
 struct net_device *
 netdev_xa_find_lock(struct net *net, struct net_device *dev,
 		    unsigned long *index)
@@ -1098,6 +1122,31 @@ netdev_xa_find_lock(struct net *net, struct net_device *dev,
 	} while (true);
 }
 
+struct net_device *
+netdev_xa_find_lock_ops_compat(struct net *net, struct net_device *dev,
+			       unsigned long *index)
+{
+	if (dev)
+		netdev_unlock_ops_compat(dev);
+
+	do {
+		rcu_read_lock();
+		dev = xa_find(&net->dev_by_index, index, ULONG_MAX, XA_PRESENT);
+		if (!dev) {
+			rcu_read_unlock();
+			return NULL;
+		}
+		dev_hold(dev);
+		rcu_read_unlock();
+
+		dev = __netdev_put_lock_ops_compat(dev);
+		if (dev)
+			return dev;
+
+		(*index)++;
+	} while (true);
+}
+
 static DEFINE_SEQLOCK(netdev_rename_lock);
 
 void netdev_copy_name(struct net_device *dev, char *name)
diff --git a/net/core/dev.h b/net/core/dev.h
index 7ee203395d8e..c4b645120d72 100644
--- a/net/core/dev.h
+++ b/net/core/dev.h
@@ -41,6 +41,21 @@ DEFINE_FREE(netdev_unlock, struct net_device *, if (_T) netdev_unlock(_T));
 	     (var_name = netdev_xa_find_lock(net, var_name, &ifindex)); \
 	     ifindex++)
 
+struct net_device *
+netdev_get_by_index_lock_ops_compat(struct net *net, int ifindex);
+struct net_device *
+netdev_xa_find_lock_ops_compat(struct net *net, struct net_device *dev,
+			       unsigned long *index);
+
+DEFINE_FREE(netdev_unlock_ops_compat, struct net_device *,
+	    if (_T) netdev_unlock_ops_compat(_T));
+
+#define for_each_netdev_lock_ops_compat_scoped(net, var_name, ifindex)	\
+	for (struct net_device *var_name __free(netdev_unlock_ops_compat) = NULL; \
+	     (var_name = netdev_xa_find_lock_ops_compat(net, var_name,	\
+							&ifindex));	\
+	     ifindex++)
+
 #ifdef CONFIG_PROC_FS
 int __init dev_proc_init(void);
 #else
-- 
2.48.1

[PATCH net v4 11/11] netdev: don't hold rtnl_lock over nl queue info get when possible

[Stanislav Fomichev]

From: Jakub Kicinski <kuba@kernel.org>

Netdev queue dump accesses: NAPI, memory providers, XSk pointers.
All three are "ops protected" now, switch to the op compat locking.
rtnl lock does not have to be taken for "ops locked" devices.

Acked-by: Stanislav Fomichev <sdf@fomichev.me>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 net/core/netdev-genl.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index fd1cfa9707dc..39f52a311f07 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -481,18 +481,15 @@ int netdev_nl_queue_get_doit(struct sk_buff *skb, struct genl_info *info)
 	if (!rsp)
 		return -ENOMEM;
 
-	rtnl_lock();
-
-	netdev = netdev_get_by_index_lock(genl_info_net(info), ifindex);
+	netdev = netdev_get_by_index_lock_ops_compat(genl_info_net(info),
+						     ifindex);
 	if (netdev) {
 		err = netdev_nl_queue_fill(rsp, netdev, q_id, q_type, info);
-		netdev_unlock(netdev);
+		netdev_unlock_ops_compat(netdev);
 	} else {
 		err = -ENODEV;
 	}
 
-	rtnl_unlock();
-
 	if (err)
 		goto err_free_msg;
 
@@ -541,17 +538,17 @@ int netdev_nl_queue_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 	if (info->attrs[NETDEV_A_QUEUE_IFINDEX])
 		ifindex = nla_get_u32(info->attrs[NETDEV_A_QUEUE_IFINDEX]);
 
-	rtnl_lock();
 	if (ifindex) {
-		netdev = netdev_get_by_index_lock(net, ifindex);
+		netdev = netdev_get_by_index_lock_ops_compat(net, ifindex);
 		if (netdev) {
 			err = netdev_nl_queue_dump_one(netdev, skb, info, ctx);
-			netdev_unlock(netdev);
+			netdev_unlock_ops_compat(netdev);
 		} else {
 			err = -ENODEV;
 		}
 	} else {
-		for_each_netdev_lock_scoped(net, netdev, ctx->ifindex) {
+		for_each_netdev_lock_ops_compat_scoped(net, netdev,
+						       ctx->ifindex) {
 			err = netdev_nl_queue_dump_one(netdev, skb, info, ctx);
 			if (err < 0)
 				break;
@@ -559,7 +556,6 @@ int netdev_nl_queue_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 			ctx->txq_idx = 0;
 		}
 	}
-	rtnl_unlock();
 
 	return err;
 }
-- 
2.48.1

Re: [PATCH net v4 01/11] net: switch to netif_disable_lro in inetdev_init

[Jakub Kicinski]

On Mon, 31 Mar 2025 08:05:53 -0700 Stanislav Fomichev wrote:
> +EXPORT_IPV6_MOD(netif_disable_lro);

> +++ b/net/ipv4/devinet.c

>  	if (IPV4_DEVCONF(in_dev->cnf, FORWARDING))
> -		dev_disable_lro(dev);
> +		netif_disable_lro(dev);
>  	/* Reference in_dev->dev */
>  	netdev_hold(dev, &in_dev->dev_tracker, GFP_KERNEL);
>  	/* Account for reference dev->ip_ptr (below) */

I still don't see a way for devinet.c to be built into a module.
The export and moving the defines needs to go to patch 3.

Re: [PATCH net v4 02/11] net: hold instance lock during NETDEV_REGISTER/UP

[Jakub Kicinski]

On Mon, 31 Mar 2025 08:05:54 -0700 Stanislav Fomichev wrote:
> Callers of inetdev_init can come from several places with inconsistent
> expectation about netdev instance lock. Grab instance lock during
> REGISTER (plus UP). Also solve the inconsistency with UNREGISTER
> where it was locked only during move netns path.

Couple of nits, with that:

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

> diff --git a/net/core/dev_api.c b/net/core/dev_api.c
> index 8dbc60612100..cb3e5807dce8 100644
> --- a/net/core/dev_api.c
> +++ b/net/core/dev_api.c
> @@ -119,9 +119,7 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net,
>  {
>  	int ret;
>  
> -	netdev_lock_ops(dev);
> -	ret = netif_change_net_namespace(dev, net, pat, 0, NULL);
> -	netdev_unlock_ops(dev);
> +	ret = __dev_change_net_namespace(dev, net, pat, 0, NULL);
>  
>  	return ret;
>  }

nit: no need for the temp variable for ret, now

> @@ -3042,14 +3040,16 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
>  
>  		new_ifindex = nla_get_s32_default(tb[IFLA_NEW_IFINDEX], 0);
>  
> -		err = netif_change_net_namespace(dev, tgt_net, pat,
> -						 new_ifindex, extack);
> +		err = __dev_change_net_namespace(dev, tgt_net, pat, new_ifindex,

nit: over 80 chars now

> +						 extack);
>  		if (err)
> -			goto errout;
> +			return err;

Re: [PATCH net v4 04/11] net: rename rtnl_net_debug to lock_debug

[Jakub Kicinski]

On Mon, 31 Mar 2025 08:05:56 -0700 Stanislav Fomichev wrote:
> And make it selected by CONFIG_DEBUG_NET. Don't rename any of
> the structs/functions. Next patch will use rtnl_net_debug_event in
> netdevsim.

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

Re: [PATCH net v4 05/11] netdevsim: add dummy device notifiers

[Jakub Kicinski]

On Mon, 31 Mar 2025 08:05:57 -0700 Stanislav Fomichev wrote:
> +#if IS_ENABLED(CONFIG_DEBUG_NET)
> +int netdev_debug_event(struct notifier_block *nb, unsigned long event,
> +		       void *ptr);
> +#else
> +static inline int netdev_debug_event(struct notifier_block *nb,
> +				     unsigned long event, void *ptr)
> +{
> +	return 0;
> +}
> +#endif

Maybe we can wrap the while notifier setup in

	if (IS_ENABLED(CONFIG_DEBUG_NET)) {

instead? We don't expect more users of the event callback, and it may
be useful to give readers of the netdevsim code a hint that this
callback will only do something when DEBUG_NET=y

>  #endif
> diff --git a/net/core/lock_debug.c b/net/core/lock_debug.c
> index 7ecd28cc1c22..506899164f31 100644
> --- a/net/core/lock_debug.c
> +++ b/net/core/lock_debug.c

> @@ -66,6 +69,7 @@ static int rtnl_net_debug_event(struct notifier_block *nb,
>  
>  	return NOTIFY_DONE;
>  }
> +EXPORT_SYMBOL_GPL(netdev_debug_event);

EXPORT_SYMBOL_NS_GPL(netdev_debug_event, "NETDEV_INTERNAL");

Re: [PATCH net v4 07/11] docs: net: document netdev notifier expectations

[Jakub Kicinski]

On Mon, 31 Mar 2025 08:05:59 -0700 Stanislav Fomichev wrote:
> +The following notifiers are running without the lock (so the ops-locked
> +devices need to manually grab the lock if needed):

Not sure about the text in the parenthesis, "the devices" don't "grab
the lock". I mean - drivers don't generally register for notifications
about their own devices. It's whoever registered the notifier that needs
to make sure they take appropriate locks. I think we're fine without
that sentence.

> +* ``NETDEV_UNREGISTER``
> +
> +There are no clear expectations for the remaining notifiers. Notifiers not on
> +the list may run with or without the instance lock, potentially even invoking
> +the same notifier type with and without the lock from different code paths.
> +The goal is to eventually ensure that all (or most, with a few documented
> +exceptions) notifiers run under the instance lock.

Should we add a sentence here along the lines of "Please extend this
documentation whenever you make explicit assumption about lock being
held from a notifier." or is that obvious?

Re: [PATCH net v4 01/11] net: switch to netif_disable_lro in inetdev_init

[Stanislav Fomichev]

On 03/31, Jakub Kicinski wrote:
> On Mon, 31 Mar 2025 08:05:53 -0700 Stanislav Fomichev wrote:
> > +EXPORT_IPV6_MOD(netif_disable_lro);
> 
> > +++ b/net/ipv4/devinet.c
> 
> >  	if (IPV4_DEVCONF(in_dev->cnf, FORWARDING))
> > -		dev_disable_lro(dev);
> > +		netif_disable_lro(dev);
> >  	/* Reference in_dev->dev */
> >  	netdev_hold(dev, &in_dev->dev_tracker, GFP_KERNEL);
> >  	/* Account for reference dev->ip_ptr (below) */
> 
> I still don't see a way for devinet.c to be built into a module.
> The export and moving the defines needs to go to patch 3.

Ah, I missed that, you're right, will do!

Re: [PATCH net v4 02/11] net: hold instance lock during NETDEV_REGISTER/UP

[Stanislav Fomichev]

On 03/31, Jakub Kicinski wrote:
> On Mon, 31 Mar 2025 08:05:54 -0700 Stanislav Fomichev wrote:
> > Callers of inetdev_init can come from several places with inconsistent
> > expectation about netdev instance lock. Grab instance lock during
> > REGISTER (plus UP). Also solve the inconsistency with UNREGISTER
> > where it was locked only during move netns path.
> 
> Couple of nits, with that:
> 
> Reviewed-by: Jakub Kicinski <kuba@kernel.org>
> 
> > diff --git a/net/core/dev_api.c b/net/core/dev_api.c
> > index 8dbc60612100..cb3e5807dce8 100644
> > --- a/net/core/dev_api.c
> > +++ b/net/core/dev_api.c
> > @@ -119,9 +119,7 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net,
> >  {
> >  	int ret;
> >  
> > -	netdev_lock_ops(dev);
> > -	ret = netif_change_net_namespace(dev, net, pat, 0, NULL);
> > -	netdev_unlock_ops(dev);
> > +	ret = __dev_change_net_namespace(dev, net, pat, 0, NULL);
> >  
> >  	return ret;
> >  }
> 
> nit: no need for the temp variable for ret, now
> 
> > @@ -3042,14 +3040,16 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
> >  
> >  		new_ifindex = nla_get_s32_default(tb[IFLA_NEW_IFINDEX], 0);
> >  
> > -		err = netif_change_net_namespace(dev, tgt_net, pat,
> > -						 new_ifindex, extack);
> > +		err = __dev_change_net_namespace(dev, tgt_net, pat, new_ifindex,
> 
> nit: over 80 chars now

It's exactly 80, is it considered over? This has been done by clang
formatter which has 'ColumnLimit: 80'.. Will undo regardless, but lmk
if the rule is >80 or >=80 (the formatter thinks it's the former)

Re: [PATCH net v4 05/11] netdevsim: add dummy device notifiers

[Stanislav Fomichev]

On 03/31, Jakub Kicinski wrote:
> On Mon, 31 Mar 2025 08:05:57 -0700 Stanislav Fomichev wrote:
> > +#if IS_ENABLED(CONFIG_DEBUG_NET)
> > +int netdev_debug_event(struct notifier_block *nb, unsigned long event,
> > +		       void *ptr);
> > +#else
> > +static inline int netdev_debug_event(struct notifier_block *nb,
> > +				     unsigned long event, void *ptr)
> > +{
> > +	return 0;
> > +}
> > +#endif
> 
> Maybe we can wrap the while notifier setup in
> 
> 	if (IS_ENABLED(CONFIG_DEBUG_NET)) {
> 
> instead? We don't expect more users of the event callback, and it may
> be useful to give readers of the netdevsim code a hint that this
> callback will only do something when DEBUG_NET=y

Will do. I'm not sure if (IS_ENABLED()) will compile, will double-check;
worst case will wrap into #ifdef
 
> >  #endif
> > diff --git a/net/core/lock_debug.c b/net/core/lock_debug.c
> > index 7ecd28cc1c22..506899164f31 100644
> > --- a/net/core/lock_debug.c
> > +++ b/net/core/lock_debug.c
> 
> > @@ -66,6 +69,7 @@ static int rtnl_net_debug_event(struct notifier_block *nb,
> >  
> >  	return NOTIFY_DONE;
> >  }
> > +EXPORT_SYMBOL_GPL(netdev_debug_event);
> 
> EXPORT_SYMBOL_NS_GPL(netdev_debug_event, "NETDEV_INTERNAL");

Ah, good, and it's already imported by netdevsim. I was toying with making
this export dependent on netdevsim=ym...

Re: [PATCH net v4 02/11] net: hold instance lock during NETDEV_REGISTER/UP

[Jakub Kicinski]

On Mon, 31 Mar 2025 14:13:09 -0700 Stanislav Fomichev wrote:
> > > @@ -3042,14 +3040,16 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
> > >  
> > >  		new_ifindex = nla_get_s32_default(tb[IFLA_NEW_IFINDEX], 0);
> > >  
> > > -		err = netif_change_net_namespace(dev, tgt_net, pat,
> > > -						 new_ifindex, extack);
> > > +		err = __dev_change_net_namespace(dev, tgt_net, pat, new_ifindex,  
> > 
> > nit: over 80 chars now  
> 
> It's exactly 80, is it considered over? This has been done by clang
> formatter which has 'ColumnLimit: 80'.. Will undo regardless, but lmk
> if the rule is >80 or >=80 (the formatter thinks it's the former)

My bad, unplugging the external screen seems to slightly shrink windows
in GNOME. My terminal was set to 79 chars :S

Re: [PATCH net v4 07/11] docs: net: document netdev notifier expectations

[Stanislav Fomichev]

On 03/31, Jakub Kicinski wrote:
> On Mon, 31 Mar 2025 08:05:59 -0700 Stanislav Fomichev wrote:
> > +The following notifiers are running without the lock (so the ops-locked
> > +devices need to manually grab the lock if needed):
> 
> Not sure about the text in the parenthesis, "the devices" don't "grab
> the lock". I mean - drivers don't generally register for notifications
> about their own devices. It's whoever registered the notifier that needs
> to make sure they take appropriate locks. I think we're fine without
> that sentence.

Good point, I was mostly referring to dev_ vs netif_ calls for managing
lower devices, will drop the sentence.

> > +* ``NETDEV_UNREGISTER``
> > +
> > +There are no clear expectations for the remaining notifiers. Notifiers not on
> > +the list may run with or without the instance lock, potentially even invoking
> > +the same notifier type with and without the lock from different code paths.
> > +The goal is to eventually ensure that all (or most, with a few documented
> > +exceptions) notifiers run under the instance lock.
> 
> Should we add a sentence here along the lines of "Please extend this
> documentation whenever you make explicit assumption about lock being
> held from a notifier." or is that obvious?

Yes, that was the assumption, but let's explicitly state that, shouldn't
hurt.