Messages in this thread | | | Date | Fri, 5 May 2023 11:39:24 -0700 | From | "Paul E. McKenney" <> | Subject | Re: [RFC PATCH] rcu: rcupdate.h: Add missing parentheses around macro pointer dereference |
| |
On Fri, May 05, 2023 at 09:15:12AM -0400, Mathieu Desnoyers wrote: > On 2023-05-04 20:28, Paul E. McKenney wrote: > > On Wed, May 03, 2023 at 06:06:40PM -0400, Steven Rostedt wrote: > > > On Wed, 3 May 2023 16:32:36 -0400 > > > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > > > > > > linux/rcupdate.h macros use the *p parameter without parentheses, e.g.: > > > > > > > > typeof(*p) > > > > > > > > rather than > > > > > > > > typeof(*(p)) > > > > > > > > The following test-case shows how it can generate confusion due to C > > > > operator precedence being reversed compared to the expectations: > > > > > > > > #define m(p) \ > > > > do { \ > > > > __typeof__(*p) v = 0; \ > > > > } while (0) > > > > > > > > void fct(unsigned long long *p1) > > > > { > > > > m(p1 + 1); /* works */ > > > > m(1 + p1); /* broken */ > > > > } > > > > > > > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > > > > Cc: "Paul E. McKenney" <paulmck@kernel.org> > > > > Cc: Joel Fernandes <joel@joelfernandes.org> > > > > Cc: Josh Triplett <josh@joshtriplett.org> > > > > Cc: Boqun Feng <boqun.feng@gmail.com> > > > > Cc: Steven Rostedt <rostedt@goodmis.org> > > > > Cc: Lai Jiangshan <jiangshanlai@gmail.com> > > > > Cc: Zqiang <qiang1.zhang@intel.com> > > > > --- > > > > include/linux/rcupdate.h | 18 +++++++++--------- > > > > 1 file changed, 9 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h > > > > index dcd2cf1e8326..1565012fa47f 100644 > > > > --- a/include/linux/rcupdate.h > > > > +++ b/include/linux/rcupdate.h > > > > @@ -430,16 +430,16 @@ static inline void rcu_preempt_sleep_check(void) { } > > > > #ifdef __CHECKER__ > > > > #define rcu_check_sparse(p, space) \ > > > > - ((void)(((typeof(*p) space *)p) == p)) > > > > + ((void)(((typeof(*(p)) space *)p) == p)) > > > > > > Hmm, should we have that be: > > > ((void)(((typeof(*(p)) space *)(p)) == (p))) > > > > > > In case of the 1 + p1, which would end up as: > > > > > > ((void)(((typeof(*(1 + p1)) __rcu *)1 + p1 == 1 + p1; > > > > > > I don't know how that __rcu get's passed around via the + statement there, > > > so it may be fine. May not even make sense to have that. But I like to > > > error on more parenthesis. ;-) > > > > > > The rest looks fine. > > > > > > Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org> > > > > Thank you all! I applied Steve's suggested change with attribution > > as shown below. Please let me know if I messed anything up. > > Hi Paul, > > I've done a new version of that patch which fixes other issues in rcupdate.h > in the next round. Can you hold merging this until I remove the "RFC PATCH" > tag please ? My goal is to gather feedback first to make sure everyone is OK > with the overall changes across headers, so everything can become > consistent.
Hello, Mathieu,
Very well, I have removed it.
Thanx, Paul
> Thanks, > > Mathieu > > > > > Thanx, Paul > > > > ------------------------------------------------------------------------ > > > > commit d3d734216c88fb7c13205dc62178ff5011da415b > > Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > > Date: Wed May 3 16:32:36 2023 -0400 > > > > rcu: Add missing parentheses around rcu_dereference() "p" parameter > > linux/rcupdate.h macros use the *p parameter without parentheses, e.g.: > > typeof(*p) > > rather than > > typeof(*(p)) > > The following test-case shows how it can generate confusion due to C > > operator precedence being reversed compared to the expectations: > > #define m(p) \ > > do { \ > > __typeof__(*p) v = 0; \ > > } while (0) > > void fct(unsigned long long *p1) > > { > > m(p1 + 1); /* works */ > > m(1 + p1); /* broken */ > > } > > [ paulmck: Apply Steve Rostedt additional () feedback. ] > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > > Cc: "Paul E. McKenney" <paulmck@kernel.org> > > Cc: Joel Fernandes <joel@joelfernandes.org> > > Cc: Josh Triplett <josh@joshtriplett.org> > > Cc: Boqun Feng <boqun.feng@gmail.com> > > Cc: Steven Rostedt <rostedt@goodmis.org> > > Cc: Lai Jiangshan <jiangshanlai@gmail.com> > > Cc: Zqiang <qiang1.zhang@intel.com> > > Reviewed-by: Boqun Feng <boqun.feng@gmail.com> > > Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org> > > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h > > index ddd42efc6224..cb938a89a923 100644 > > --- a/include/linux/rcupdate.h > > +++ b/include/linux/rcupdate.h > > @@ -405,16 +405,16 @@ static inline void rcu_preempt_sleep_check(void) { } > > #ifdef __CHECKER__ > > #define rcu_check_sparse(p, space) \ > > - ((void)(((typeof(*p) space *)p) == p)) > > + ((void)(((typeof(*(p)) space *)(p)) == (p))) > > #else /* #ifdef __CHECKER__ */ > > #define rcu_check_sparse(p, space) > > #endif /* #else #ifdef __CHECKER__ */ > > #define __unrcu_pointer(p, local) \ > > ({ \ > > - typeof(*p) *local = (typeof(*p) *__force)(p); \ > > + typeof(*(p)) *local = (typeof(*(p)) *__force)(p); \ > > rcu_check_sparse(p, __rcu); \ > > - ((typeof(*p) __force __kernel *)(local)); \ > > + ((typeof(*(p)) __force __kernel *)(local)); \ > > }) > > /** > > * unrcu_pointer - mark a pointer as not being RCU protected > > @@ -427,29 +427,29 @@ static inline void rcu_preempt_sleep_check(void) { } > > #define __rcu_access_pointer(p, local, space) \ > > ({ \ > > - typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \ > > + typeof(*(p)) *local = (typeof(*(p)) *__force)READ_ONCE(p); \ > > rcu_check_sparse(p, space); \ > > - ((typeof(*p) __force __kernel *)(local)); \ > > + ((typeof(*(p)) __force __kernel *)(local)); \ > > }) > > #define __rcu_dereference_check(p, local, c, space) \ > > ({ \ > > /* Dependency order vs. p above. */ \ > > - typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \ > > + typeof(*(p)) *local = (typeof(*(p)) *__force)READ_ONCE(p); \ > > RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_check() usage"); \ > > rcu_check_sparse(p, space); \ > > - ((typeof(*p) __force __kernel *)(local)); \ > > + ((typeof(*(p)) __force __kernel *)(local)); \ > > }) > > #define __rcu_dereference_protected(p, local, c, space) \ > > ({ \ > > RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_protected() usage"); \ > > rcu_check_sparse(p, space); \ > > - ((typeof(*p) __force __kernel *)(p)); \ > > + ((typeof(*(p)) __force __kernel *)(p)); \ > > }) > > #define __rcu_dereference_raw(p, local) \ > > ({ \ > > /* Dependency order vs. p above. */ \ > > typeof(p) local = READ_ONCE(p); \ > > - ((typeof(*p) __force __kernel *)(local)); \ > > + ((typeof(*(p)) __force __kernel *)(local)); \ > > }) > > #define rcu_dereference_raw(p) __rcu_dereference_raw(p, __UNIQUE_ID(rcu)) > > -- > Mathieu Desnoyers > EfficiOS Inc. > https://www.efficios.com >
| |