linux-kernel - Re: [PATCH] fix rcu_try_flip_waitack_needed() to prevent grace-period stall

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 22 Mar 2008 17:55:53 -0700
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Peter Zijlstra <a.p.zijlstra@...llo.nl>
Cc:	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Ingo Molnar <mingo@...e.hu>
Subject: Re: [PATCH] fix rcu_try_flip_waitack_needed() to prevent grace-period stall

On Sat, Mar 22, 2008 at 06:43:48PM +0100, Peter Zijlstra wrote:
> On Fri, 2008-03-21 at 13:38 -0700, Paul E. McKenney wrote:
> > The comment was correct -- need to make the code match the comment.
> > Without this patch, if a CPU goes dynticks idle (and stays there forever)
> > in just the right phase of preemptible-RCU grace-period processing,
> > grace periods stall.  The offending sequence of events (courtesy
> > of Promela/spin, at least after I got the liveness criterion coded
> > correctly...) is as follows:
> > 
> > o	CPU 0 is in dynticks-idle mode.  Its dynticks_progress_counter
> > 	is (say) 10.
> > 
> > o	CPU 0 takes an interrupt, so rcu_irq_enter() increments CPU 0's
> > 	dynticks_progress_counter to 11.
> > 
> > o	CPU 1 is doing RCU grace-period processing in rcu_try_flip_idle(),
> > 	sees rcu_pending(), so invokes dyntick_save_progress_counter(),
> > 	which in turn takes a snapshot of CPU 0's dynticks_progress_counter
> > 	into CPU 0's rcu_dyntick_snapshot -- now set to 11.  CPU 1 then
> > 	updates the RCU grace-period state to rcu_try_flip_waitack().
> > 
> > o	CPU 0 returns from its interrupt, so rcu_irq_exit() increments
> > 	CPU 0's dynticks_progress_counter to 12.
> > 
> > o	CPU 1 later invokes rcu_try_flip_waitack(), which notices that
> > 	CPU 0 has not yet responded, and hence in turn invokes
> > 	rcu_try_flip_waitack_needed().  This function examines the
> > 	state of CPU 0's dynticks_progress_counter and rcu_dyntick_snapshot
> > 	variables, which it copies to curr (== 12) and snap (== 11),
> > 	respectively.
> > 
> > 	Because curr!=snap, the first condition fails.
> > 
> > 	Because curr-snap is only 1 and snap is odd, the second
> > 	condition fails.
> > 
> > 	rcu_try_flip_waitack_needed() therefore incorrectly concludes
> > 	that it must wait for CPU 0 to explicitly acknowledge the
> > 	counter flip.
> > 
> > o	CPU 0 remains forever in dynticks-idle mode, never taking
> > 	any more hardware interrupts or any NMIs, and never running
> > 	any more tasks.  (Of course, -something- will usually eventually
> > 	happen, which might be why we haven't seen this one in the
> > 	wild.  Still should be fixed!)
> > 
> > Therefore the grace period never ends.  Fix is to make the code match
> > the comment, as shown below.  With this fix, the above scenario
> > would be satisfied with curr being even, and allow the grace period
> > to proceed.
> > 
> > Signed-off-by: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>
> 
> Paul, should this go upstream ASAP?

Given that any activity (task wakeup, interrupt, NMI) on the offending CPU
gets things going again, I have a hard time labeling it as super urgent.
So I could argue for it being added to the last release candidate,
but not to the final release itself.

Seem reasonable?

							Thanx, Paul

> > ---
> > 
> >  rcupreempt.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff -urpNa -X dontdiff linux-2.6.25-rc6/kernel/rcupreempt.c linux-2.6.25-rc6-rcunohz-if/kernel/rcupreempt.c
> > --- linux-2.6.25-rc6/kernel/rcupreempt.c	2008-03-16 17:45:17.000000000 -0700
> > +++ linux-2.6.25-rc6-rcunohz-if/kernel/rcupreempt.c	2008-03-18 20:27:47.000000000 -0700
> > @@ -569,7 +569,7 @@ rcu_try_flip_waitack_needed(int cpu)
> >  	 * that this CPU already acknowledged the counter.
> >  	 */
> >  
> > -	if ((curr - snap) > 2 || (snap & 0x1) == 0)
> > +	if ((curr - snap) > 2 || (curr & 0x1) == 0)
> >  		return 0;
> >  
> >  	/* We need this CPU to explicitly acknowledge the counter flip. */
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@...r.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/