| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 4 May 2023 12:19:38 -0400
From: Joel Fernandes <joel@...lfernandes.org>
To: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
linux-kernel@...r.kernel.org,
"Paul E. McKenney" <paulmck@...nel.org>,
Frederic Weisbecker <frederic@...nel.org>,
Neeraj Upadhyay <quic_neeraju@...cinc.com>,
Josh Triplett <josh@...htriplett.org>,
Boqun Feng <boqun.feng@...il.com>,
Steven Rostedt <rostedt@...dmis.org>,
Lai Jiangshan <jiangshanlai@...il.com>,
Zqiang <qiang1.zhang@...el.com>, rcu@...r.kernel.org
Subject: Re: [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@...icios.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@...icios.com>
> Cc: "Paul E. McKenney" <paulmck@...nel.org>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Frederic Weisbecker <frederic@...nel.org>
> Cc: Neeraj Upadhyay <quic_neeraju@...cinc.com>
> Cc: Joel Fernandes <joel@...lfernandes.org>
> Cc: Josh Triplett <josh@...htriplett.org>
> Cc: Boqun Feng <boqun.feng@...il.com>
> Cc: Steven Rostedt <rostedt@...dmis.org>
> Cc: Lai Jiangshan <jiangshanlai@...il.com>
> Cc: Zqiang <qiang1.zhang@...el.com>
> Cc: rcu@...r.kernel.org
> Cc: linux-kernel@...r.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
>
Powered by blists - more mailing lists