Re: [RFC PATCH] rcu: rcupdate.h: Add missing parentheses around macro pointer dereference
From: Mathieu Desnoyers
Date: Fri May 05 2023 - 09:15:15 EST
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.
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