Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230504012914.1797355-1-mathieu.desnoyers@efficios.com> <20230504012914.1797355-3-mathieu.desnoyers@efficios.com>
In-Reply-To: <20230504012914.1797355-3-mathieu.desnoyers@efficios.com>
From:   Joel Fernandes <joel@joelfernandes.org>
Date:   Thu, 4 May 2023 12:19:38 -0400
Message-ID: <CAEXW_YRoqwX1Z3OcRJKxDZ+jAN6Woby+avC9FTdP_8+xyuYqCQ@mail.gmail.com>
Subject: Re: [RFC PATCH 3/4] rculist.h: Fix parentheses around macro pointer
 parameter use
To:     Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        linux-kernel@vger.kernel.org,
        "Paul E. McKenney" <paulmck@kernel.org>,
        Frederic Weisbecker <frederic@kernel.org>,
        Neeraj Upadhyay <quic_neeraju@quicinc.com>,
        Josh Triplett <josh@joshtriplett.org>,
        Boqun Feng <boqun.feng@gmail.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Lai Jiangshan <jiangshanlai@gmail.com>,
        Zqiang <qiang1.zhang@intel.com>, rcu@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hello Mathieu,

On Wed, May 3, 2023 at 9:29=E2=80=AFPM 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 =3D &t1;
>
>           list_for_each_entry_rcu((*t2), &testlist, node) {       /* work=
s */
>                   //...
>           }
>           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 l=
ist_head *list,
>   */
>  #define list_for_each_entry_rcu(pos, head, member, cond...)            \
>         for (__list_check_rcu(dummy, ## cond, 0),                       \
> -            pos =3D list_entry_rcu((head)->next, typeof(*pos), member); =
 \
> -               &pos->member !=3D (head);                                =
 \
> -               pos =3D list_entry_rcu(pos->member.next, typeof(*pos), me=
mber))
> +            pos =3D list_entry_rcu((head)->next, typeof(*(pos)), member)=
;\
> +               &(pos)->member !=3D (head);                              =
 \
> +               pos =3D list_entry_rcu((pos)->member.next, typeof(*(pos))=
, member))
>
>  /**
>   * list_for_each_entry_srcu    -       iterate over rcu list of given ty=
pe
> @@ -407,9 +407,9 @@ static inline void list_splice_tail_init_rcu(struct l=
ist_head *list,
>   */
>  #define list_for_each_entry_srcu(pos, head, member, cond)              \
>         for (__list_check_srcu(cond),                                   \
> -            pos =3D list_entry_rcu((head)->next, typeof(*pos), member); =
 \
> -               &pos->member !=3D (head);                                =
 \
> -               pos =3D list_entry_rcu(pos->member.next, typeof(*pos), me=
mber))
> +            pos =3D list_entry_rcu((head)->next, typeof(*(pos)), member)=
;\
> +               &(pos)->member !=3D (head);                              =
 \
> +               pos =3D 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 l=
ist_head *list,
>   * but never deleted.
>   */
>  #define list_for_each_entry_lockless(pos, head, member) \
> -       for (pos =3D list_entry_lockless((head)->next, typeof(*pos), memb=
er); \
> -            &pos->member !=3D (head); \
> -            pos =3D list_entry_lockless(pos->member.next, typeof(*pos), =
member))
> +       for (pos =3D list_entry_lockless((head)->next, typeof(*(pos)), me=
mber); \
> +            &(pos)->member !=3D (head); \
> +            pos =3D list_entry_lockless((pos)->member.next, typeof(*(pos=
)), member))
>
>  /**
>   * list_for_each_entry_continue_rcu - continue iteration over list of gi=
ven type
> @@ -464,9 +464,9 @@ static inline void list_splice_tail_init_rcu(struct l=
ist_head *list,
>   * position.
>   */
>  #define list_for_each_entry_continue_rcu(pos, head, member)            \
> -       for (pos =3D list_entry_rcu(pos->member.next, typeof(*pos), membe=
r); \
> -            &pos->member !=3D (head);    \
> -            pos =3D list_entry_rcu(pos->member.next, typeof(*pos), membe=
r))
> +       for (pos =3D list_entry_rcu((pos)->member.next, typeof(*(pos)), m=
ember); \
> +            &(pos)->member !=3D (head);  \
> +            pos =3D list_entry_rcu((pos)->member.next, typeof(*(pos)), m=
ember))
>
>  /**
>   * 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 l=
ist_head *list,
>   * after the given position.
>   */
>  #define list_for_each_entry_from_rcu(pos, head, member)                 =
       \
> -       for (; &(pos)->member !=3D (head);                               =
         \
> -               pos =3D list_entry_rcu(pos->member.next, typeof(*(pos)), =
member))
> +       for (; &(pos)->member !=3D (head);                               =
 \
> +               pos =3D list_entry_rcu((pos)->member.next, typeof(*(pos))=
, member))
>
>  /**
>   * hlist_del_rcu - deletes entry from hash list without re-initializatio=
n
> --
> 2.25.1
>