Re: [PATCH AUTOSEL 3.18 55/98] rcu: Clear need_qs flag to prevent splat

[Paul E. McKenney]

On Thu, Oct 25, 2018 at 10:18:10AM -0400, Sasha Levin wrote:
> From: "Paul E. McKenney" <paulmck@xxxxxxxxxxxxxxxxxx>
> 
> [ Upstream commit c0135d07b013fa8f7ba9ec91b4369c372e6a28cb ]
> 
> If the scheduling-clock interrupt sets the current tasks need_qs flag,
> but if the current CPU passes through a quiescent state in the meantime,
> then rcu_preempt_qs() will fail to clear the need_qs flag, which can fool
> RCU into thinking that additional rcu_read_unlock_special() processing
> is needed.  This commit therefore clears the need_qs flag before checking
> for additional processing.

Given that this produced a splat that someone (you, in fact) actually
encountered, no objection to it going to -stable.

> For this problem to occur, we need rcu_preempt_data.passed_quiesce equal
> to true and current->rcu_read_unlock_special.b.need_qs also equal to true.
> This condition can occur as follows:
> 
> 1.	CPU 0 is aware of the current preemptible RCU grace period,
> 	but has not yet passed through a quiescent state.  Among other
> 	things, this means that rcu_preempt_data.passed_quiesce is false.
> 
> 2.	Task A running on CPU 0 enters a preemptible RCU read-side
> 	critical section.
> 
> 3.	CPU 0 takes a scheduling-clock interrupt, which notices the
> 	RCU read-side critical section and the need for a quiescent state,
> 	and thus sets current->rcu_read_unlock_special.b.need_qs to true.
> 
> 4.	Task A is preempted, enters the scheduler, eventually invoking
> 	rcu_preempt_note_context_switch() which in turn invokes
> 	rcu_preempt_qs().
> 
> 	Because rcu_preempt_data.passed_quiesce is false,
> 	control enters the body of the "if" statement, which sets
> 	rcu_preempt_data.passed_quiesce to true.
> 
> 5.	At this point, CPU 0 takes an interrupt.  The interrupt
> 	handler contains an RCU read-side critical section, and
> 	the rcu_read_unlock() notes that current->rcu_read_unlock_special
> 	is nonzero, and thus invokes rcu_read_unlock_special().
> 
> 6.	Once in rcu_read_unlock_special(), the fact that
> 	current->rcu_read_unlock_special.b.need_qs is true becomes
> 	apparent, so rcu_read_unlock_special() invokes rcu_preempt_qs().
> 	Recursively, given that we interrupted out of that same
> 	function in the preceding step.
> 
> 7.	Because rcu_preempt_data.passed_quiesce is now true,
> 	rcu_preempt_qs() does nothing, and simply returns.
> 
> 8.	Upon return to rcu_read_unlock_special(), it is noted that
> 	current->rcu_read_unlock_special is still nonzero (because
> 	the interrupted rcu_preempt_qs() had not yet gotten around
> 	to clearing current->rcu_read_unlock_special.b.need_qs).
> 
> 9.	Execution proceeds to the WARN_ON_ONCE(), which notes that
> 	we are in an interrupt handler and thus duly splats.
> 
> The solution, as noted above, is to make rcu_read_unlock_special()
> clear out current->rcu_read_unlock_special.b.need_qs after calling
> rcu_preempt_qs().  The interrupted rcu_preempt_qs() will clear it again,
> but this is harmless.  The worst that happens is that we clobber another
> attempt to set this field, but this is not a problem because we just
> got done reporting a quiescent state.
> 
> Reported-by: Sasha Levin <sasha.levin@xxxxxxxxxx>
> Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>
> [ paulmck: Fix embarrassing build bug noted by Sasha Levin. ]
> Tested-by: Sasha Levin <sasha.levin@xxxxxxxxxx>
> Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
> ---
>  kernel/rcu/tree_plugin.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index c1d7f27bd38f..c038831bfa57 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -328,6 +328,7 @@ void rcu_read_unlock_special(struct task_struct *t)
>  	special = t->rcu_read_unlock_special;
>  	if (special.b.need_qs) {
>  		rcu_preempt_qs();
> +		t->rcu_read_unlock_special.b.need_qs = false;
>  		if (!t->rcu_read_unlock_special.s) {
>  			local_irq_restore(flags);
>  			return;
> -- 
> 2.17.1
>