[PATCH RFC] Make rcu_dereference_raw() safe for NMI etc.

[PATCH RFC] Make rcu_dereference_raw() safe for NMI etc.

[Paul E. McKenney]

As promised/threatened on IRC.

							Thanx, Paul

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

rcu: Reverse rcu_dereference_check() conditions

The rcu_dereference_check() family of primitives evaluates the RCU
lockdep expression first, and only then evaluates the expression passed
in.  This works fine normally, but can potentially fail in environments
(such as NMI handlers) where lockdep cannot be invoked.  The problem is
that even if the expression passed in is "1", the compiler would need to
prove that the RCU lockdep expression (rcu_read_lock_held(), for example)
is free of side effects in order to be able to elide it.  Given that
rcu_read_lock_held() is sometimes separately compiled, the compiler cannot
always use this optimization.

This commit therefore reverse the order of evaluation, so that the
expression passed in is evaluated first, and the RCU lockdep expression is
evaluated only if the passed-in expression evaluated to false, courtesy
of the C-language short-circuit boolean evaluation rules.  This compells
the compiler to forego executing the RCU lockdep expression in cases
where the passed-in expression evaluates to "1" at compile time, so that
(for example) rcu_dereference_raw() can be guaranteed to execute safely
withing an NMI handler.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index b33aed415872..9d3d0e1d0766 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -731,7 +731,7 @@ static inline void rcu_preempt_sleep_check(void)
  * annotated as __rcu.
  */
 #define rcu_dereference_check(p, c) \
-	__rcu_dereference_check((p), rcu_read_lock_held() || (c), __rcu)
+	__rcu_dereference_check((p), (c) || rcu_read_lock_held(), __rcu)
 
 /**
  * rcu_dereference_bh_check() - rcu_dereference_bh with debug checking
@@ -741,7 +741,7 @@ static inline void rcu_preempt_sleep_check(void)
  * This is the RCU-bh counterpart to rcu_dereference_check().
  */
 #define rcu_dereference_bh_check(p, c) \
-	__rcu_dereference_check((p), rcu_read_lock_bh_held() || (c), __rcu)
+	__rcu_dereference_check((p), (c) || rcu_read_lock_bh_held(), __rcu)
 
 /**
  * rcu_dereference_sched_check() - rcu_dereference_sched with debug checking
@@ -751,7 +751,7 @@ static inline void rcu_preempt_sleep_check(void)
  * This is the RCU-sched counterpart to rcu_dereference_check().
  */
 #define rcu_dereference_sched_check(p, c) \
