[PATCH stable 6.1] devlink: Fix RCU stall when unregistering a devlink instance

[PATCH stable 6.1] devlink: Fix RCU stall when unregistering a devlink instance

[Ido Schimmel]

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 <vkarri@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
I read the stable rules and I am not providing an "upstream commit ID"
since the code in upstream has been reworked, making this fix
irrelevant. The only affected stable kernel is 6.1.y.
---
 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 032c7af065cd..c6f781a08d06 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.46.1

Re: [PATCH stable 6.1] devlink: Fix RCU stall when unregistering a devlink instance

[Greg KH]

On Tue, Oct 01, 2024 at 02:20:35PM +0300, Ido Schimmel wrote:
> 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 <vkarri@nvidia.com>
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
> I read the stable rules and I am not providing an "upstream commit ID"
> since the code in upstream has been reworked, making this fix
> irrelevant. The only affected stable kernel is 6.1.y.

You need to document the heck out of why this is only relevant for this
one specific kernel branch IN the changelog text, so that we understand
what is going on, AND you need to get acks from the relevant maintainers
of this area of the kernel to accept something that is not in Linus's
tree.

But first of, why?  Why not just take the upstrema commits instead?

thanks,

greg k-h

Re: [PATCH stable 6.1] devlink: Fix RCU stall when unregistering a devlink instance

[Ido Schimmel]

On Tue, Oct 01, 2024 at 02:11:27PM +0200, Greg KH wrote:
> On Tue, Oct 01, 2024 at 02:20:35PM +0300, Ido Schimmel wrote:
> > I read the stable rules and I am not providing an "upstream commit ID"
> > since the code in upstream has been reworked, making this fix
> > irrelevant. The only affected stable kernel is 6.1.y.
> 
> You need to document the heck out of why this is only relevant for this
> one specific kernel branch IN the changelog text, so that we understand
> what is going on, AND you need to get acks from the relevant maintainers
> of this area of the kernel to accept something that is not in Linus's
> tree.
> 
> But first of, why?  Why not just take the upstrema commits instead?

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.

The above explains why the patch is only relevant to 6.1.y.

Jakub / Jiri, what is your preference here? This patch or cherry picking
a lot of code from 6.3?

Re: Re: [PATCH stable 6.1] devlink: Fix RCU stall when unregistering a devlink instance

[Aleksandr Nogikh]

Hi Ido,

On Tue, 1 Oct 2024 16:38:39 +0300, Ido Schimmel wrote:
> 
> On Tue, Oct 01, 2024 at 02:11:27PM +0200, Greg KH wrote:
> > On Tue, Oct 01, 2024 at 02:20:35PM +0300, Ido Schimmel wrote:
> > > I read the stable rules and I am not providing an "upstream commit ID"
> > > since the code in upstream has been reworked, making this fix
> > > irrelevant. The only affected stable kernel is 6.1.y.
> > 
> > You need to document the heck out of why this is only relevant for this
> > one specific kernel branch IN the changelog text, so that we understand
> > what is going on, AND you need to get acks from the relevant maintainers
> > of this area of the kernel to accept something that is not in Linus's
> > tree.
> > 
> > But first of, why?  Why not just take the upstrema commits instead?
> 
> 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.
> 
> The above explains why the patch is only relevant to 6.1.y.

Thanks for bringing up this topic!

For what it's worth, syzbot would also greatly benefit from your fix:
https://github.com/google/syzkaller/issues/5328

I've built a kernel locally with your changes, run syzkaller against it,
and I can confirm that the kernel no longer crashes due to devlink.

-- 
Aleksandr

> 
> Jakub / Jiri, what is your preference here? This patch or cherry picking
> a lot of code from 6.3?

Re: Re: [PATCH stable 6.1] devlink: Fix RCU stall when unregistering a devlink instance

[Ido Schimmel]

On Tue, Oct 01, 2024 at 06:47:59PM +0200, Aleksandr Nogikh wrote:
> Hi Ido,
> 
> On Tue, 1 Oct 2024 16:38:39 +0300, Ido Schimmel wrote:
> > 
> > On Tue, Oct 01, 2024 at 02:11:27PM +0200, Greg KH wrote:
> > > On Tue, Oct 01, 2024 at 02:20:35PM +0300, Ido Schimmel wrote:
> > > > I read the stable rules and I am not providing an "upstream commit ID"
> > > > since the code in upstream has been reworked, making this fix
> > > > irrelevant. The only affected stable kernel is 6.1.y.
> > > 
> > > You need to document the heck out of why this is only relevant for this
> > > one specific kernel branch IN the changelog text, so that we understand
> > > what is going on, AND you need to get acks from the relevant maintainers
> > > of this area of the kernel to accept something that is not in Linus's
> > > tree.
> > > 
> > > But first of, why?  Why not just take the upstrema commits instead?
> > 
> > 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.
> > 
> > The above explains why the patch is only relevant to 6.1.y.
> 
> Thanks for bringing up this topic!
> 
> For what it's worth, syzbot would also greatly benefit from your fix:
> https://github.com/google/syzkaller/issues/5328
> 
> I've built a kernel locally with your changes, run syzkaller against it,
> and I can confirm that the kernel no longer crashes due to devlink.

