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
>
