[RFC PATCH 1/4] rcu: rcupdate.h: Add missing parentheses around macro pointer dereference

[RFC PATCH 1/4] rcu: rcupdate.h: Add missing parentheses around macro pointer dereference

[Mathieu Desnoyers]

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@efficios.com>
Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
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>
---
 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..38e84f3d515e 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)))
 #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
@@ -452,29 +452,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))
 
-- 
2.25.1

[RFC PATCH 2/4] list.h: Fix parentheses around macro pointer parameter use

[Mathieu Desnoyers]

Add missing parentheses around use of macro argument "pos" in those
patterns to ensure operator precedence behaves as expected:

- typeof(*pos)
- pos->member

Remove useless parentheses around use of macro parameter (head) in the
following pattern:

- list_is_head(pos, (head))

Because comma is the lowest priority operator already, so the extra pair
of parentheses is redundant.

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((*t2), &testlist, node) {   /* works */
                  //...
          }
          list_for_each_entry(*t2, &testlist, node) {     /* broken */
                  //...
          }
  }

  // typeof(*pos) issue
  void f2(void)
  {
          struct test *t1 = NULL, *t2;

          t2 = list_prepare_entry((0 + t1), &testlist, node);     /* works */
          t2 = list_prepare_entry(0 + t1, &testlist, node);       /* broken */
  }

Note that the macros in which "pos" is also used as an lvalue probably
don't suffer from the lack of parentheses around "pos" in typeof(*pos),
but add those nevertheless to keep everything consistent.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Ricardo Martinez <ricardo.martinez@linux.intel.com>
---
 include/linux/list.h | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/include/linux/list.h b/include/linux/list.h
index f10344dbad4d..bb106238eaf6 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -603,7 +603,7 @@ static inline void list_splice_tail_init(struct list_head *list,
  * @head:	the head for your list.
  */
 #define list_for_each(pos, head) \
-	for (pos = (head)->next; !list_is_head(pos, (head)); pos = pos->next)
+	for (pos = (head)->next; !list_is_head(pos, head); pos = (pos)->next)
 
 /**
  * list_for_each_rcu - Iterate over a list in an RCU-safe fashion
@@ -612,8 +612,8 @@ static inline void list_splice_tail_init(struct list_head *list,
  */
 #define list_for_each_rcu(pos, head)		  \
 	for (pos = rcu_dereference((head)->next); \
-	     !list_is_head(pos, (head)); \
-	     pos = rcu_dereference(pos->next))
+	     !list_is_head(pos, head); \
+	     pos = rcu_dereference((pos)->next))
 
 /**
  * list_for_each_continue - continue iteration over a list
@@ -623,7 +623,7 @@ static inline void list_splice_tail_init(struct list_head *list,
  * Continue to iterate over a list, continuing after the current position.
  */
 #define list_for_each_continue(pos, head) \
-	for (pos = pos->next; !list_is_head(pos, (head)); pos = pos->next)
+	for (pos = (pos)->next; !list_is_head(pos, head); pos = (pos)->next)
 
 /**
  * list_for_each_prev	-	iterate over a list backwards
@@ -631,7 +631,7 @@ static inline void list_splice_tail_init(struct list_head *list,
  * @head:	the head for your list.
  */
 #define list_for_each_prev(pos, head) \
-	for (pos = (head)->prev; !list_is_head(pos, (head)); pos = pos->prev)
+	for (pos = (head)->prev; !list_is_head(pos, head); pos = (pos)->prev)
 
 /**
  * list_for_each_safe - iterate over a list safe against removal of list entry
@@ -640,9 +640,9 @@ static inline void list_splice_tail_init(struct list_head *list,
  * @head:	the head for your list.
  */
 #define list_for_each_safe(pos, n, head) \
