Re: [RFC PATCH 3/4] rculist.h: Fix parentheses around macro pointer parameter use

[Joel Fernandes]

Hello Mathieu,

On Wed, May 3, 2023 at 9:29 PM Mathieu Desnoyers
<mathieu.desnoyers@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> Cc: "Paul E. McKenney" <paulmck@xxxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Frederic Weisbecker <frederic@xxxxxxxxxx>
> Cc: Neeraj Upadhyay <quic_neeraju@xxxxxxxxxxx>
> 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>
> Cc: rcu@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> ---
>  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
>