diff mbox series

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

Message ID 20230503203236.1587590-1-mathieu.desnoyers@efficios.com
State New
Headers
Series [RFC] rcu: rcupdate.h: Add missing parentheses around macro pointer dereference |

Commit Message

Mathieu Desnoyers May 3, 2023, 8:32 p.m. UTC 
  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(-)

Comments

Boqun Feng May 3, 2023, 8:33 p.m. UTC | #1 
On Wed, May 03, 2023 at 04:32:36PM -0400, Mathieu Desnoyers 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>

Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

Regards,
Boqun

> ---
>  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))
>  #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
> @@ -452,29 +452,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))
>  
> -- 
> 2.25.1
>
Steven Rostedt May 3, 2023, 10:06 p.m. UTC | #2 
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>

-- Steve



>  #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
> @@ -452,29 +452,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))
>
Joel Fernandes May 4, 2023, 1:17 a.m. UTC | #3 
On Wed, May 3, 2023 at 4:32 PM 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>

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

thanks,

 - Joel


> ---
>  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))
>  #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
> @@ -452,29 +452,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))
>
> --
> 2.25.1
>
Paul E. McKenney May 5, 2023, 12:28 a.m. UTC | #4 
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.

							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 May 5, 2023, 1:15 p.m. UTC | #5 
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))
>
Paul E. McKenney May 5, 2023, 6:39 p.m. UTC | #6 
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
>
diff mbox series

Patch

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))
 #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
@@ -452,29 +452,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))