Good to know :)

I hope this patch can be accepted as-is instead of a much larger patch.
Will copy you on the next version.

Thanks for testing!

Re: [PATCH stable 6.1] devlink: Fix RCU stall when unregistering a devlink instance

[Jakub Kicinski]

On Tue, 1 Oct 2024 16:38:39 +0300 Ido Schimmel wrote:
> > You need to document the heck out of why this is only relevant for this
> > one specific kernel branch IN the changelog text, so that we understand
> > what is going on, AND you need to get acks from the relevant maintainers
> > of this area of the kernel to accept something that is not in Linus's
> > tree.
> > 
> > But first of, why?  Why not just take the upstrema commits instead?  
> 
> 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.
> 
> The above explains why the patch is only relevant to 6.1.y.
> 
> Jakub / Jiri, what is your preference here? This patch or cherry picking
> a lot of code from 6.3?

No preference here. The fix as posted looks correct. The backport of
the upstream commit should be correct too (I don't see any
incompatibilities) but as you said the code has moved and got exposed
via a header, so the diff will look quite different.

I think Greg would still prefer to use the bastardized upstream commit
in such cases.

Re: [PATCH stable 6.1] devlink: Fix RCU stall when unregistering a devlink instance

[Ido Schimmel]

On Tue, Oct 01, 2024 at 03:39:53PM -0700, Jakub Kicinski wrote:
> On Tue, 1 Oct 2024 16:38:39 +0300 Ido Schimmel wrote:
> > > You need to document the heck out of why this is only relevant for this
> > > one specific kernel branch IN the changelog text, so that we understand
> > > what is going on, AND you need to get acks from the relevant maintainers
> > > of this area of the kernel to accept something that is not in Linus's
> > > tree.
> > > 
> > > But first of, why?  Why not just take the upstrema commits instead?  
> > 
> > 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.
> > 
> > The above explains why the patch is only relevant to 6.1.y.
> > 
> > Jakub / Jiri, what is your preference here? This patch or cherry picking
> > a lot of code from 6.3?
> 
> No preference here. The fix as posted looks correct. The backport of
> the upstream commit should be correct too (I don't see any
> incompatibilities) but as you said the code has moved and got exposed
> via a header, so the diff will look quite different.
> 
> I think Greg would still prefer to use the bastardized upstream commit
> in such cases.

Greg, if I augment the commit message with the necessary information,
would you be willing to take this patch instead of a much larger patch?

Re: [PATCH stable 6.1] devlink: Fix RCU stall when unregistering a devlink instance

[Greg KH]

On Sun, Oct 06, 2024 at 11:44:42AM +0300, Ido Schimmel wrote:
> On Tue, Oct 01, 2024 at 03:39:53PM -0700, Jakub Kicinski wrote:
> > On Tue, 1 Oct 2024 16:38:39 +0300 Ido Schimmel wrote:
> > > > You need to document the heck out of why this is only relevant for this
> > > > one specific kernel branch IN the changelog text, so that we understand
> > > > what is going on, AND you need to get acks from the relevant maintainers
> > > > of this area of the kernel to accept something that is not in Linus's
> > > > tree.
> > > > 
> > > > But first of, why?  Why not just take the upstrema commits instead?  
> > > 
> > > 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.
> > > 
> > > The above explains why the patch is only relevant to 6.1.y.
> > > 
> > > Jakub / Jiri, what is your preference here? This patch or cherry picking
> > > a lot of code from 6.3?
> > 
> > No preference here. The fix as posted looks correct. The backport of
> > the upstream commit should be correct too (I don't see any
> > incompatibilities) but as you said the code has moved and got exposed
> > via a header, so the diff will look quite different.
> > 
> > I think Greg would still prefer to use the bastardized upstream commit
> > in such cases.
> 
> Greg, if I augment the commit message with the necessary information,
> would you be willing to take this patch instead of a much larger patch?

I almost always want to take whatever is in Linus's tree to ensure that
it will be easier to maintain over time due to other changes needing to
happen in the same area over the next 5+ years.  ALSO, almost always,
without fail, whenever we take code that is NOT in Linus's tree, it's
wrong, and it needs to be fixed up again as it's going outside of our
normal development and review and testing processes.

So unless there is some MAJOR reason why we can't just take what is in
Linus's tree, I strongly prefer that.  It is trivial for us to take 10's
and even 100's of patches of backports over a one-off change that is
going to end up costing us more work and effort over time.

thanks,

greg k-h

Re: [PATCH stable 6.1] devlink: Fix RCU stall when unregistering a devlink instance

[Sasha Levin]

On Tue, Oct 01, 2024 at 02:20:35PM +0300, Ido Schimmel wrote:
>I read the stable rules and I am not providing an "upstream commit ID"
>since the code in upstream has been reworked, making this fix
>irrelevant. The only affected stable kernel is 6.1.y.

Could you please add some information about *how* the code has changed
so backporting became infeasible? In our experience, custom backports
tend to cause more pain than what they fix, so at least our
future-selves could use some pointers around what happened here.

-- 
Thanks,
Sasha