-	for (pos = (head)->next, n = pos->next; \
-	     !list_is_head(pos, (head)); \
-	     pos = n, n = pos->next)
+	for (pos = (head)->next, n = (pos)->next; \
+	     !list_is_head(pos, head); \
+	     pos = n, n = (pos)->next)
 
 /**
  * list_for_each_prev_safe - iterate over a list backwards safe against removal of list entry
@@ -651,9 +651,9 @@ static inline void list_splice_tail_init(struct list_head *list,
  * @head:	the head for your list.
  */
 #define list_for_each_prev_safe(pos, n, head) \
-	for (pos = (head)->prev, n = pos->prev; \
-	     !list_is_head(pos, (head)); \
-	     pos = n, n = pos->prev)
+	for (pos = (head)->prev, n = (pos)->prev; \
+	     !list_is_head(pos, head); \
+	     pos = n, n = (pos)->prev)
 
 /**
  * list_count_nodes - count nodes in the list
@@ -677,7 +677,7 @@ static inline size_t list_count_nodes(struct list_head *head)
  * @member:	the name of the list_head within the struct.
  */
 #define list_entry_is_head(pos, head, member)				\
-	(&pos->member == (head))
+	(&(pos)->member == (head))
 
 /**
  * list_for_each_entry	-	iterate over list of given type
@@ -686,7 +686,7 @@ static inline size_t list_count_nodes(struct list_head *head)
  * @member:	the name of the list_head within the struct.
  */
 #define list_for_each_entry(pos, head, member)				\
