linux-kernel - Re: [RFC/INCOMPLETE 01/13] context_tracking: Add context_tracking_assert

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: <20150630161623.GB3717@linux.vnet.ibm.com>
Date:	Tue, 30 Jun 2015 09:16:24 -0700
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Ingo Molnar <mingo@...nel.org>
Cc:	Andy Lutomirski <luto@...nel.org>, x86@...nel.org,
	linux-kernel@...r.kernel.org,
	Frédéric Weisbecker <fweisbec@...il.com>,
	Rik van Riel <riel@...hat.com>,
	Oleg Nesterov <oleg@...hat.com>,
	Denys Vlasenko <vda.linux@...glemail.com>,
	Borislav Petkov <bp@...en8.de>,
	Kees Cook <keescook@...omium.org>,
	Brian Gerst <brgerst@...il.com>
Subject: Re: [RFC/INCOMPLETE 01/13] context_tracking: Add
 context_tracking_assert_state

On Tue, Jun 30, 2015 at 01:04:14PM +0200, Ingo Molnar wrote:
> 
> * Paul E. McKenney <paulmck@...ux.vnet.ibm.com> wrote:
> 
> > > Yeah, and inverting the condition. Assuming the condition was assert()-style 
> > > inverted to begin with! :-)
> > 
> > It appears to have been.  ;-)
> > 
> > Please see below for an untested patch.  I made this be one big patch, but could 
> > have one patch add RCU_LOCKDEP_WARN(), a series convert uses from 
> > rcu_lockdep_assert() to RCU_LOCKDEP_WARN(), and a final patch remove 
> > rcu_lockdep_assert().  Let me know!
> 
> One big patch is perfect I think - it's a rename and a relatively mechanic 
> inversion of conditions, no point in splitting it up any more IMHO. (But it's your 
> call really.)
> 
> So I had a quick look at this patch, and IMHO the RCU_LOCKDEP_WARN() lines read a 
> lot more 'naturally', because the new, inverted conditions now highlight buggy 
> scenarios - which has the same logic parity as the kernel's historic 
> WARN_ON()/BUG_ON() patterns:
> 
> Reviewed-by: Ingo Molnar <mingo@...nel.org>

Thank you, added!

> This one looked a bit weird:
> 
> > index a0a0dd03c73a..47268fb1d27b 100644
> > --- a/kernel/rcu/update.c
> > +++ b/kernel/rcu/update.c
> > @@ -589,8 +589,8 @@ EXPORT_SYMBOL_GPL(call_rcu_tasks);
> >  void synchronize_rcu_tasks(void)
> >  {
> >  	/* Complain if the scheduler has not started.  */
> > -	rcu_lockdep_assert(!rcu_scheduler_active,
> > -			   "synchronize_rcu_tasks called too soon");
> > +	RCU_LOCKDEP_WARN(rcu_scheduler_active,
> > +			 "synchronize_rcu_tasks called too soon");
> >  
> 
> So I'd assume that a flag called 'rcu_scheduler_active' would be 1 if the RCU 
> scheduler is active.
> 
> So why do we warn on it being active? Shouldn't the condition be:
> 
> 	RCU_LOCKDEP_WARN(!rcu_scheduler_active,
> 			 "synchronize_rcu_tasks called too soon");
> 
> I.e. we warn when the RCU scheduler is not yet active and we called 
> synchronize_rcu_tasks() too soon?
> 
> So either the original assert() was wrong, or I'm missing something obvious?

You are missing nothing!  But I really do test this stuff...

Ah, I see...  I need the following patch in order to enable lockdep-RCU
on one of my RCU-tasks rcutorture scenarios...  :-/

Good catch!  There were at least three bugs hiding behind that one!  ;-)

							Thanx, Paul

------------------------------------------------------------------------

commit dc883f1668c83f9525a13ee1b3cd45e9e85a0fe5
Author: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>
Date:   Tue Jun 30 09:14:01 2015 -0700

    rcutorture: Enable lockdep-RCU on TASKS01
    
    Currently none of the RCU-tasks scenarios enables lockdep-RCU, which
    causes bugs to be missed.  This commit therefore enables lockdep-RCU
    on TASKS01.
    
    Signed-off-by: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>

diff --git a/tools/testing/selftests/rcutorture/configs/rcu/TASKS01 b/tools/testing/selftests/rcutorture/configs/rcu/TASKS01
index 2cc0e60eba6e..bafe94cbd739 100644
--- a/tools/testing/selftests/rcutorture/configs/rcu/TASKS01
+++ b/tools/testing/selftests/rcutorture/configs/rcu/TASKS01
@@ -5,6 +5,6 @@ CONFIG_PREEMPT_NONE=n
 CONFIG_PREEMPT_VOLUNTARY=n
 CONFIG_PREEMPT=y
 CONFIG_DEBUG_LOCK_ALLOC=y
-CONFIG_PROVE_LOCKING=n
-#CHECK#CONFIG_PROVE_RCU=n
+CONFIG_PROVE_LOCKING=y
+#CHECK#CONFIG_PROVE_RCU=y
 CONFIG_RCU_EXPERT=y

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