Re: [RFC PATCH] rcu: rcupdate.h: Add missing parentheses around macro pointer dereference

[Paul E. McKenney]

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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > > > Cc: "Paul E. McKenney" <paulmck@xxxxxxxxxx>
> > > > Cc: Joel Fernandes <joel@xxxxxxxxxxxxxxxxx>
> > > > Cc: Josh Triplett <josh@xxxxxxxxxxxxxxxx>
> > > > Cc: Boqun Feng <boqun.feng@xxxxxxxxx>
> > > > Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> > > > Cc: Lai Jiangshan <jiangshanlai@xxxxxxxxx>
> > > > Cc: Zqiang <qiang1.zhang@xxxxxxxxx>
> > > > ---
> > > >   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@xxxxxxxxxxx>
> > 
> > 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@xxxxxxxxxxxx>
> > 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@xxxxxxxxxxxx>
> >      Cc: "Paul E. McKenney" <paulmck@xxxxxxxxxx>
> >      Cc: Joel Fernandes <joel@xxxxxxxxxxxxxxxxx>
> >      Cc: Josh Triplett <josh@xxxxxxxxxxxxxxxx>
> >      Cc: Boqun Feng <boqun.feng@xxxxxxxxx>
> >      Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> >      Cc: Lai Jiangshan <jiangshanlai@xxxxxxxxx>
> >      Cc: Zqiang <qiang1.zhang@xxxxxxxxx>
> >      Reviewed-by: Boqun Feng <boqun.feng@xxxxxxxxx>
> >      Reviewed-by: Steven Rostedt (Google) <rostedt@xxxxxxxxxxx>
> >      Reviewed-by: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>
> >      Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxx>
> > 
> > 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
>