-	for (pos = list_first_entry(head, typeof(*pos), member);	\
+	for (pos = list_first_entry(head, typeof(*(pos)), member);	\
 	     !list_entry_is_head(pos, head, member);			\
 	     pos = list_next_entry(pos, member))
 
@@ -697,7 +697,7 @@ static inline size_t list_count_nodes(struct list_head *head)
  * @member:	the name of the list_head within the struct.
  */
 #define list_for_each_entry_reverse(pos, head, member)			\
-	for (pos = list_last_entry(head, typeof(*pos), member);		\
+	for (pos = list_last_entry(head, typeof(*(pos)), member);	\
 	     !list_entry_is_head(pos, head, member); 			\
 	     pos = list_prev_entry(pos, member))
 
@@ -710,7 +710,7 @@ static inline size_t list_count_nodes(struct list_head *head)
  * Prepares a pos entry for use as a start point in list_for_each_entry_continue().
  */
 #define list_prepare_entry(pos, head, member) \
-	((pos) ? : list_entry(head, typeof(*pos), member))
+	((pos) ? : list_entry(head, typeof(*(pos)), member))
 
 /**
  * list_for_each_entry_continue - continue iteration over list of given type
@@ -773,7 +773,7 @@ static inline size_t list_count_nodes(struct list_head *head)
  * @member:	the name of the list_head within the struct.
  */
 #define list_for_each_entry_safe(pos, n, head, member)			\
-	for (pos = list_first_entry(head, typeof(*pos), member),	\
+	for (pos = list_first_entry(head, typeof(*(pos)), member),	\
 		n = list_next_entry(pos, member);			\
 	     !list_entry_is_head(pos, head, member); 			\
 	     pos = n, n = list_next_entry(n, member))
@@ -820,7 +820,7 @@ static inline size_t list_count_nodes(struct list_head *head)
  * of list entry.
  */
 #define list_for_each_entry_safe_reverse(pos, n, head, member)		\
-	for (pos = list_last_entry(head, typeof(*pos), member),		\
+	for (pos = list_last_entry(head, typeof(*(pos)), member),	\
 		n = list_prev_entry(pos, member);			\
 	     !list_entry_is_head(pos, head, member); 			\
 	     pos = n, n = list_prev_entry(n, member))
@@ -1033,10 +1033,10 @@ static inline void hlist_move_list(struct hlist_head *old,
 #define hlist_entry(ptr, type, member) container_of(ptr,type,member)
 
 #define hlist_for_each(pos, head) \
-	for (pos = (head)->first; pos ; pos = pos->next)
+	for (pos = (head)->first; pos ; pos = (pos)->next)
 
 #define hlist_for_each_safe(pos, n, head) \
-	for (pos = (head)->first; pos && ({ n = pos->next; 1; }); \
+	for (pos = (head)->first; pos && ({ n = (pos)->next; 1; }); \
 	     pos = n)
 
 #define hlist_entry_safe(ptr, type, member) \
@@ -1082,8 +1082,8 @@ static inline void hlist_move_list(struct hlist_head *old,
  * @member:	the name of the hlist_node within the struct.
  */
 #define hlist_for_each_entry_safe(pos, n, head, member) 		\
-	for (pos = hlist_entry_safe((head)->first, typeof(*pos), member);\
-	     pos && ({ n = pos->member.next; 1; });			\
-	     pos = hlist_entry_safe(n, typeof(*pos), member))
+	for (pos = hlist_entry_safe((head)->first, typeof(*(pos)), member);\
+	     pos && ({ n = (pos)->member.next; 1; });			\
+	     pos = hlist_entry_safe(n, typeof(*(pos)), member))
 
 #endif
-- 
2.25.1

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

[Mathieu Desnoyers]

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 */
                  //...
          }
  }

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.

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

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

[Mathieu Desnoyers]

Add missing parentheses around use of macro argument "pos" in those
patterns to ensure operator precedence behaves as expected:

- typeof(*pos)
- pos->member

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.

Remove useless parentheses around use of macro parameter (node) in the
following pattern:

  llist_entry((node), typeof(*pos), member)

Because comma is the lowest priority operator already, so the extra pair
of parentheses is redundant.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Huang Ying <ying.huang@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 include/linux/llist.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/llist.h b/include/linux/llist.h
index 85bda2d02d65..45d358c15d0d 100644
--- a/include/linux/llist.h
+++ b/include/linux/llist.h
@@ -173,9 +173,9 @@ static inline void init_llist_head(struct llist_head *list)
  * reverse the order by yourself before traversing.
  */
 #define llist_for_each_entry_safe(pos, n, node, member)			       \
-	for (pos = llist_entry((node), typeof(*pos), member);		       \
+	for (pos = llist_entry(node, typeof(*(pos)), member);		       \
 	     member_address_is_nonnull(pos, member) &&			       \
-	        (n = llist_entry(pos->member.next, typeof(*n), member), true); \
+		(n = llist_entry((pos)->member.next, typeof(*(n)), member), true); \
 	     pos = n)
 
 /**
-- 
2.25.1

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

[Huang, Ying]

Hi, Mathieu,

Mathieu Desnoyers <mathieu.desnoyers@efficios.com> writes:

> Add missing parentheses around use of macro argument "pos" in those
> patterns to ensure operator precedence behaves as expected:
>
> - typeof(*pos)
> - pos->member
>
> 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.

I don't think it's necessary to add parentheses here.  As you said,
"pos" is used as an lvalue.

> Remove useless parentheses around use of macro parameter (node) in the
> following pattern:
>
>   llist_entry((node), typeof(*pos), member)
>
> Because comma is the lowest priority operator already, so the extra pair
> of parentheses is redundant.

This change looks good for me.

Best Regards,
Huang, Ying

> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Huang Ying <ying.huang@intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> ---
>  include/linux/llist.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/llist.h b/include/linux/llist.h
> index 85bda2d02d65..45d358c15d0d 100644
> --- a/include/linux/llist.h
> +++ b/include/linux/llist.h
> @@ -173,9 +173,9 @@ static inline void init_llist_head(struct llist_head *list)
>   * reverse the order by yourself before traversing.
>   */
>  #define llist_for_each_entry_safe(pos, n, node, member)			       \
> -	for (pos = llist_entry((node), typeof(*pos), member);		       \
> +	for (pos = llist_entry(node, typeof(*(pos)), member);		       \
>  	     member_address_is_nonnull(pos, member) &&			       \
> -	        (n = llist_entry(pos->member.next, typeof(*n), member), true); \
> +		(n = llist_entry((pos)->member.next, typeof(*(n)), member), true); \
>  	     pos = n)
>  
>  /**

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

[Andy Shevchenko]

On Wed, May 03, 2023 at 09:29:12PM -0400, Mathieu Desnoyers wrote:
> Add missing parentheses around use of macro argument "pos" in those
> patterns to ensure operator precedence behaves as expected:
> 
> - typeof(*pos)
> - pos->member
> 
> Remove useless parentheses around use of macro parameter (head) in the
> following pattern:
> 
> - list_is_head(pos, (head))
> 
> Because comma is the lowest priority operator already, so the extra pair
> of parentheses is redundant.
> 
> 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;

I'm not against the patch, but I'm in doubt, looking into this example, it's useful.
Any real use case like above in the Linux kernel, please?

>           list_for_each_entry((*t2), &testlist, node) {   /* works */
>                   //...
>           }
>           list_for_each_entry(*t2, &testlist, node) {     /* broken */
>                   //...
>           }
>   }

-- 
With Best Regards,
Andy Shevchenko

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

[Mathieu Desnoyers]

On 2023-05-04 10:41, Andy Shevchenko wrote:
> On Wed, May 03, 2023 at 09:29:12PM -0400, Mathieu Desnoyers wrote:
>> Add missing parentheses around use of macro argument "pos" in those
>> patterns to ensure operator precedence behaves as expected:
>>
>> - typeof(*pos)
>> - pos->member
>>
>> Remove useless parentheses around use of macro parameter (head) in the
>> following pattern:
>>
>> - list_is_head(pos, (head))
>>
>> Because comma is the lowest priority operator already, so the extra pair
>> of parentheses is redundant.
>>
>> 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;
> 
> I'm not against the patch, but I'm in doubt, looking into this example, it's useful.
> Any real use case like above in the Linux kernel, please?

There aren't because the code would not compile with the current header 
implementation. But it's unexpected that this kind of pattern does not work.

It's not about being useful, but rather about eliminating unexpected 
operator precedence within macros, and about being consistent everywhere.

Thanks,

Mathieu

> 
>>            list_for_each_entry((*t2), &testlist, node) {   /* works */
>>                    //...
>>            }
>>            list_for_each_entry(*t2, &testlist, node) {     /* broken */
>>                    //...
>>            }
>>    }
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

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

[Mathieu Desnoyers]

On 2023-05-04 01:54, Huang, Ying wrote:
> Hi, Mathieu,
> 
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> writes:
> 
>> Add missing parentheses around use of macro argument "pos" in those
>> patterns to ensure operator precedence behaves as expected:
>>
>> - typeof(*pos)
>> - pos->member
>>
>> 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.
> 
> I don't think it's necessary to add parentheses here.  As you said,
> "pos" is used as an lvalue.

I agree that it's not strictly necessary to add the parentheses around
"pos" in typeof(*pos) when pos is also used as an lvalue within a macro,
but if we look at what happened in list.h, we can see why having a consistent
pattern is good to eliminate issues as the code evolves.

When code from list_for_each_entry_continue was lifted into
list_prepare_entry(), we had a situation where "pos" was initially used
as an lvalue in the original macro, but not in list_prepare_entry(), for
which the parentheses were relevant.

This example is from the pre-git era, in tglx's history tree:

commit a3500b9e955d47891e57587c30006de83a3591f5
Author: Linus Torvalds <torvalds@home.osdl.org>
Date:   Wed Feb 11 21:00:34 2004 -0800

     Fix "bus_for_each_dev()" and "bus_for_each_drv()", which did not
     correctly handle the "restart from this device/driver" case, and
     caused oopses with ieee1394.
     
     This just uses "list_for_each_entry_continue()" instead.
     
     Add helper macro to make usage of "list_for_each_entry_continue()"
     a bit more readable.

[...]

+/**
+ * list_prepare_entry - prepare a pos entry for use as a start point in
+ *                     list_for_each_entry_continue
+ * @pos:       the type * to use as a start point
+ * @head:      the head of the list
+ * @member:    the name of the list_struct within the struct.
+ */
+#define list_prepare_entry(pos, head, member) \
+       ((pos) ? : list_entry(head, typeof(*pos), member))

So even though the fact that "pos" is used as an lvalue specifically in
llist_for_each_entry_safe() makes it so that the parentheses are not
strictly required around "pos" in typeof(*pos), I argue that we should
still add those for consistency.

> 
>> Remove useless parentheses around use of macro parameter (node) in the
>> following pattern:
>>
>>    llist_entry((node), typeof(*pos), member)
>>
>> Because comma is the lowest priority operator already, so the extra pair
>> of parentheses is redundant.
> 
> This change looks good for me.

Thanks,

Mathieu

> 
> Best Regards,
> Huang, Ying
> 
>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Huang Ying <ying.huang@intel.com>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> ---
>>   include/linux/llist.h | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/llist.h b/include/linux/llist.h
>> index 85bda2d02d65..45d358c15d0d 100644
>> --- a/include/linux/llist.h
>> +++ b/include/linux/llist.h
>> @@ -173,9 +173,9 @@ static inline void init_llist_head(struct llist_head *list)
>>    * reverse the order by yourself before traversing.
>>    */
>>   #define llist_for_each_entry_safe(pos, n, node, member)			       \
>> -	for (pos = llist_entry((node), typeof(*pos), member);		       \
>> +	for (pos = llist_entry(node, typeof(*(pos)), member);		       \
>>   	     member_address_is_nonnull(pos, member) &&			       \
>> -	        (n = llist_entry(pos->member.next, typeof(*n), member), true); \
>> +		(n = llist_entry((pos)->member.next, typeof(*(n)), member), true); \
>>   	     pos = n)
>>   
>>   /**

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

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@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
>

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

[Linus Torvalds]

On Thu, May 4, 2023 at 7:54 AM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
> +#define list_prepare_entry(pos, head, member) \
> +       ((pos) ? : list_entry(head, typeof(*pos), member))
>
> So even though the fact that "pos" is used as an lvalue specifically in
> llist_for_each_entry_safe() makes it so that the parentheses are not
> strictly required around "pos" in typeof(*pos), I argue that we should
> still add those for consistency.

Ack. It may not matter in reality because of how 'pos' is supposed to
be just a local variable name, but I agree that for consistency our
macros should still follow the usual pattern.

Of course, *because* of how 'pos' is not some random expression, and
is supposed to be that local variable, and because of how it is used,
it must always violate the whole "only use once" usual pattern,.

Exactly the same way the member name is also typically used multiple
times because of how it's not an expression, but really a "part of the
syntax".

So an alternative might be that we should use a different syntax for
those things and make it clear that they are not normal expressions.
Some people use upper-case names for special things like that to make
them stand out as "not normal" (kind of the same way upper-case macros
themselves were a warning that they weren't normal and might evaluate
arguments multiple times).

                   Linus

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

[Huang, Ying]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Thu, May 4, 2023 at 7:54 AM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>> +#define list_prepare_entry(pos, head, member) \
>> +       ((pos) ? : list_entry(head, typeof(*pos), member))
>>
>> So even though the fact that "pos" is used as an lvalue specifically in
>> llist_for_each_entry_safe() makes it so that the parentheses are not
>> strictly required around "pos" in typeof(*pos), I argue that we should
>> still add those for consistency.
>
> Ack. It may not matter in reality because of how 'pos' is supposed to
> be just a local variable name, but I agree that for consistency our
> macros should still follow the usual pattern.
>
> Of course, *because* of how 'pos' is not some random expression, and
> is supposed to be that local variable, and because of how it is used,
> it must always violate the whole "only use once" usual pattern,.
>
> Exactly the same way the member name is also typically used multiple
> times because of how it's not an expression, but really a "part of the
> syntax".
>
> So an alternative might be that we should use a different syntax for
> those things and make it clear that they are not normal expressions.
> Some people use upper-case names for special things like that to make
> them stand out as "not normal" (kind of the same way upper-case macros
> themselves were a warning that they weren't normal and might evaluate
> arguments multiple times).

This sounds a good idea for me.

And with this, I think that we will use typeof(*POS) instead of
typeof(*(pos))?

Best Regards,
Huang, Ying

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

[Steven Rostedt]

On Thu, 4 May 2023 12:19:38 -0400
Joel Fernandes <joel@joelfernandes.org> wrote:

> >   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 ;).

Remember interfaces should not be enforcing policy unless it's key to the
way the interface works.

-- Steve

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

[Mathieu Desnoyers]

On 2023-05-04 13:16, Linus Torvalds wrote:
> On Thu, May 4, 2023 at 7:54 AM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>> +#define list_prepare_entry(pos, head, member) \
>> +       ((pos) ? : list_entry(head, typeof(*pos), member))
>>
>> So even though the fact that "pos" is used as an lvalue specifically in
>> llist_for_each_entry_safe() makes it so that the parentheses are not
>> strictly required around "pos" in typeof(*pos), I argue that we should
>> still add those for consistency.
> 
> Ack. It may not matter in reality because of how 'pos' is supposed to
> be just a local variable name, but I agree that for consistency our
> macros should still follow the usual pattern.
> 
> Of course, *because* of how 'pos' is not some random expression, and
> is supposed to be that local variable, and because of how it is used,
> it must always violate the whole "only use once" usual pattern,.
> 
> Exactly the same way the member name is also typically used multiple
> times because of how it's not an expression, but really a "part of the
> syntax".
> 
> So an alternative might be that we should use a different syntax for
> those things and make it clear that they are not normal expressions.
> Some people use upper-case names for special things like that to make
> them stand out as "not normal" (kind of the same way upper-case macros
> themselves were a warning that they weren't normal and might evaluate
> arguments multiple times).

Is a list iteration position absolutely required to be a local variable,
or can it be a dereferenced pointer ?

Let's say we take "list_for_each()" as example:

#define list_for_each(pos, head) \
         for (pos = (head)->next; !list_is_head(pos, head); pos = pos->next)

and turn "pos" into "POS" to make it clearer that it is used as an lvalue:

#define list_for_each(POS, head) \
         for (POS = (head)->next; !list_is_head(POS, head); pos = POS->next)

The following usage pattern is still broken:

struct list_head list;

void f(struct list_head **ppos)
{
   list_for_each(*ppos, &list) {
     //...
   }
}

Because ->next has higher operator precedence than "*", which is unexpected.

I would argue that even if we choose to capitalize the macro special arguments used
as lvalues and as member names so they stand out, it does not eliminate the need to
be careful about proper use of parentheses around those parameters when they are also
used as rvalues.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

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

[Joel Fernandes]

> On May 5, 2023, at 10:06 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> On Thu, 4 May 2023 12:19:38 -0400
> Joel Fernandes <joel@joelfernandes.org> wrote:
> 
>>>  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 ;).
> 
> Remember interfaces should not be enforcing policy unless it's key to the
> way the interface works.

Oh yeah, 100% agree. I am not particularly against this particular patch but I also dont see it as solving any problem. Feel free to Ack the patch if you feel strongly about wanting it.

 - Joel


> 
> -- Steve

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

[Steven Rostedt]

On Fri, 5 May 2023 10:35:39 -0400
Joel Fernandes <joel@joelfernandes.org> wrote:

> >> 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 ;).  
> > 
> > Remember interfaces should not be enforcing policy unless it's key to the
> > way the interface works.  
> 
> Oh yeah, 100% agree. I am not particularly against this particular patch
> but I also dont see it as solving any problem. Feel free to Ack the patch
> if you feel strongly about wanting it.

I agree that this isn't solving any real bugs, but it is a legitimate
cleanup.

And as someone that tends to push interfaces to their limits, I would
likely be the one that hits it ;-)

Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org>

-- Steve

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

[Linus Torvalds]

On Fri, May 5, 2023 at 7:23 AM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> Is a list iteration position absolutely required to be a local variable,
> or can it be a dereferenced pointer ?

Well, it was certainly the intention, but while the member name
obviously has to be a member name, the iterator *could* be any lvalue.

Many of the "foreach" kind of loops would actually prefer to have
entirely local variables, but C syntax rules didn't allow that (now
with C11 we can do that variable declaration in the for-loop itself,
but we're still limited to just _one_ variable which can be a
problem).

So if we had started with C11, that 'list_for_each()' wouldn't have
had an external 'pos' declaration at all, it would have done it
internally, but that's not the reality we're in today ;/

              Linus

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

[Huang, Ying]

Mathieu Desnoyers <mathieu.desnoyers@efficios.com> writes:

> On 2023-05-04 13:16, Linus Torvalds wrote:
>> On Thu, May 4, 2023 at 7:54/AM Mathieu Desnoyers
>> <mathieu.desnoyers@efficios.com> wrote:
>>> +#define list_prepare_entry(pos, head, member) \
>>> +       ((pos) ? : list_entry(head, typeof(*pos), member))
>>>
>>> So even though the fact that "pos" is used as an lvalue specifically in
>>> llist_for_each_entry_safe() makes it so that the parentheses are not
>>> strictly required around "pos" in typeof(*pos), I argue that we should
>>> still add those for consistency.
>> Ack. It may not matter in reality because of how 'pos' is supposed
>> to
>> be just a local variable name, but I agree that for consistency our
>> macros should still follow the usual pattern.
>> Of course, *because* of how 'pos' is not some random expression, and
>> is supposed to be that local variable, and because of how it is used,
>> it must always violate the whole "only use once" usual pattern,.
>> Exactly the same way the member name is also typically used multiple
>> times because of how it's not an expression, but really a "part of the
>> syntax".
>> So an alternative might be that we should use a different syntax for
>> those things and make it clear that they are not normal expressions.
>> Some people use upper-case names for special things like that to make
>> them stand out as "not normal" (kind of the same way upper-case macros
>> themselves were a warning that they weren't normal and might evaluate
>> arguments multiple times).
>
> Is a list iteration position absolutely required to be a local variable,
> or can it be a dereferenced pointer ?
>
> Let's say we take "list_for_each()" as example:
>
> #define list_for_each(pos, head) \
>         for (pos = (head)->next; !list_is_head(pos, head); pos = pos->next)
>
> and turn "pos" into "POS" to make it clearer that it is used as an lvalue:
>
> #define list_for_each(POS, head) \
>         for (POS = (head)->next; !list_is_head(POS, head); pos = POS->next)
>
> The following usage pattern is still broken:
>
> struct list_head list;
>
> void f(struct list_head **ppos)
> {
>   list_for_each(*ppos, &list) {
>     //...
>   }
> }
>
> Because ->next has higher operator precedence than "*", which is unexpected.
>
> I would argue that even if we choose to capitalize the macro special arguments used
> as lvalues and as member names so they stand out, it does not eliminate the need to
> be careful about proper use of parentheses around those parameters when they are also
> used as rvalues.

Yes.  The special parameter isn't necessary a variable name (except the
member name).  So special parameters

- are lvalue or member name
- may be "evaluated" multiple times

This makes them special enough from other macro parameters.  And we
still need to be careful about them (for example, when used as rvalue)
as other macro parameters.

Best Regards,
Huang, Ying