lkml.org 
[lkml]   [2023]   [May]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [RFC PATCH 3/4] rculist.h: Fix parentheses around macro pointer parameter use
Hello Mathieu,

On Wed, May 3, 2023 at 9:29 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> Add missing parentheses around use of macro argument "pos" in those
> patterns to ensure operator precedence behaves as expected:
>
> - typeof(*pos)
> - pos->member
>
> This corrects the following usage pattern where operator precedence is
> unexpected:
>
> LIST_HEAD(testlist);
>
> struct test {
> struct list_head node;
> int a;
> };
>
> // pos->member issue
> void f(void)
> {
> struct test *t1;
> struct test **t2 = &t1;
>
> list_for_each_entry_rcu((*t2), &testlist, node) { /* works */
> //...
> }
> list_for_each_entry_rcu(*t2, &testlist, node) { /* broken */
> //...
> }

Yeah it is not clear why anyone would ever want to use it like this.
Why don't they just pass t1 to list_for_each_entry_rcu() ? I would
rather it break them and they re-think their code ;).

> }
>
> The typeof(*pos) lack of parentheses around "pos" is not an issue per se
> in the specific macros modified here because "pos" is used as an lvalue,
> which should prevent use of any operator causing issue. Still add the
> extra parentheses for consistency.

The consistency argument is valid though. I will stay neutral on this
patch. ;-)

thanks!

- Joel



>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Frederic Weisbecker <frederic@kernel.org>
> Cc: Neeraj Upadhyay <quic_neeraju@quicinc.com>
> 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>
> Cc: rcu@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
> include/linux/rculist.h | 28 ++++++++++++++--------------
> 1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> index d29740be4833..d27aeff5447d 100644
> --- a/include/linux/rculist.h
> +++ b/include/linux/rculist.h
> @@ -388,9 +388,9 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
> */
> #define list_for_each_entry_rcu(pos, head, member, cond...) \
> for (__list_check_rcu(dummy, ## cond, 0), \
> - pos = list_entry_rcu((head)->next, typeof(*pos), member); \
> - &pos->member != (head); \
> - pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
> + pos = list_entry_rcu((head)->next, typeof(*(pos)), member);\
> + &(pos)->member != (head); \
> + pos = list_entry_rcu((pos)->member.next, typeof(*(pos)), member))
>
> /**
> * list_for_each_entry_srcu - iterate over rcu list of given type
> @@ -407,9 +407,9 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
> */
> #define list_for_each_entry_srcu(pos, head, member, cond) \
> for (__list_check_srcu(cond), \
> - pos = list_entry_rcu((head)->next, typeof(*pos), member); \
> - &pos->member != (head); \
> - pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
> + pos = list_entry_rcu((head)->next, typeof(*(pos)), member);\
> + &(pos)->member != (head); \
> + pos = list_entry_rcu((pos)->member.next, typeof(*(pos)), member))
>
> /**
> * list_entry_lockless - get the struct for this entry
> @@ -441,9 +441,9 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
> * but never deleted.
> */
> #define list_for_each_entry_lockless(pos, head, member) \
> - for (pos = list_entry_lockless((head)->next, typeof(*pos), member); \
> - &pos->member != (head); \
> - pos = list_entry_lockless(pos->member.next, typeof(*pos), member))
> + for (pos = list_entry_lockless((head)->next, typeof(*(pos)), member); \
> + &(pos)->member != (head); \
> + pos = list_entry_lockless((pos)->member.next, typeof(*(pos)), member))
>
> /**
> * list_for_each_entry_continue_rcu - continue iteration over list of given type
> @@ -464,9 +464,9 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
> * position.
> */
> #define list_for_each_entry_continue_rcu(pos, head, member) \
> - for (pos = list_entry_rcu(pos->member.next, typeof(*pos), member); \
> - &pos->member != (head); \
> - pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
> + for (pos = list_entry_rcu((pos)->member.next, typeof(*(pos)), member); \
> + &(pos)->member != (head); \
> + pos = list_entry_rcu((pos)->member.next, typeof(*(pos)), member))
>
> /**
> * list_for_each_entry_from_rcu - iterate over a list from current point
> @@ -486,8 +486,8 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
> * after the given position.
> */
> #define list_for_each_entry_from_rcu(pos, head, member) \
> - for (; &(pos)->member != (head); \
> - pos = list_entry_rcu(pos->member.next, typeof(*(pos)), member))
> + for (; &(pos)->member != (head); \
> + pos = list_entry_rcu((pos)->member.next, typeof(*(pos)), member))
>
> /**
> * hlist_del_rcu - deletes entry from hash list without re-initialization
> --
> 2.25.1
>

\
 
 \ /
  Last update: 2023-05-04 18:20    [W:0.129 / U:0.248 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site