| 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
| ||
|
Message-ID: <20190524170328.GA7540@andrea>
Date: Fri, 24 May 2019 19:03:28 +0200
From: Andrea Parri <andrea.parri@...rulasolutions.com>
To: Joel Fernandes <joel@...lfernandes.org>
Cc: linux-kernel@...r.kernel.org,
"Paul E. McKenney" <paulmck@...ux.ibm.com>,
Josh Triplett <josh@...htriplett.org>,
Steven Rostedt <rostedt@...dmis.org>,
Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
Lai Jiangshan <jiangshanlai@...il.com>, rcu@...r.kernel.org,
Peter Zijlstra <peterz@...radead.org>,
Will Deacon <will.deacon@....com>,
Mark Rutland <mark.rutland@....com>,
Matthew Wilcox <willy@...radead.org>,
Sasha Levin <sashal@...nel.org>
Subject: Re: [PATCH] rcu: Prevent evaluation of rcu_assign_pointer()
On Fri, May 24, 2019 at 12:45:09PM -0400, Joel Fernandes wrote:
> On Fri, May 24, 2019 at 12:36:37PM +0200, Andrea Parri wrote:
> > Quoting Paul [1]:
> >
> > "Given that a quick (and perhaps error-prone) search of the uses
> > of rcu_assign_pointer() in v5.1 didn't find a single use of the
> > return value, let's please instead change the documentation and
> > implementation to eliminate the return value."
> >
> > [1] https://lkml.kernel.org/r/20190523135013.GL28207@linux.ibm.com
> >
> > Signed-off-by: Andrea Parri <andrea.parri@...rulasolutions.com>
> > Cc: "Paul E. McKenney" <paulmck@...ux.ibm.com>
> > Cc: Josh Triplett <josh@...htriplett.org>
> > Cc: Steven Rostedt <rostedt@...dmis.org>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
> > Cc: Lai Jiangshan <jiangshanlai@...il.com>
> > Cc: Joel Fernandes <joel@...lfernandes.org>
> > Cc: rcu@...r.kernel.org
> > Cc: Peter Zijlstra <peterz@...radead.org>
> > Cc: Will Deacon <will.deacon@....com>
> > Cc: Mark Rutland <mark.rutland@....com>
> > Cc: Matthew Wilcox <willy@...radead.org>
> > Cc: Sasha Levin <sashal@...nel.org>
> > ---
> > Matthew, Sasha:
> >
> > The patch is based on -rcu/dev; I took the liberty of applying the
> > same change to your #defines in:
> >
> > tools/testing/radix-tree/linux/rcupdate.h
> > tools/include/linux/rcu.h
> >
> > but I admit that I'm not familiar with their uses: please shout if
> > you have any objections with it.
> > ---
> > Documentation/RCU/whatisRCU.txt | 8 ++++----
> > include/linux/rcupdate.h | 5 ++---
> > tools/include/linux/rcu.h | 11 +++++++++--
> > tools/testing/radix-tree/linux/rcupdate.h | 5 ++++-
> > 4 files changed, 19 insertions(+), 10 deletions(-)
> >
> > diff --git a/Documentation/RCU/whatisRCU.txt b/Documentation/RCU/whatisRCU.txt
> > index 981651a8b65d2..f99a87b9a88fa 100644
> > --- a/Documentation/RCU/whatisRCU.txt
> > +++ b/Documentation/RCU/whatisRCU.txt
> > @@ -212,7 +212,7 @@ synchronize_rcu()
> >
> > rcu_assign_pointer()
> >
> > - typeof(p) rcu_assign_pointer(p, typeof(p) v);
> > + rcu_assign_pointer(p, typeof(p) v);
> >
> > Yes, rcu_assign_pointer() -is- implemented as a macro, though it
> > would be cool to be able to declare a function in this manner.
> > @@ -220,9 +220,9 @@ rcu_assign_pointer()
> >
> > The updater uses this function to assign a new value to an
> > RCU-protected pointer, in order to safely communicate the change
> > - in value from the updater to the reader. This function returns
> > - the new value, and also executes any memory-barrier instructions
> > - required for a given CPU architecture.
> > + in value from the updater to the reader. This macro does not
> > + evaluate to an rvalue, but it does execute any memory-barrier
> > + instructions required for a given CPU architecture.
> >
> > Perhaps just as important, it serves to document (1) which
> > pointers are protected by RCU and (2) the point at which a
> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > index 915460ec08722..a5f61a08e65fc 100644
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -367,7 +367,7 @@ static inline void rcu_preempt_sleep_check(void) { }
> > * other macros that it invokes.
> > */
> > #define rcu_assign_pointer(p, v) \
> > -({ \
> > +do { \
> > uintptr_t _r_a_p__v = (uintptr_t)(v); \
> > rcu_check_sparse(p, __rcu); \
> > \
> > @@ -375,8 +375,7 @@ static inline void rcu_preempt_sleep_check(void) { }
> > WRITE_ONCE((p), (typeof(p))(_r_a_p__v)); \
> > else \
> > smp_store_release(&p, RCU_INITIALIZER((typeof(p))_r_a_p__v)); \
> > - _r_a_p__v; \
> > -})
> > +} while (0)
> >
> > /**
> > * rcu_swap_protected() - swap an RCU and a regular pointer
> > diff --git a/tools/include/linux/rcu.h b/tools/include/linux/rcu.h
> > index 7d02527e5bcea..01a435ee48cd6 100644
> > --- a/tools/include/linux/rcu.h
> > +++ b/tools/include/linux/rcu.h
> > @@ -19,7 +19,14 @@ static inline bool rcu_is_watching(void)
> > return false;
> > }
> >
> > -#define rcu_assign_pointer(p, v) ((p) = (v))
> > -#define RCU_INIT_POINTER(p, v) p=(v)
> > +#define rcu_assign_pointer(p, v) \
> > +do { \
> > + (p) = (v); \
> > +} while (0)
> > +
> > +#define RCU_INIT_POINTER(p, v) \
> > +do { \
> > + (p) = (v); \
> > +} while (0)
> >
> > #endif
> > diff --git a/tools/testing/radix-tree/linux/rcupdate.h b/tools/testing/radix-tree/linux/rcupdate.h
> > index fd280b070fdb1..48212f3a758e6 100644
> > --- a/tools/testing/radix-tree/linux/rcupdate.h
> > +++ b/tools/testing/radix-tree/linux/rcupdate.h
> > @@ -7,6 +7,9 @@
> > #define rcu_dereference_raw(p) rcu_dereference(p)
> > #define rcu_dereference_protected(p, cond) rcu_dereference(p)
> > #define rcu_dereference_check(p, cond) rcu_dereference(p)
> > -#define RCU_INIT_POINTER(p, v) (p) = (v)
> > +#define RCU_INIT_POINTER(p, v) \
> > +do { \
> > + (p) = (v); \
> > +} while (0)
> >
> > #endif
> > --
> > 2.7.4
> >
>
> Other than Paul's nits, LGTM. Thanks.
Thank you, Joel! Will fix those and resend, most likely next week.
Andrea
Powered by blists - more mailing lists