Re: another netdev instance lock bug in ipv6_add_dev

[Stanislav Fomichev <stfomichev@gmail.com>]

On 04/03, Cosmin Ratiu wrote:
> On Wed, 2025-04-02 at 16:20 -0700, Stanislav Fomichev wrote:
> > On 04/02, Cosmin Ratiu wrote:
> > > Hi,
> > > 
> > > Not sure if it's reported already, but I encountered a bug while
> > > testing with the new locking scheme.
> > > This is the call trace:
> > > 
> > > [ 3454.975672] WARNING: CPU: 1 PID: 58237 at
> > > ./include/net/netdev_lock.h:54 ipv6_add_dev+0x370/0x620
> > > [ 3455.008776]  ? ipv6_add_dev+0x370/0x620
> > > [ 3455.010097]  ipv6_find_idev+0x96/0xe0
> > > [ 3455.010725]  addrconf_add_dev+0x1e/0xa0
> > > [ 3455.011382]  addrconf_init_auto_addrs+0xb0/0x720
> > > [ 3455.013537]  addrconf_notify+0x35f/0x8d0
> > > [ 3455.014214]  notifier_call_chain+0x38/0xf0
> > > [ 3455.014903]  netdev_state_change+0x65/0x90
> > > [ 3455.015586]  linkwatch_do_dev+0x5a/0x70
> > > [ 3455.016238]  rtnl_getlink+0x241/0x3e0
> > > [ 3455.019046]  rtnetlink_rcv_msg+0x177/0x5e0
> > > 
> > > The call chain is rtnl_getlink -> linkwatch_sync_dev ->
> > > linkwatch_do_dev -> netdev_state_change -> ...
> > > 
> > > Nothing on this path acquires the netdev lock, resulting in a
> > > warning.
> > > Perhaps rtnl_getlink should acquire it, in addition to the RTNL
> > > already
> > > held by rtnetlink_rcv_msg?
> > > 
> > > The same thing can be seen from the regular linkwatch wq:
> > > 
> > > [ 3456.637014] WARNING: CPU: 16 PID: 83257 at
> > > ./include/net/netdev_lock.h:54 ipv6_add_dev+0x370/0x620
> > > [ 3456.655305] Call Trace:
> > > [ 3456.655610]  <TASK>
> > > [ 3456.655890]  ? __warn+0x89/0x1b0
> > > [ 3456.656261]  ? ipv6_add_dev+0x370/0x620
> > > [ 3456.660039]  ipv6_find_idev+0x96/0xe0
> > > [ 3456.660445]  addrconf_add_dev+0x1e/0xa0
> > > [ 3456.660861]  addrconf_init_auto_addrs+0xb0/0x720
> > > [ 3456.661803]  addrconf_notify+0x35f/0x8d0
> > > [ 3456.662236]  notifier_call_chain+0x38/0xf0
> > > [ 3456.662676]  netdev_state_change+0x65/0x90
> > > [ 3456.663112]  linkwatch_do_dev+0x5a/0x70
> > > [ 3456.663529]  __linkwatch_run_queue+0xeb/0x200
> > > [ 3456.663990]  linkwatch_event+0x21/0x30
> > > [ 3456.664399]  process_one_work+0x211/0x610
> > > [ 3456.664828]  worker_thread+0x1cc/0x380
> > > [ 3456.665691]  kthread+0xf4/0x210
> > > 
> > > In this case, __linkwatch_run_queue seems like a good place to grab
> > > a
> > > device lock before calling linkwatch_do_dev.
> > 
> > Thanks for the report! What about linkwatch_sync_dev in
> > netdev_run_todo
> > and carrier_show? Should probably also need to be wrapped?
> 
> Done, here's the patch I'm testing with which works for all tests I
> could get my hands on. Will you officially propose it (maybe in a
> slightly different form) please?

I'm thinking maybe we should push down the locking a bit? To the
level of netdev_state_change. Since, in theory, every NETDEV_CHANGE
can reach addrconf_notify. I was playing with the patch below,
but I think the ethtool will lockup, so I need to fix that at least...
Let me spend a bit more time today chasing down the callers...

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 9fb03a292817..f5a322b63e37 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4975,6 +4975,7 @@ void dev_set_rx_mode(struct net_device *dev);
 int dev_set_promiscuity(struct net_device *dev, int inc);
 int netif_set_allmulti(struct net_device *dev, int inc, bool notify);
 int dev_set_allmulti(struct net_device *dev, int inc);
+void netif_state_change(struct net_device *dev);
 void netdev_state_change(struct net_device *dev);
 void __netdev_notify_peers(struct net_device *dev);
 void netdev_notify_peers(struct net_device *dev);
diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
index ccaaf4c7d5f6..ea39dd23a197 100644
--- a/include/linux/rtnetlink.h
+++ b/include/linux/rtnetlink.h
@@ -240,6 +240,6 @@ rtnl_notify_needed(const struct net *net, u16 nlflags, u32 group)
 	return (nlflags & NLM_F_ECHO) || rtnl_has_listeners(net, group);
 }
 
-void netdev_set_operstate(struct net_device *dev, int newstate);
+void netif_set_operstate(struct net_device *dev, int newstate);
 
 #endif	/* __LINUX_RTNETLINK_H */
diff --git a/net/core/dev.c b/net/core/dev.c
index 87cba93fa59f..d4a5c07a0d73 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1567,15 +1567,7 @@ void netdev_features_change(struct net_device *dev)
 }
 EXPORT_SYMBOL(netdev_features_change);
 
