linux-kernel - Re: rcu warnings cause stack overflow

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] [day] [month] [year] [list]
Message-ID: <20120204165202.GC2467@linux.vnet.ibm.com>
Date:	Sat, 4 Feb 2012 08:52:02 -0800
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Frederic Weisbecker <fweisbec@...il.com>
Cc:	Heiko Carstens <heiko.carstens@...ibm.com>,
	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...e.hu>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>
Subject: Re: rcu warnings cause stack overflow

On Sat, Feb 04, 2012 at 02:13:35PM +0100, Frederic Weisbecker wrote:
> On Fri, Feb 03, 2012 at 10:33:35AM -0800, Paul E. McKenney wrote:
> > On Fri, Feb 03, 2012 at 10:32:14AM +0100, Heiko Carstens wrote:
> > > On Thu, Feb 02, 2012 at 11:11:16AM -0800, Paul E. McKenney wrote:
> > > > On Thu, Feb 02, 2012 at 03:52:20PM +0100, Frederic Weisbecker wrote:
> > > > > On Thu, Feb 02, 2012 at 01:27:42PM +0100, Heiko Carstens wrote:
> > > > > > On Wed, Feb 01, 2012 at 04:14:48PM +0100, Frederic Weisbecker wrote:
> > > > > > > > Removing the WARN_ON_ONCE will fix this and, if lockdep is turned on, still
> > > > > > > > will find illegal uses. But it won't work for lockdep off configs...
> > > > > > > > So we probably want something better than the patch below.
> > > > > > > 
> > > > > > > Ah ok. Hmm, but why are you using an exception to implement WARN_ON()
> > > > > > > in s390? Is it to have a whole new stack for the warning path in order
> > > > > > > to avoid stack overflow from the place that called the WARN_ON() ?
> > > > > > 
> > > > > > The reason was to reduce the code footprint of the WARN_ON() and also
> > > > > > be able to print the register contents at the time the warning happened.
> > > > > 
> > > > > Ah ok, makes sense.
> > > > 
> > > > So Frederic should push his anti-recursion patch, then?
> > > 
> > > Yes, please.
> > > 
> > > Tested-by: Heiko Carstens <heiko.carstens@...ibm.com>
> > > 
> > > It still generates recursive warnings because the WARNON_ONCE is inlined and
> > > every different usage will generate an exception, but it didn't produce a
> > > stack overflow anymore.
> > > To avoid the recursive warning the patch below would help. Not sure if it's
> > > worth it...
> > > 
> > > Subject: [PATCH] rcu: move rcu_is_cpu_idle() check warning into C file
> > > 
> > > From: Heiko Carstens <heiko.carstens@...ibm.com>
> > > 
> > > rcu_read_lock() and rcu_read_unlock() generate a warning if a cpu is in
> > > extended quiescant state. Since these functions are inlined this can cause
> > > a lot of warnings if in the processing of the WARN_ON_ONCE() there is
> > > another usage of e.g. rcu_read_lock(). To make sure we only get one
> > > warning (and avoid possible stack overflows) uninline the check.
> > > 
> > > Signed-off-by: Heiko Carstens <heiko.carstens@...ibm.com>
> > > ---
> > >  include/linux/rcupdate.h |    9 +++++++--
> > >  kernel/rcupdate.c        |    6 ++++++
> > >  2 files changed, 13 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > index 81c04f4..9fe7be5 100644
> > > --- a/include/linux/rcupdate.h
> > > +++ b/include/linux/rcupdate.h
> > > @@ -230,22 +230,27 @@ static inline void destroy_rcu_head_on_stack(struct rcu_head *head)
> > > 
> > >  #ifdef CONFIG_PROVE_RCU
> > >  extern int rcu_is_cpu_idle(void);
> > > +extern void rcu_warn_if_is_cpu_idle(void);
> > >  #else /* !CONFIG_PROVE_RCU */
> > >  static inline int rcu_is_cpu_idle(void)
> > >  {
> > >  	return 0;
> > >  }
> > > +
> > > +static inline void rcu_warn_if_is_cpu_idle(void)
> > > +{
> > > +}
> > >  #endif /* else !CONFIG_PROVE_RCU */
> > > 
> > >  static inline void rcu_lock_acquire(struct lockdep_map *map)
> > >  {
> > > -	WARN_ON_ONCE(rcu_is_cpu_idle());
> > > +	rcu_warn_if_is_cpu_idle();
> > 
> > Thank you for the patch, but this WARN_ON_ONCE() has now been removed
> > in favor of lockdep-RCU checks elsewhere.  This has the advantage of
> > leveraging lockdep's splat-once and anti-recursion facilities.
> > 
> > So I believe that current -rcu covers this.  (And yes, I do need to
> > push my most recent changes out.)
> 
> This still uncovers cases where we call rcu_read_lock() without matching
> rcu_dereference(). Amongst this we have rcu_dereference_raw(), conditional
> rcu_dereference() and may be cases where we simply have no rcu_dereference*
> but we use rcu_read_lock() alone for some reason...

Ah!  I moved the !rcu_is_cpu_idle() check to the read-side primitives,
for example:

	static inline void rcu_read_lock(void)
	{
		__rcu_read_lock();
		__acquire(RCU);
		rcu_lock_acquire(&rcu_lock_map);
		rcu_lockdep_assert(!rcu_is_cpu_idle(),
				   "rcu_read_lock() used illegally while idle");
	}

The reason I did this is that there is a good chance that it will be
necessary to allow SRCU readers on CPUs that the other flavors of RCU
believe to be idle.  One reason for this is that the KVM guys want your
tickless idle to apply to KVM guest processes, but they need the guest
to be in an SRCU read-side critical section during that same time.

Now, this would break given current synchronize_srcu(), but Peter
Zijlstra's recent SRCU patches might get us to a point where it would work.

Then of course there will be the challenge of making it scale, but one
thing at a time.  ;-)

							Thanx, Paul

--
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/