[PATCH] srcu: Fix WARN_ON() for rcu_segcblist_n_cbs() in cleanup_srcu_struct()

[PATCH] srcu: Fix WARN_ON() for rcu_segcblist_n_cbs() in cleanup_srcu_struct()

[Sunho Park]

The WARN_ON() added by commit 78a38cbf6f20 ("srcu: Queue sdp->work
when the delay timer is successfully deleted") uses rcu_segcblist_n_cbs()
to detect callbacks that srcu_barrier() failed to wait for. However, the
->len counter is decremented only at the end of srcu_invoke_callbacks(),
after the invoking loop has finished. Since srcu_barrier() can return
right after the barrier callback is invoked, cleanup_srcu_struct() can see
a non-zero n_cbs even though the cblist is already physically empty,
falsely triggering the WARN_ON() together with a still-pending delay_work
timer.

Use rcu_segcblist_empty(), which checks the actual head of the cblist.
Callbacks that have genuinely not been invoked yet still leave the list
non-empty, so the WARN_ON() still catches callers that skip srcu_barrier()
or queue callbacks after it.

Link: https://lore.kernel.org/rcu/e6350377085ddd85d6ef00d8e9a67bd50c762d3c@linux.dev/T/#t
Reported-by: syzbot+d4faf7db59e11f6fd1ab@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=d4faf7db59e11f6fd1ab
Fixes: 78a38cbf6f20 ("srcu: Queue sdp->work when the delay timer is successfully deleted")
Signed-off-by: Sunho Park <shpark061104@gmail.com>
---
 kernel/rcu/srcutree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index ed204b3f4b84..ad27880dd690 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -704,7 +704,7 @@ void cleanup_srcu_struct(struct srcu_struct *ssp)
 		// Call srcu_barrier() before this cleanup_srcu_struct()
 		// to avoid triggering this WARN_ON().
 		if (WARN_ON(timer_delete_sync(&sdp->delay_work) &&
-			    rcu_segcblist_n_cbs(&sdp->srcu_cblist)) &&
+			    !rcu_segcblist_empty(&sdp->srcu_cblist)) &&
 		    rcu_cpu_beenfullyonline(sdp->cpu))
 			queue_work_on(sdp->cpu, rcu_gp_wq, &sdp->work);
 		flush_work(&sdp->work);
-- 
2.43.0

Re: [PATCH] srcu: Fix WARN_ON() for rcu_segcblist_n_cbs() in cleanup_srcu_struct()

[Zqiang]

> 
> The WARN_ON() added by commit 78a38cbf6f20 ("srcu: Queue sdp->work
> when the delay timer is successfully deleted") uses rcu_segcblist_n_cbs()
> to detect callbacks that srcu_barrier() failed to wait for. However, the
> ->len counter is decremented only at the end of srcu_invoke_callbacks(),
> after the invoking loop has finished. Since srcu_barrier() can return
> right after the barrier callback is invoked, cleanup_srcu_struct() can see
> a non-zero n_cbs even though the cblist is already physically empty,
> falsely triggering the WARN_ON() together with a still-pending delay_work
> timer.
> 
> Use rcu_segcblist_empty(), which checks the actual head of the cblist.
> Callbacks that have genuinely not been invoked yet still leave the list
> non-empty, so the WARN_ON() still catches callers that skip srcu_barrier()
> or queue callbacks after it.


You should describe in detail how this scene was triggered like this：

1. call_srcu(&kvm->srcu, &bus->rcu, __free_bus) start a SRCU GP1.

2. The SRCU GP1 end, arms a timer and queue sdp->work, but this sdp->work not yet run.

3. Another call_srcu(&kvm->srcu, &bus->rcu, __free_bus) invoke srcu_segcblist_advance()
   to push step1 srcu callback to RCU_DONE_TAIL and start a SRCU GP2.
 
4. srcu_barrier() is called and queues barrier callbacks after step3 srcu callback,
   waits for srcu_invoke_callbacks() to invoke them.

5. The SRCU GP2 end, also arms a timer, and the sdp->work by step2 queued begin run,
   the srcu_invoke_callbacks() invoke srcu_segcblist_advance() also push step3 and step4
   srcu callback to RCU_DONE_TAIL, all srcu callback execution completed,
   but the not yet invoke rcu_segcblist_add_len() to update srcu_cblist's len.

6. The srcu_barrier() return, cleanup_srcu_struct() begin run, after then, find
   the step5 timer still pending and the srcu_cblist's len not zero, trigger
   WARN_ON()
   
Thanks
Zqiang


> 
> Link: https://lore.kernel.org/rcu/e6350377085ddd85d6ef00d8e9a67bd50c762d3c@linux.dev/T/#t
> Reported-by: syzbot+d4faf7db59e11f6fd1ab@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=d4faf7db59e11f6fd1ab
> Fixes: 78a38cbf6f20 ("srcu: Queue sdp->work when the delay timer is successfully deleted")
> Signed-off-by: Sunho Park <shpark061104@gmail.com>
> ---
>  kernel/rcu/srcutree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index ed204b3f4b84..ad27880dd690 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -704,7 +704,7 @@ void cleanup_srcu_struct(struct srcu_struct *ssp)
>  // Call srcu_barrier() before this cleanup_srcu_struct()
>  // to avoid triggering this WARN_ON().
>  if (WARN_ON(timer_delete_sync(&sdp->delay_work) &&
> - rcu_segcblist_n_cbs(&sdp->srcu_cblist)) &&
> + !rcu_segcblist_empty(&sdp->srcu_cblist)) &&
>  rcu_cpu_beenfullyonline(sdp->cpu))
>  queue_work_on(sdp->cpu, rcu_gp_wq, &sdp->work);
>  flush_work(&sdp->work);
> -- 
> 2.43.0
>