linux-kernel - Re: [PATCH 1/4] rcu: Detect uses of rcu read side in extended quiescent states

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]

Message-ID: <20110610002350.GO2285@linux.vnet.ibm.com>
Date:	Thu, 9 Jun 2011 17:23:50 -0700
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Frederic Weisbecker <fweisbec@...il.com>
Cc:	LKML <linux-kernel@...r.kernel.org>, Ingo Molnar <mingo@...e.hu>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Thomas Gleixner <tglx@...utronix.de>,
	Milton Miller <miltonm@....com>
Subject: Re: [PATCH 1/4] rcu: Detect uses of rcu read side in extended
 quiescent states

On Fri, Jun 10, 2011 at 01:47:24AM +0200, Frederic Weisbecker wrote:
> Detect uses of rcu that are not supposed to happen when we
> are in an extended quiescent state.
> 
> This can happen for example if we use rcu between the time we
> stop the tick and the time we restart it. Or inside an irq that
> didn't use rcu_irq_enter,exit() or other possible kind of rcu API
> misuse.
> 
> v2: Rebase against latest rcu changes, handle tiny RCU as well

Good idea on checking for RCU read-side critical sections happening
in dyntick-idle periods!

But wouldn't it be better to put the checks in rcu_read_lock() and
friends?  The problem I see with putting them in rcu_dereference_check()
is that someone can legitimately do something like the following
while in dyntick-idle mode:

	spin_lock(&mylock);
	/* do a bunch of stuff */
	p = rcu_dereference_check(myrcuptr, lockdep_is_held(&mylock));

The logic below would complain about this usage, despite the fact
that it is perfectly safe because the update-side lock is held.

Make sense, or am I missing something?

						Thanx, Paul

> Signed-off-by: Frederic Weisbecker <fweisbec@...il.com>
> Cc: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>
> Cc: Ingo Molnar <mingo@...e.hu>
> Cc: Peter Zijlstra <a.p.zijlstra@...llo.nl>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Milton Miller <miltonm@....com>
> ---
>  include/linux/rcupdate.h |    9 +++++++++
>  kernel/rcutiny.c         |   13 +++++++++++++
>  kernel/rcutree.c         |   14 ++++++++++++++
>  3 files changed, 36 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 9db50df..6cad1f3 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -388,6 +388,12 @@ extern int rcu_my_thread_group_empty(void);
> 
>  #endif /* #else #ifdef CONFIG_PROVE_RCU */
> 
> +#if defined(CONFIG_PROVE_RCU) && defined(CONFIG_NO_HZ)
> +extern bool rcu_check_extended_qs(void);
> +#else
> +static inline bool rcu_check_extended_qs(void) { return false; }
> +#endif
> +
>  /*
>   * Helper functions for rcu_dereference_check(), rcu_dereference_protected()
>   * and rcu_assign_pointer().  Some of these could be folded into their
> @@ -415,6 +421,9 @@ extern int rcu_my_thread_group_empty(void);
>  		typeof(*p) *_________p1 = (typeof(*p)*__force )ACCESS_ONCE(p); \
>  		rcu_lockdep_assert(c, "suspicious rcu_dereference_check()" \
>  				      " usage"); \
> +		rcu_lockdep_assert(!rcu_check_extended_qs(), \
> +				   "use of rcu_dereference_check() in an extended" \
> +				   " quiescent state");	\
>  		rcu_dereference_sparse(p, space); \
>  		smp_read_barrier_depends(); \
>  		((typeof(*p) __force __kernel *)(_________p1)); \
> diff --git a/kernel/rcutiny.c b/kernel/rcutiny.c
> index 775d69a..44a2a15 100644
> --- a/kernel/rcutiny.c
> +++ b/kernel/rcutiny.c
> @@ -76,6 +76,19 @@ void rcu_exit_nohz(void)
>  	rcu_dynticks_nesting++;
>  }
> 
> +
> +#ifdef CONFIG_PROVE_RCU
> +
> +bool rcu_check_extended_qs(void)
> +{
> +	if (!rcu_dynticks_nesting)
> +		return true;
> +
> +	return false;
> +}
> +
> +#endif
> +
>  #endif /* #ifdef CONFIG_NO_HZ */
> 
>  /*
> diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> index 305dfae..8211527 100644
> --- a/kernel/rcutree.c
> +++ b/kernel/rcutree.c
> @@ -432,6 +432,20 @@ void rcu_irq_exit(void)
>  	rcu_enter_nohz();
>  }
> 
> +#ifdef CONFIG_PROVE_RCU
> +
> +bool rcu_check_extended_qs(void)
> +{
> +	struct rcu_dynticks *rdtp = &__get_cpu_var(rcu_dynticks);
> +
> +	if (atomic_read(&rdtp->dynticks) & 0x1)
> +		return false;
> +
> +	return true;
> +}
> +
> +#endif /* CONFIG_PROVE_RCU */
> +
>  #ifdef CONFIG_SMP
> 
>  /*
> -- 
> 1.7.5.4
> 
--
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/