| 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
| ||
|
Message-ID: <02c63e79-ec8c-4d6a-9fcf-75f0e67ea242@rowland.harvard.edu>
Date: Sat, 28 Sep 2024 10:49:45 -0400
From: Alan Stern <stern@...land.harvard.edu>
To: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,
linux-kernel@...r.kernel.org,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
"Paul E. McKenney" <paulmck@...nel.org>,
Will Deacon <will@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
Boqun Feng <boqun.feng@...il.com>, John Stultz <jstultz@...gle.com>,
Neeraj Upadhyay <Neeraj.Upadhyay@....com>,
Frederic Weisbecker <frederic@...nel.org>,
Joel Fernandes <joel@...lfernandes.org>,
Josh Triplett <josh@...htriplett.org>,
Uladzislau Rezki <urezki@...il.com>,
Steven Rostedt <rostedt@...dmis.org>,
Lai Jiangshan <jiangshanlai@...il.com>,
Zqiang <qiang.zhang1211@...il.com>, Ingo Molnar <mingo@...hat.com>,
Waiman Long <longman@...hat.com>,
Mark Rutland <mark.rutland@....com>,
Thomas Gleixner <tglx@...utronix.de>,
Vlastimil Babka <vbabka@...e.cz>, maged.michael@...il.com,
Mateusz Guzik <mjguzik@...il.com>, Gary Guo <gary@...yguo.net>,
Jonas Oberhauser <jonas.oberhauser@...weicloud.com>,
rcu@...r.kernel.org, linux-mm@...ck.org, lkmm@...ts.linux.dev
Subject: Re: [PATCH 1/2] compiler.h: Introduce ptr_eq() to preserve address
dependency
On Sat, Sep 28, 2024 at 09:51:27AM -0400, Mathieu Desnoyers wrote:
> Compiler CSE and SSA GVN optimizations can cause the address dependency
> of addresses returned by rcu_dereference to be lost when comparing those
> pointers with either constants or previously loaded pointers.
>
> Introduce ptr_eq() to compare two addresses while preserving the address
> dependencies for later use of the address. It should be used when
> comparing an address returned by rcu_dereference().
>
> This is needed to prevent the compiler CSE and SSA GVN optimizations
> from replacing the registers holding @a or @b based on their
"Replacing" isn't the right word. What the compiler does is use one
rather than the other. Furthermore, the compiler can play these games
even with values that aren't in registers.
You should just say: "... from using @a (or @b) in places where the
source refers to @b (or @a) (based on the fact that after the
comparison, the two are known to be equal), which does not ..."
> equality, which does not preserve address dependencies and allows the
> following misordering speculations:
>
> - If @b is a constant, the compiler can issue the loads which depend
> on @a before loading @a.
> - If @b is a register populated by a prior load, weakly-ordered
> CPUs can speculate loads which depend on @a before loading @a.
It shouldn't matter whether @a and @b are constants, registers, or
anything else. All that matters is that the compiler uses the wrong
one, which allows weakly ordered CPUs to speculate loads you wouldn't
expect it to, based on the source code alone.
> The same logic applies with @a and @b swapped.
>
> The compiler barrier() is ineffective at fixing this issue.
> It does not prevent the compiler CSE from losing the address dependency:
>
> int fct_2_volatile_barriers(void)
> {
> int *a, *b;
>
> do {
> a = READ_ONCE(p);
> asm volatile ("" : : : "memory");
> b = READ_ONCE(p);
> } while (a != b);
> asm volatile ("" : : : "memory"); <----- barrier()
> return *b;
> }
>
> With gcc 14.2 (arm64):
>
> fct_2_volatile_barriers:
> adrp x0, .LANCHOR0
> add x0, x0, :lo12:.LANCHOR0
> .L2:
> ldr x1, [x0] <------ x1 populated by first load.
> ldr x2, [x0]
> cmp x1, x2
> bne .L2
> ldr w0, [x1] <------ x1 is used for access which should depend on b.
> ret
>
> On weakly-ordered architectures, this lets CPU speculation use the
> result from the first load to speculate "ldr w0, [x1]" before
> "ldr x2, [x0]".
> Based on the RCU documentation, the control dependency does not prevent
> the CPU from speculating loads.
>
> Suggested-by: Linus Torvalds <torvalds@...ux-foundation.org>
> Suggested-by: Boqun Feng <boqun.feng@...il.com>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
> Reviewed-by: Boqun Feng <boqun.feng@...il.com>
> Acked-by: "Paul E. McKenney" <paulmck@...nel.org>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> Cc: "Paul E. McKenney" <paulmck@...nel.org>
> Cc: Will Deacon <will@...nel.org>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Boqun Feng <boqun.feng@...il.com>
> Cc: Alan Stern <stern@...land.harvard.edu>
> Cc: John Stultz <jstultz@...gle.com>
> Cc: Neeraj Upadhyay <Neeraj.Upadhyay@....com>
> Cc: Linus Torvalds <torvalds@...ux-foundation.org>
> Cc: Boqun Feng <boqun.feng@...il.com>
> Cc: Frederic Weisbecker <frederic@...nel.org>
> Cc: Joel Fernandes <joel@...lfernandes.org>
> Cc: Josh Triplett <josh@...htriplett.org>
> Cc: Uladzislau Rezki <urezki@...il.com>
> Cc: Steven Rostedt <rostedt@...dmis.org>
> Cc: Lai Jiangshan <jiangshanlai@...il.com>
> Cc: Zqiang <qiang.zhang1211@...il.com>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: Waiman Long <longman@...hat.com>
> Cc: Mark Rutland <mark.rutland@....com>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Vlastimil Babka <vbabka@...e.cz>
> Cc: maged.michael@...il.com
> Cc: Mateusz Guzik <mjguzik@...il.com>
> Cc: Gary Guo <gary@...yguo.net>
> Cc: Jonas Oberhauser <jonas.oberhauser@...weicloud.com>
> Cc: rcu@...r.kernel.org
> Cc: linux-mm@...ck.org
> Cc: lkmm@...ts.linux.dev
> ---
> include/linux/compiler.h | 62 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
>
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 2df665fa2964..f26705c267e8 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -186,6 +186,68 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
> __asm__ ("" : "=r" (var) : "0" (var))
> #endif
>
> +/*
> + * Compare two addresses while preserving the address dependencies for
> + * later use of the address. It should be used when comparing an address
> + * returned by rcu_dereference().
> + *
> + * This is needed to prevent the compiler CSE and SSA GVN optimizations
> + * from replacing the registers holding @a or @b based on their
> + * equality, which does not preserve address dependencies and allows the
> + * following misordering speculations:
> + *
> + * - If @b is a constant, the compiler can issue the loads which depend
> + * on @a before loading @a.
> + * - If @b is a register populated by a prior load, weakly-ordered
> + * CPUs can speculate loads which depend on @a before loading @a.
> + *
> + * The same logic applies with @a and @b swapped.
This could be more concise, and it should be more general (along the
same lines as the description above).
> + *
> + * Return value: true if pointers are equal, false otherwise.
> + *
> + * The compiler barrier() is ineffective at fixing this issue. It does
> + * not prevent the compiler CSE from losing the address dependency:
> + *
> + * int fct_2_volatile_barriers(void)
> + * {
> + * int *a, *b;
> + *
> + * do {
> + * a = READ_ONCE(p);
> + * asm volatile ("" : : : "memory");
> + * b = READ_ONCE(p);
> + * } while (a != b);
> + * asm volatile ("" : : : "memory"); <-- barrier()
> + * return *b;
> + * }
> + *
> + * With gcc 14.2 (arm64):
> + *
> + * fct_2_volatile_barriers:
> + * adrp x0, .LANCHOR0
> + * add x0, x0, :lo12:.LANCHOR0
> + * .L2:
> + * ldr x1, [x0] <-- x1 populated by first load.
> + * ldr x2, [x0]
> + * cmp x1, x2
> + * bne .L2
> + * ldr w0, [x1] <-- x1 is used for access which should depend on b.
> + * ret
> + *
> + * On weakly-ordered architectures, this lets CPU speculation use the
> + * result from the first load to speculate "ldr w0, [x1]" before
> + * "ldr x2, [x0]".
> + * Based on the RCU documentation, the control dependency does not
> + * prevent the CPU from speculating loads.
IMO, this lengthy explanation is not needed in the source code. Just
refer interested readers to the commit description. You're repeating
the same text verbatim, after all.
(Or if you firmly believe that this explanation _does_ belong in the
code, then omit it from the commit description. There's no need to say
everything twice.)
Alan Stern
> + */
> +static __always_inline
> +int ptr_eq(const volatile void *a, const volatile void *b)
> +{
> + OPTIMIZER_HIDE_VAR(a);
> + OPTIMIZER_HIDE_VAR(b);
> + return a == b;
> +}
> +
> #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
>
> /**
> --
> 2.39.2
>
Powered by blists - more mailing lists