2023-05-03 20:45:13

by Mathieu Desnoyers

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

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 <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Joel Fernandes <[email protected]>
Cc: Josh Triplett <[email protected]>
Cc: Boqun Feng <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Lai Jiangshan <[email protected]>
Cc: Zqiang <[email protected]>
---
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

2023-05-03 20:46:00

by Boqun Feng

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

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 <[email protected]>
> Cc: "Paul E. McKenney" <[email protected]>
> Cc: Joel Fernandes <[email protected]>
> Cc: Josh Triplett <[email protected]>
> Cc: Boqun Feng <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Lai Jiangshan <[email protected]>
> Cc: Zqiang <[email protected]>

Reviewed-by: Boqun Feng <[email protected]>

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
>

2023-05-03 22:24:02

by Steven Rostedt

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

On Wed, 3 May 2023 16:32:36 -0400
Mathieu Desnoyers <[email protected]> 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 <[email protected]>
> Cc: "Paul E. McKenney" <[email protected]>
> Cc: Joel Fernandes <[email protected]>
> Cc: Josh Triplett <[email protected]>
> Cc: Boqun Feng <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Lai Jiangshan <[email protected]>
> Cc: Zqiang <[email protected]>
> ---
> 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) <[email protected]>

-- 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))
>

2023-05-04 01:50:05

by Joel Fernandes

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

On Wed, May 3, 2023 at 4:32 PM Mathieu Desnoyers
<[email protected]> 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 <[email protected]>
> Cc: "Paul E. McKenney" <[email protected]>
> Cc: Joel Fernandes <[email protected]>
> Cc: Josh Triplett <[email protected]>
> Cc: Boqun Feng <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Lai Jiangshan <[email protected]>
> Cc: Zqiang <[email protected]>

Reviewed-by: Joel Fernandes (Google) <[email protected]>

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
>

2023-05-05 00:47:06

by Paul E. McKenney

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

On Wed, May 03, 2023 at 06:06:40PM -0400, Steven Rostedt wrote:
> On Wed, 3 May 2023 16:32:36 -0400
> Mathieu Desnoyers <[email protected]> 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 <[email protected]>
> > Cc: "Paul E. McKenney" <[email protected]>
> > Cc: Joel Fernandes <[email protected]>
> > Cc: Josh Triplett <[email protected]>
> > Cc: Boqun Feng <[email protected]>
> > Cc: Steven Rostedt <[email protected]>
> > Cc: Lai Jiangshan <[email protected]>
> > Cc: Zqiang <[email protected]>
> > ---
> > 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) <[email protected]>

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 <[email protected]>
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 <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Joel Fernandes <[email protected]>
Cc: Josh Triplett <[email protected]>
Cc: Boqun Feng <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Lai Jiangshan <[email protected]>
Cc: Zqiang <[email protected]>
Reviewed-by: Boqun Feng <[email protected]>
Reviewed-by: Steven Rostedt (Google) <[email protected]>
Reviewed-by: Joel Fernandes (Google) <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>

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))

2023-05-05 13:28:47

by Mathieu Desnoyers

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

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 <[email protected]> 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 <[email protected]>
>>> Cc: "Paul E. McKenney" <[email protected]>
>>> Cc: Joel Fernandes <[email protected]>
>>> Cc: Josh Triplett <[email protected]>
>>> Cc: Boqun Feng <[email protected]>
>>> Cc: Steven Rostedt <[email protected]>
>>> Cc: Lai Jiangshan <[email protected]>
>>> Cc: Zqiang <[email protected]>
>>> ---
>>> 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) <[email protected]>
>
> 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 <[email protected]>
> 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 <[email protected]>
> Cc: "Paul E. McKenney" <[email protected]>
> Cc: Joel Fernandes <[email protected]>
> Cc: Josh Triplett <[email protected]>
> Cc: Boqun Feng <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Lai Jiangshan <[email protected]>
> Cc: Zqiang <[email protected]>
> Reviewed-by: Boqun Feng <[email protected]>
> Reviewed-by: Steven Rostedt (Google) <[email protected]>
> Reviewed-by: Joel Fernandes (Google) <[email protected]>
> Signed-off-by: Paul E. McKenney <[email protected]>
>
> 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

2023-05-05 18:44:54

by 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 <[email protected]> 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 <[email protected]>
> > > > Cc: "Paul E. McKenney" <[email protected]>
> > > > Cc: Joel Fernandes <[email protected]>
> > > > Cc: Josh Triplett <[email protected]>
> > > > Cc: Boqun Feng <[email protected]>
> > > > Cc: Steven Rostedt <[email protected]>
> > > > Cc: Lai Jiangshan <[email protected]>
> > > > Cc: Zqiang <[email protected]>
> > > > ---
> > > > 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) <[email protected]>
> >
> > 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 <[email protected]>
> > 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 <[email protected]>
> > Cc: "Paul E. McKenney" <[email protected]>
> > Cc: Joel Fernandes <[email protected]>
> > Cc: Josh Triplett <[email protected]>
> > Cc: Boqun Feng <[email protected]>
> > Cc: Steven Rostedt <[email protected]>
> > Cc: Lai Jiangshan <[email protected]>
> > Cc: Zqiang <[email protected]>
> > Reviewed-by: Boqun Feng <[email protected]>
> > Reviewed-by: Steven Rostedt (Google) <[email protected]>
> > Reviewed-by: Joel Fernandes (Google) <[email protected]>
> > Signed-off-by: Paul E. McKenney <[email protected]>
> >
> > 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
>