[BUG] srcu: false-positive WARN in cleanup_srcu_struct() after 78a38cbf6f20

[BUG] srcu: false-positive WARN in cleanup_srcu_struct() after 78a38cbf6f20

[Sunho Park]

The main crash report [1] which is tested on non-merged commit 6b8c8af514d7
is caused by the single-condition WARN_ON(timer_delete_sync(&sdp->delay_work))
in cleanup_srcu_struct(&kvm->irq_srcu). As discussed in [2], it is a false
positive because irq_srcu does not use call_srcu().

However, the merged WARN_ON(timer_delete_sync(&sdp->delay_work) &&
rcu_segcblist_n_cbs(&sdp->srcu_cblist)) is also triggered in
cleanup_srcu_struct(&kvm->srcu) which is called after srcu_barrier() properly.
Although my syz test command [3] failed to reproduce, it was reproducible
in my QEMU environment built with the .config of the report.

I found out that the return value of rcu_segcblist_n_cbs can be nonzero
even after srcu_barrier() because of the srcu_barrier_cb() that srcu_barrier()
inserts at the end of the queue. The length of cblist is decreased after
srcu_invoke_callbacks() finishes invoking all callbacks in a batch. But
srcu_barrier() may return when all the srcu_barrier_cb() are called, bringing
the counter to zero, even if srcu_invoke_callbacks() has not yet decremented
the length. So checking cblist length before flush_work() is inaccurate.

By the comment of srcu_barrier(), it guarantees that all the previously
registered call_srcu() callbacks are completed. Therefore srcu_barrier()
did what it said, only the length of cblist was not updated. I think there
are two options:

1) Not to check the length of cblist before flush_work()
2) Make srcu_barrier() guarantee the length of cblist is adjusted when it returns

[1] https://lore.kernel.org/all/6a78d191.b50370da.49fe0.0042.GAE@google.com/T
[2] https://lore.kernel.org/rcu/01484cd339ea024dd7c01f02a9781f00e67cb52d@linux.dev/T
[3] https://lore.kernel.org/all/6a8bc111.91706f20.16b6e3.02cd.GAE@google.com

Reported-by: syzbot+d4faf7db59e11f6fd1ab@syzkaller.appspotmail.com

Re: [BUG] srcu: false-positive WARN in cleanup_srcu_struct() after 78a38cbf6f20

[Zqiang]

> 
> The main crash report [1] which is tested on non-merged commit 6b8c8af514d7
> is caused by the single-condition WARN_ON(timer_delete_sync(&sdp->delay_work))
> in cleanup_srcu_struct(&kvm->irq_srcu). As discussed in [2], it is a false
> positive because irq_srcu does not use call_srcu().
> 
> However, the merged WARN_ON(timer_delete_sync(&sdp->delay_work) &&
> rcu_segcblist_n_cbs(&sdp->srcu_cblist)) is also triggered in
> cleanup_srcu_struct(&kvm->srcu) which is called after srcu_barrier() properly.
> Although my syz test command [3] failed to reproduce, it was reproducible
> in my QEMU environment built with the .config of the report.
> 
> I found out that the return value of rcu_segcblist_n_cbs can be nonzero
> even after srcu_barrier() because of the srcu_barrier_cb() that srcu_barrier()
> inserts at the end of the queue. The length of cblist is decreased after
> srcu_invoke_callbacks() finishes invoking all callbacks in a batch. But
> srcu_barrier() may return when all the srcu_barrier_cb() are called, bringing
> the counter to zero, even if srcu_invoke_callbacks() has not yet decremented
> the length. So checking cblist length before flush_work() is inaccurate.

If srcu_barrier() be invoke before srcu_cleanup(), and after srcu_barrier()
completion, there are no concurrent srcu grace period start again (e.g. call_srcu() calls), 
the timer_delete_sync() should return false, the rcu_segcblist_n_cbs()
will not be check.

Or did I miss something?

Thanks
Zqiang


> 
> By the comment of srcu_barrier(), it guarantees that all the previously
> registered call_srcu() callbacks are completed. Therefore srcu_barrier()
> did what it said, only the length of cblist was not updated. I think there
> are two options:
> 
> 1) Not to check the length of cblist before flush_work()
> 2) Make srcu_barrier() guarantee the length of cblist is adjusted when it returns
> 
> [1] https://lore.kernel.org/all/6a78d191.b50370da.49fe0.0042.GAE@google.com/T
> [2] https://lore.kernel.org/rcu/01484cd339ea024dd7c01f02a9781f00e67cb52d@linux.dev/T
> [3] https://lore.kernel.org/all/6a8bc111.91706f20.16b6e3.02cd.GAE@google.com
> 
> Reported-by: syzbot+d4faf7db59e11f6fd1ab@syzkaller.appspotmail.com
>

Re: [BUG] srcu: false-positive WARN in cleanup_srcu_struct() after 78a38cbf6f20

[Sunho Park]

On 8/25/26 21:33, Zqiang wrote:
> 
> If srcu_barrier() be invoke before srcu_cleanup(), and after srcu_barrier()
> completion, there are no concurrent srcu grace period start again (e.g. call_srcu() calls),
> the timer_delete_sync() should return false, the rcu_segcblist_n_cbs()
> will not be check.
> 
> Or did I miss something?
In srcu_gp_end(), delayed work timer is always armed in SRCU_SIZE_SMALL 
mode regardless of whether its cblist has callbacks:

	if (ss_state < SRCU_SIZE_WAIT_BARRIER) {
		srcu_schedule_cbs_sdp(per_cpu_ptr(ssp->sda, get_boot_cpu_id()),
					cbdelay);
	} else {
		...
	}

and with cbdelay=1 (the non-expedited path) srcu_schedule_cbs_sdp() arms 
the per-CPU delay_work timer for the next jiffy. This includes the 
srcu_gp_end() whose grace period makes the barrier's callbacks ready, so 
an armed timer is left pending when srcu_invoke_callbacks() completes 
the barrier.

Thanks
Sunho Park