-	__rcu_dereference_check((p), rcu_read_lock_sched_held() || (c), \
+	__rcu_dereference_check((p), (c) || rcu_read_lock_sched_held(), \
 				__rcu)
 
 #define rcu_dereference_raw(p) rcu_dereference_check(p, 1) /*@@@ needed? @@@*/
diff --git a/include/linux/srcu.h b/include/linux/srcu.h
index 9cfd9623fb03..bdeb4567b71e 100644
--- a/include/linux/srcu.h
+++ b/include/linux/srcu.h
@@ -182,7 +182,7 @@ static inline int srcu_read_lock_held(struct srcu_struct *sp)
  * lockdep_is_held() calls.
  */
 #define srcu_dereference_check(p, sp, c) \
-	__rcu_dereference_check((p), srcu_read_lock_held(sp) || (c), __rcu)
+	__rcu_dereference_check((p), (c) || srcu_read_lock_held(sp), __rcu)
 
 /**
  * srcu_dereference - fetch SRCU-protected pointer for later dereferencing

Re: [PATCH RFC] Make rcu_dereference_raw() safe for NMI etc.

[Peter Zijlstra]

On Mon, Feb 02, 2015 at 11:55:33AM -0800, Paul E. McKenney wrote:
> As promised/threatened on IRC.
> 
> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> rcu: Reverse rcu_dereference_check() conditions
> 
> The rcu_dereference_check() family of primitives evaluates the RCU
> lockdep expression first, and only then evaluates the expression passed
> in.  This works fine normally, but can potentially fail in environments
> (such as NMI handlers) where lockdep cannot be invoked.  The problem is
> that even if the expression passed in is "1", the compiler would need to
> prove that the RCU lockdep expression (rcu_read_lock_held(), for example)
> is free of side effects in order to be able to elide it.  Given that
> rcu_read_lock_held() is sometimes separately compiled, the compiler cannot
> always use this optimization.
> 
> This commit therefore reverse the order of evaluation, so that the
> expression passed in is evaluated first, and the RCU lockdep expression is
> evaluated only if the passed-in expression evaluated to false, courtesy
> of the C-language short-circuit boolean evaluation rules.  This compells
> the compiler to forego executing the RCU lockdep expression in cases
> where the passed-in expression evaluates to "1" at compile time, so that
> (for example) rcu_dereference_raw() can be guaranteed to execute safely
> withing an NMI handler.

My particular worry yesterday was tracing; I was looking at
rcu_read_{,un}lock_notrace() and wondered what would happen if I used
list_for_each_entry_rcu() under it.

_If_ it would indeed do that call, we can end up in:

  list_entry_rcu() -> rcu_dereference_raw() -> rcu_dereference_check()
  -> rcu_read_lock_held() -> rcu_lockdep_current_cpu_online()
  -> preempt_disable()

And preempt_disable() is a traceable thing -- not to mention half the
callstack above doesn't have notrace annotations and would equally
generate function trace events.

Thereby rendering the rcu list ops unsuitable for using under _notrace()
rcu primitives.

So yes, fully agreed on this patch.

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>


FWIW I think I won't be needing the rcu _notrace() bits (for now), but
it leading to this patch was worth it anyhow ;-)

Re: [PATCH RFC] Make rcu_dereference_raw() safe for NMI etc.

[Paul E. McKenney]

On Tue, Feb 03, 2015 at 12:00:40PM +0100, Peter Zijlstra wrote:
> On Mon, Feb 02, 2015 at 11:55:33AM -0800, Paul E. McKenney wrote:
> > As promised/threatened on IRC.
> > 
> > 							Thanx, Paul
> > 
> > ------------------------------------------------------------------------
> > 
> > rcu: Reverse rcu_dereference_check() conditions
> > 
> > The rcu_dereference_check() family of primitives evaluates the RCU
> > lockdep expression first, and only then evaluates the expression passed
> > in.  This works fine normally, but can potentially fail in environments
> > (such as NMI handlers) where lockdep cannot be invoked.  The problem is
> > that even if the expression passed in is "1", the compiler would need to
> > prove that the RCU lockdep expression (rcu_read_lock_held(), for example)
> > is free of side effects in order to be able to elide it.  Given that
> > rcu_read_lock_held() is sometimes separately compiled, the compiler cannot
> > always use this optimization.
> > 
> > This commit therefore reverse the order of evaluation, so that the
> > expression passed in is evaluated first, and the RCU lockdep expression is
> > evaluated only if the passed-in expression evaluated to false, courtesy
> > of the C-language short-circuit boolean evaluation rules.  This compells
> > the compiler to forego executing the RCU lockdep expression in cases
> > where the passed-in expression evaluates to "1" at compile time, so that
> > (for example) rcu_dereference_raw() can be guaranteed to execute safely
> > withing an NMI handler.
> 
> My particular worry yesterday was tracing; I was looking at
> rcu_read_{,un}lock_notrace() and wondered what would happen if I used
> list_for_each_entry_rcu() under it.
> 
> _If_ it would indeed do that call, we can end up in:
> 
>   list_entry_rcu() -> rcu_dereference_raw() -> rcu_dereference_check()
>   -> rcu_read_lock_held() -> rcu_lockdep_current_cpu_online()
>   -> preempt_disable()
> 
> And preempt_disable() is a traceable thing -- not to mention half the
> callstack above doesn't have notrace annotations and would equally
> generate function trace events.
> 
> Thereby rendering the rcu list ops unsuitable for using under _notrace()
> rcu primitives.
> 
> So yes, fully agreed on this patch.
> 
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Applied your Acked-by, thank you!

> FWIW I think I won't be needing the rcu _notrace() bits (for now), but
> it leading to this patch was worth it anyhow ;-)

No argument here!  This could have been a nasty one to track down,
depending on exactly how it manifested.  Much easier this way!  ;-)

							Thanx, Paul