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] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 30 Jun 2015 13:04:14 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
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


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

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?

Thanks,

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