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

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

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.

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