-/**
- *	netdev_state_change - device changes state
- *	@dev: device to cause notification
- *
- *	Called to indicate a device has changed state. This function calls
- *	the notifier chains for netdev_chain and sends a NEWLINK message
- *	to the routing socket.
- */
-void netdev_state_change(struct net_device *dev)
+void netif_state_change(struct net_device *dev)
 {
 	if (dev->flags & IFF_UP) {
 		struct netdev_notifier_change_info change_info = {
@@ -1587,7 +1579,6 @@ void netdev_state_change(struct net_device *dev)
 		rtmsg_ifinfo(RTM_NEWLINK, dev, 0, GFP_KERNEL, 0, NULL);
 	}
 }
-EXPORT_SYMBOL(netdev_state_change);
 
 /**
  * __netdev_notify_peers - notify network peers about existence of @dev,
diff --git a/net/core/dev_api.c b/net/core/dev_api.c
index 90bafb0b1b8c..6c6ca15ef2a3 100644
--- a/net/core/dev_api.c
+++ b/net/core/dev_api.c
@@ -327,3 +327,19 @@ int dev_xdp_propagate(struct net_device *dev, struct netdev_bpf *bpf)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(dev_xdp_propagate);
+
+/**
+ *	netdev_state_change - device changes state
+ *	@dev: device to cause notification
+ *
+ *	Called to indicate a device has changed state. This function calls
+ *	the notifier chains for netdev_chain and sends a NEWLINK message
+ *	to the routing socket.
+ */
+void netdev_state_change(struct net_device *dev)
+{
+	netdev_lock_ops(dev);
+	netif_state_change(dev);
+	netdev_unlock_ops(dev);
+}
+EXPORT_SYMBOL(netdev_state_change);
diff --git a/net/core/lock_debug.c b/net/core/lock_debug.c
index 72e522a68775..c442bf52dbaf 100644
--- a/net/core/lock_debug.c
+++ b/net/core/lock_debug.c
@@ -20,11 +20,11 @@ int netdev_debug_event(struct notifier_block *nb, unsigned long event,
 	switch (cmd) {
 	case NETDEV_REGISTER:
 	case NETDEV_UP:
+	case NETDEV_CHANGE:
 		netdev_ops_assert_locked(dev);
 		fallthrough;
 	case NETDEV_DOWN:
 	case NETDEV_REBOOT:
-	case NETDEV_CHANGE:
 	case NETDEV_UNREGISTER:
 	case NETDEV_CHANGEMTU:
 	case NETDEV_CHANGEADDR:
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index c23852835050..d8d03ff87a3b 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1043,7 +1043,7 @@ int rtnl_put_cacheinfo(struct sk_buff *skb, struct dst_entry *dst, u32 id,
 }
 EXPORT_SYMBOL_GPL(rtnl_put_cacheinfo);
 
-void netdev_set_operstate(struct net_device *dev, int newstate)
+void netif_set_operstate(struct net_device *dev, int newstate)
 {
 	unsigned int old = READ_ONCE(dev->operstate);
 
@@ -1052,9 +1052,9 @@ void netdev_set_operstate(struct net_device *dev, int newstate)
 			return;
 	} while (!try_cmpxchg(&dev->operstate, &old, newstate));
 
-	netdev_state_change(dev);
+	netif_state_change(dev);
 }
-EXPORT_SYMBOL(netdev_set_operstate);
+EXPORT_SYMBOL(netif_set_operstate);
 
 static void set_operstate(struct net_device *dev, unsigned char transition)
 {
@@ -1080,7 +1080,7 @@ static void set_operstate(struct net_device *dev, unsigned char transition)
 		break;
 	}
 
-	netdev_set_operstate(dev, operstate);
+	netif_set_operstate(dev, operstate);
 }
 
 static unsigned int rtnl_dev_get_flags(const struct net_device *dev)
@@ -3396,7 +3396,7 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
 errout:
 	if (status & DO_SETLINK_MODIFIED) {
 		if ((status & DO_SETLINK_NOTIFY) == DO_SETLINK_NOTIFY)
-			netdev_state_change(dev);
+			netif_state_change(dev);
 
 		if (err < 0)
 			net_warn_ratelimited("A link change request failed with some changes committed already. Interface %s may have been left with an inconsistent configuration, please check.\n",
@@ -3676,8 +3676,11 @@ struct net_device *rtnl_create_link(struct net *net, const char *ifname,
 				nla_len(tb[IFLA_BROADCAST]));
 	if (tb[IFLA_TXQLEN])
 		dev->tx_queue_len = nla_get_u32(tb[IFLA_TXQLEN]);
-	if (tb[IFLA_OPERSTATE])
+	if (tb[IFLA_OPERSTATE]) {
+		netdev_lock_ops(dev);
 		set_operstate(dev, nla_get_u8(tb[IFLA_OPERSTATE]));
+		netdev_unlock_ops(dev);
+	}
 	if (tb[IFLA_LINKMODE])
 		dev->link_mode = nla_get_u8(tb[IFLA_LINKMODE]);
 	if (tb[IFLA_GROUP])
diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index 439cfb7ad5d1..1b1b700ec05e 100644
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c
@@ -33,14 +33,14 @@ static void hsr_set_operstate(struct hsr_port *master, bool has_carrier)
 	struct net_device *dev = master->dev;
 
 	if (!is_admin_up(dev)) {
-		netdev_set_operstate(dev, IF_OPER_DOWN);
+		netif_set_operstate(dev, IF_OPER_DOWN);
 		return;
 	}
 
 	if (has_carrier)
-		netdev_set_operstate(dev, IF_OPER_UP);
+		netif_set_operstate(dev, IF_OPER_UP);
 	else
-		netdev_set_operstate(dev, IF_OPER_LOWERLAYERDOWN);
+		netif_set_operstate(dev, IF_OPER_LOWERLAYERDOWN);
 }
 
 static bool hsr_check_carrier(struct hsr_port *master)