Fix RCU stall detected in kernel 6.1 when unregistering devlink (sonic-net#437)

Resolves: sonic-net#20368

Ref: https://lore.kernel.org/stable/[email protected]/t/#u

> But first of, why? Why not just take the upstrema commits instead?
> 
> thanks,
> greg k-h

Ido:

> There were a lot of changes as part of the 6.3 cycle to completely
> rework the semantics of the devlink instance reference count. As part of
> these changes, commit d77278196441 ("devlink: bump the instance index
> directly when iterating") inadvertently fixed the bug mentioned in this
> patch. This commit cannot be applied to 6.1.y as-is because a prior
> commit (also in 6.3) moved the code to a different file (leftover.c ->
> core.c). There might be more dependencies that I'm currently unaware of.
> 
> The alternative, proposed in this patch, is to provide a minimal and
> contained fix for the bug introduced in upstream commit c2368b19807a
> ("net: devlink: introduce "unregistering" mark and use it during
> devlinks iteration") as part of the 6.0 cycle.

Signed-off-by: Vivek Reddy <[email protected]>

Author: vivekrnv

patch/0001-devlink-Fix-RCU-stall-when-unregistering-a-devlink-i.patch

@@ -0,0 +1,91 @@
+From c1bf9b90ce85dd46f958b9f0e60143d58f885963 Mon Sep 17 00:00:00 2001
+From: Ido Schimmel <[email protected]>
+Date: Tue, 1 Oct 2024 14:20:35 +0300
+Subject: [PATCH] devlink: Fix RCU stall when unregistering a devlink instance
+
+When a devlink instance is unregistered the following happens (among
+other things):
+
+t0 - The instance is marked with 'DEVLINK_UNREGISTERING'.
+t1 - Blocking until an RCU grace period passes.
+t2 - The 'DEVLINK_UNREGISTERING' mark is cleared from the instance.
+
+When iterating over devlink instances (f.e., when requesting a dump of
+available instances) and encountering an instance that is currently
+being unregistered, the current code will loop around until the
+'DEVLINK_UNREGISTERING' mark is cleared.
+
+The iteration over devlink instances happens in an RCU critical section,
+so if the instance that is currently being unregistered was encountered
+between t0 and t1, the system will deadlock and RCU stalls will be
+reported [1]. The task unregistering the instance will forever wait for an
+RCU grace period to pass and the task iterating over the instances will
+forever wait for the mark to be cleared.
+
+The issue can be reliably reproduced by increasing the time window
+between t0 and t1 (used a 60 seconds sleep) and running the following
+reproducer [2].
+
+Fix by skipping over instances that are currently being unregistered.
+
+[1]
+rcu: INFO: rcu_preempt detected stalls on CPUs/tasks:
+rcu:     Tasks blocked on level-0 rcu_node (CPUs 0-7): P344
+ (detected by 4, t=26002 jiffies, g=5773, q=12 ncpus=8)
+task:devlink         state:R  running task     stack:25568 pid:344   ppid:260    flags:0x00004002
+[...]
+Call Trace:
+ xa_get_mark+0x184/0x3e0
+ devlinks_xa_find_get.constprop.0+0xc6/0x2e0
+ devlink_nl_cmd_get_dumpit+0x105/0x3f0
+ netlink_dump+0x568/0xff0
+ __netlink_dump_start+0x651/0x900
+ genl_family_rcv_msg_dumpit+0x201/0x340
+ genl_rcv_msg+0x573/0x780
+ netlink_rcv_skb+0x15f/0x430
+ genl_rcv+0x29/0x40
+ netlink_unicast+0x546/0x800
+ netlink_sendmsg+0x958/0xe60
+ __sys_sendto+0x3a2/0x480
+ __x64_sys_sendto+0xe1/0x1b0
+ do_syscall_64+0x35/0x80
+ entry_SYSCALL_64_after_hwframe+0x68/0xd2
+
+[2]
+ # echo 10 > /sys/bus/netdevsim/new_device
+ # echo 10 > /sys/bus/netdevsim/del_device &
+ # devlink dev
+
+Fixes: c2368b19807a ("net: devlink: introduce "unregistering" mark and use it during devlinks iteration")
+Reported-by: Vivek Reddy Karri <[email protected]>
+Signed-off-by: Ido Schimmel <[email protected]>
+---
+ net/devlink/leftover.c | 5 +++--
+ 1 file changed, 3 insertions(+), 2 deletions(-)
+
+diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
+index 032c7af06..c6f781a08 100644
+--- a/net/devlink/leftover.c
++++ b/net/devlink/leftover.c
+@@ -301,6 +301,9 @@ devlinks_xa_find_get(struct net *net, unsigned long *indexp, xa_mark_t filter,
+ 	if (!devlink)
+ 		goto unlock;
+ 
++	/* For a possible retry, the xa_find_after() should be always used */
++	xa_find_fn = xa_find_after;
++
+ 	/* In case devlink_unregister() was already called and "unregistering"
+ 	 * mark was set, do not allow to get a devlink reference here.
+ 	 * This prevents live-lock of devlink_unregister() wait for completion.
+@@ -308,8 +311,6 @@ devlinks_xa_find_get(struct net *net, unsigned long *indexp, xa_mark_t filter,
+ 	if (xa_get_mark(&devlinks, *indexp, DEVLINK_UNREGISTERING))
+ 		goto retry;
+ 
+-	/* For a possible retry, the xa_find_after() should be always used */
+-	xa_find_fn = xa_find_after;
+ 	if (!devlink_try_get(devlink))
+ 		goto retry;
+ 	if (!net_eq(devlink_net(devlink), net)) {
+-- 
+2.43.2
+

patch/series

@@ -68,6 +68,8 @@ Support-for-fullcone-nat.patch
 # Tbridge-per-port-multicast-broadcast-flood-flags.patch
 #
 
+0001-devlink-Fix-RCU-stall-when-unregistering-a-devlink-i.patch
+
 # Mellanox patches for 5.10
 ###-> mellanox_sdk-start
 0001-psample-define-the-macro-PSAMPLE_MD_EXTENDED_ATTR.patch