Re: [PATCH 18/18] arm64: lto: Strengthen READ_ONCE() to acquire when CLANG_LTO=y
On Tue, Jul 07, 2020 at 04:01:28PM -0700, Nick Desaulniers wrote: > I'm trying to put together a Micro Conference for Linux Plumbers > conference focused on "make LLVM slightly less shitty." Do you all > plan on attending the conference? Would it be worthwhile to hold a > session focused on discussing this (LTO and memory models) be > worthwhile? I'd love to have a session about compilers and memory ordering with both GCC and CLANG in attendance. We need a solution for dependent-loads and control-dependencies for both toolchains. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 18/18] arm64: lto: Strengthen READ_ONCE() to acquire when CLANG_LTO=y
I'm trying to put together a Micro Conference for Linux Plumbers conference focused on "make LLVM slightly less shitty." Do you all plan on attending the conference? Would it be worthwhile to hold a session focused on discussing this (LTO and memory models) be worthwhile? On Tue, Jul 7, 2020 at 3:51 PM Paul E. McKenney wrote: > > On Tue, Jul 07, 2020 at 11:29:15AM +0100, Dave Martin wrote: > > On Mon, Jul 06, 2020 at 10:36:28AM -0700, Paul E. McKenney wrote: > > > On Mon, Jul 06, 2020 at 06:05:57PM +0100, Dave Martin wrote: > > [ . . . ] > > > > > The underlying problem here seems to be that the necessary ordering > > > > rule is not part of what passes for the C memory model prior to C11. > > > > If we want to control the data flow, don't we have to wrap the entire > > > > dereference in a macro? > > > > > > Yes, exactly. Because we are relying on things that are not guaranteed > > > by the C memory model, we need to pay attention to the implementations. > > > As I have said elsewhere, the price of control dependencies is eternal > > > vigilance. > > > > > > And this also applies, to a lesser extent, to address and data > > > dependencies, which are also not well supported by the C standard. > > > > > > There is one important case in which the C memory model -does- support > > > control dependencies, and that is when the dependent write is a normal > > > C-language write that is not involved in a data race. In that case, > > > if the compiler broke the control dependency, it might have introduced > > > a data race, which it is forbidden to do. However, this rule can also > > > be broken when the compiler knows too much, as it might be able to prove > > > that breaking the dependency won't introduce a data race. In that case, > > > according to the standard, it is free to break the dependency. > > > > Which only matters because the C abstract machine may not match reality. > > > > LTO has no bearing on the abstract machine though. > > > > If specific compiler options etc. can be added to inhibit the > > problematic optimisations, that would be ideal. I guess that can't > > happen overnight though. > > Sadly, I must agree. > > > > > > > > We likely won't realise if/when this goes wrong, other than > > > > > > > impossible to > > > > > > > debug, subtle breakage that crops up seemingly randomly. Ideally, > > > > > > > we'd be > > > > > > > able to detect this sort of thing happening at build time, and > > > > > > > perhaps > > > > > > > even prevent it with compiler options or annotations, but none of > > > > > > > that is > > > > > > > close to being available and I'm keen to progress the LTO patches > > > > > > > in the > > > > > > > meantime because they are a requirement for CFI. > > > > > > > > > > > > My concern was not so much why LTO makes things dangerous, as why > > > > > > !LTO > > > > > > makes things safe... > > > > > > > > > > Because ignorant compilers are safe compilers! ;-) > > > > > > > > AFAICT ignorance is no gurantee of ordering in general -- the compiler > > > > is free to speculatively invent knowledge any place that the language > > > > spec allows it to. !LTO doesn't stop this happening. > > > > > > Agreed, according to the standard, the compiler has great freedom. > > > > > > We have two choices: (1) Restrict ourselves to live within the confines of > > > the standard or (2) Pay continued close attention to the implementation. > > > We have made different choices at different times, but for many ordering > > > situations we have gone with door #2. > > > > > > Me, I have been working to get the standard to better support our > > > use case. This is at best slow going. But don't take my word for it, > > > ask Will. > > > > I can believe it. They want to enable optimisations rather than prevent > > them... > > Right in one! ;-) > > > > > Hopefully some of the knowledge I invented in my reply is valid... > > > > > > It is. It is just that there are multiple valid strategies, and the > > > Linux kernel is currently taking a mixed-strategy approach. > > > > Ack. The hope that there is a correct way to fix everything dies > > hard ;) > > Either that, or one slowly degrades ones definition of "correct". :-/ > > > Life was cosier before I started trying to reason about language specs. > > Same here! > > Thanx, Paul -- Thanks, ~Nick Desaulniers ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 18/18] arm64: lto: Strengthen READ_ONCE() to acquire when CLANG_LTO=y
On Mon, Jul 06, 2020 at 05:08:20PM +0100, Dave Martin wrote: > On Tue, Jun 30, 2020 at 06:37:34PM +0100, Will Deacon wrote: > > diff --git a/arch/arm64/include/asm/rwonce.h > > b/arch/arm64/include/asm/rwonce.h > > new file mode 100644 > > index ..515e360b01a1 > > --- /dev/null > > +++ b/arch/arm64/include/asm/rwonce.h > > @@ -0,0 +1,63 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Copyright (C) 2020 Google LLC. > > + */ > > +#ifndef __ASM_RWONCE_H > > +#define __ASM_RWONCE_H > > + > > +#ifdef CONFIG_CLANG_LTO > > + > > +#include > > +#include > > + > > +#ifndef BUILD_VDSO > > + > > +#ifdef CONFIG_AS_HAS_LDAPR > > +#define __LOAD_RCPC(sfx, regs...) \ > > + ALTERNATIVE(\ > > + "ldar" #sfx "\t" #regs,\ > > ^ Should this be here? It seems that READ_ONCE() will actually read > twice... even if that doesn't actually conflict with the required > semantics of READ_ONCE(), it looks odd. It's patched at runtime, so it's either LDAR or LDAPR. > Making a direct link between LTO and the memory model also seems highly > spurious (as discussed in the other subthread) so can we have a comment > explaining the reasoning? Sure, although like I say, this is more about helping to progress that conversation. Will ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 18/18] arm64: lto: Strengthen READ_ONCE() to acquire when CLANG_LTO=y
On Mon, Jul 06, 2020 at 05:00:23PM +0100, Dave Martin wrote: > On Thu, Jul 02, 2020 at 08:23:02AM +0100, Will Deacon wrote: > > On Wed, Jul 01, 2020 at 06:07:25PM +0100, Dave P Martin wrote: > > > Also, can you illustrate code that can only be unsafe with Clang LTO? > > > > I don't have a concrete example, but it's an ongoing concern over on the LTO > > thread [1], so I cooked this to show one way we could deal with it. The main > > concern is that the whole-program optimisations enabled by LTO may allow the > > compiler to enumerate possible values for a pointer at link time and replace > > an address dependency between two loads with a control dependency instead, > > defeating the dependency ordering within the CPU. > > Why can't that happen without LTO? It could, but I'd argue that it's considerably less likely because there is less information available to the compiler to perform these sorts of optimisations. It also doesn't appear to be happening in practice. The current state of affairs is that, if/when we catch the compiler performing harmful optimistations, we look for a way to disable them. However, there are good reasons to enable LTO, so this is one way to do that without having to worry about the potential impact on dependency ordering. Will ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 18/18] arm64: lto: Strengthen READ_ONCE() to acquire when CLANG_LTO=y
On Wed, Jul 01, 2020 at 06:07:25PM +0100, Dave P Martin wrote: > On Tue, Jun 30, 2020 at 06:37:34PM +0100, Will Deacon wrote: > > When building with LTO, there is an increased risk of the compiler > > converting an address dependency headed by a READ_ONCE() invocation > > into a control dependency and consequently allowing for harmful > > reordering by the CPU. > > > > Ensure that such transformations are harmless by overriding the generic > > READ_ONCE() definition with one that provides acquire semantics when > > building with LTO. > > > > Signed-off-by: Will Deacon > > --- > > arch/arm64/include/asm/rwonce.h | 63 +++ > > arch/arm64/kernel/vdso/Makefile | 2 +- > > arch/arm64/kernel/vdso32/Makefile | 2 +- > > 3 files changed, 65 insertions(+), 2 deletions(-) > > create mode 100644 arch/arm64/include/asm/rwonce.h > > > > diff --git a/arch/arm64/include/asm/rwonce.h > > b/arch/arm64/include/asm/rwonce.h > > new file mode 100644 > > index ..515e360b01a1 > > --- /dev/null > > +++ b/arch/arm64/include/asm/rwonce.h > > @@ -0,0 +1,63 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Copyright (C) 2020 Google LLC. > > + */ > > +#ifndef __ASM_RWONCE_H > > +#define __ASM_RWONCE_H > > + > > +#ifdef CONFIG_CLANG_LTO > > Don't we have a generic option for LTO that's not specific to Clang. /me looks at the LTO series some more Oh yeah, there's CONFIG_LTO which is selected by CONFIG_LTO_CLANG, which is the non-typoed version of the above. I can switch this to CONFIG_LTO. > Also, can you illustrate code that can only be unsafe with Clang LTO? I don't have a concrete example, but it's an ongoing concern over on the LTO thread [1], so I cooked this to show one way we could deal with it. The main concern is that the whole-program optimisations enabled by LTO may allow the compiler to enumerate possible values for a pointer at link time and replace an address dependency between two loads with a control dependency instead, defeating the dependency ordering within the CPU. We likely won't realise if/when this goes wrong, other than impossible to debug, subtle breakage that crops up seemingly randomly. Ideally, we'd be able to detect this sort of thing happening at build time, and perhaps even prevent it with compiler options or annotations, but none of that is close to being available and I'm keen to progress the LTO patches in the meantime because they are a requirement for CFI. Will [1] https://lore.kernel.org/r/20200624203200.78870-1-samitolva...@google.com ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 18/18] arm64: lto: Strengthen READ_ONCE() to acquire when CLANG_LTO=y
On Wed, Jul 1, 2020 at 12:19 PM Will Deacon wrote: > On Tue, Jun 30, 2020 at 09:25:03PM +0200, Arnd Bergmann wrote: > > On Tue, Jun 30, 2020 at 7:39 PM Will Deacon wrote: > > Once we make gcc-4.9 the minimum version, > > this could be further improved to > > > >__auto_type __x = &(x); > > Is anybody working on moving to 4.9? I've seen the mails from Linus > championing it, but I thought there was a RHEL in support that people > might care about? I don't think there was a serious discussion about it so far, and we only just moved to gcc-4.8. I think moving to gnu11 (gcc-4.9 or clang) instead of gnu99 has other benefits as well, so we may well want to do it anyway when something else comes up. For __auto_type(), we could do it like #if (clang or gcc-4.9+) #define auto_typeof(x) __auto_type #else #define auto_typeof(x) typeof(x) #endif which could be used in a lot of macros. Arnd ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 18/18] arm64: lto: Strengthen READ_ONCE() to acquire when CLANG_LTO=y
On Tue, Jun 30, 2020 at 03:57:54PM -0700, Sami Tolvanen wrote: > On Tue, Jun 30, 2020 at 12:47 PM Marco Elver wrote: > > > > On Tue, 30 Jun 2020 at 19:39, Will Deacon wrote: > > > > > > When building with LTO, there is an increased risk of the compiler > > > converting an address dependency headed by a READ_ONCE() invocation > > > into a control dependency and consequently allowing for harmful > > > reordering by the CPU. > > > > > > Ensure that such transformations are harmless by overriding the generic > > > READ_ONCE() definition with one that provides acquire semantics when > > > building with LTO. > > > > > > Signed-off-by: Will Deacon > > > --- > > > arch/arm64/include/asm/rwonce.h | 63 +++ > > > arch/arm64/kernel/vdso/Makefile | 2 +- > > > arch/arm64/kernel/vdso32/Makefile | 2 +- > > > 3 files changed, 65 insertions(+), 2 deletions(-) > > > create mode 100644 arch/arm64/include/asm/rwonce.h > > > > This seems reasonable, given we can't realistically tell the compiler > > about dependent loads. What (if any), is the performance impact? I > > guess this also heavily depends on the actual silicon. > > > > I do wonder, though, if there is some way to make the compiler do > > something better for us. Clearly, implementing real > > memory_order_consume hasn't worked out until today. But maybe the > > compiler could promote dependent loads to acquires if it recognizes it > > lost dependencies during optimizations. Just thinking out loud, it > > probably still has some weird corner case that will break. ;-) > > > > The other thing is that I'd be cautious blaming LTO, as I tried to > > summarize here: > > https://lore.kernel.org/kernel-hardening/20200630191931.ga884...@elver.google.com/ > > > > The main thing is that, yes, this might be something to be worried > > about, but if we are worried about it, we need to be worried about it > > in *all* builds (LTO or not). My guess is that's not acceptable. Would > > it be better to just guard the promotion of READ_ONCE() to acquire > > behind a config option like CONFIG_ACQUIRE_READ_DEPENDENCIES, and then > > make LTO select that (or maybe leave it optional?). In future, for > > very aggressive non-LTO compilers even, one may then also select that > > if there is substantiated worry things do actually break. > > I agree, a separate config option would be better here. > > Also Will, the LTO patches use CONFIG_LTO_CLANG instead of CLANG_LTO. D'oh, sorry. I'll fix that (I had that #ifdef commented out for my testing). Will ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 18/18] arm64: lto: Strengthen READ_ONCE() to acquire when CLANG_LTO=y
On Tue, Jun 30, 2020 at 09:47:30PM +0200, Marco Elver wrote: > On Tue, 30 Jun 2020 at 19:39, Will Deacon wrote: > > > > When building with LTO, there is an increased risk of the compiler > > converting an address dependency headed by a READ_ONCE() invocation > > into a control dependency and consequently allowing for harmful > > reordering by the CPU. > > > > Ensure that such transformations are harmless by overriding the generic > > READ_ONCE() definition with one that provides acquire semantics when > > building with LTO. > > > > Signed-off-by: Will Deacon > > --- > > arch/arm64/include/asm/rwonce.h | 63 +++ > > arch/arm64/kernel/vdso/Makefile | 2 +- > > arch/arm64/kernel/vdso32/Makefile | 2 +- > > 3 files changed, 65 insertions(+), 2 deletions(-) > > create mode 100644 arch/arm64/include/asm/rwonce.h > > This seems reasonable, given we can't realistically tell the compiler > about dependent loads. What (if any), is the performance impact? I > guess this also heavily depends on the actual silicon. Right, it depends both on the CPU micro-architecture and also the workload. When we ran some basic tests, the overhead wasn't greater than the benefit seen by enabling LTO, so it seems like a reasonable trade-off (given that LTO is a dependency for CFI, so it's not just about performance). Will ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 18/18] arm64: lto: Strengthen READ_ONCE() to acquire when CLANG_LTO=y
On Tue, Jun 30, 2020 at 09:25:03PM +0200, Arnd Bergmann wrote:
> On Tue, Jun 30, 2020 at 7:39 PM Will Deacon wrote:
> > +#define __READ_ONCE(x) \
> > +({ \
> > + int atomic = 1; \
> > + union { __unqual_scalar_typeof(x) __val; char __c[1]; } __u;\
> > + typeof(&(x)) __x = &(x);\
> > + switch (sizeof(x)) {\
> ...
> > + atomic ? (typeof(x))__u.__val : (*(volatile typeof(x) *)__x); \
> > +})
>
> This expands (x) nine times (five in __unqual_scala_typeof()), which can
> lead to significant code bloat after preprocessing if something passes a
> compound expression into READ_ONCE().
> The compiler works it out eventually, but we've seen an actual slowdown
> in compile speed from this recently, especially on clang.
>
> I think if you move the
>
> typeof(&(x)) __x = &(x);
>
> line first, all other instances can use typeof(*__x) instead of typeof(x)
> and avoid this problem.
Cheers, I was only thinking about side-effects when I wrote this, but
bloating built time is very unpopular, so I'll go with your suggestion.
> Once we make gcc-4.9 the minimum version,
> this could be further improved to
>
>__auto_type __x = &(x);
Is anybody working on moving to 4.9? I've seen the mails from Linus
championing it, but I thought there was a RHEL in support that people
might care about?
Will
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 18/18] arm64: lto: Strengthen READ_ONCE() to acquire when CLANG_LTO=y
On Tue, Jun 30, 2020 at 09:47:30PM +0200, Marco Elver wrote: > I do wonder, though, if there is some way to make the compiler do > something better for us. Clearly, implementing real > memory_order_consume hasn't worked out until today. But maybe the > compiler could promote dependent loads to acquires if it recognizes it > lost dependencies during optimizations. Just thinking out loud, it > probably still has some weird corner case that will break. ;-) I'd be very hesitant to let the compiler upgrade the ordering for us, specifically because we're not using C11 crud and are using a lot of inline asm. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 18/18] arm64: lto: Strengthen READ_ONCE() to acquire when CLANG_LTO=y
On Tue, Jun 30, 2020 at 7:39 PM Will Deacon wrote:
> +#define __READ_ONCE(x) \
> +({ \
> + int atomic = 1; \
> + union { __unqual_scalar_typeof(x) __val; char __c[1]; } __u;\
> + typeof(&(x)) __x = &(x);\
> + switch (sizeof(x)) {\
...
> + atomic ? (typeof(x))__u.__val : (*(volatile typeof(x) *)__x); \
> +})
This expands (x) nine times (five in __unqual_scala_typeof()), which can
lead to significant code bloat after preprocessing if something passes a
compound expression into READ_ONCE().
The compiler works it out eventually, but we've seen an actual slowdown
in compile speed from this recently, especially on clang.
I think if you move the
typeof(&(x)) __x = &(x);
line first, all other instances can use typeof(*__x) instead of typeof(x)
and avoid this problem. Once we make gcc-4.9 the minimum version,
this could be further improved to
__auto_type __x = &(x);
Arnd
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 18/18] arm64: lto: Strengthen READ_ONCE() to acquire when CLANG_LTO=y
When building with LTO, there is an increased risk of the compiler
converting an address dependency headed by a READ_ONCE() invocation
into a control dependency and consequently allowing for harmful
reordering by the CPU.
Ensure that such transformations are harmless by overriding the generic
READ_ONCE() definition with one that provides acquire semantics when
building with LTO.
Signed-off-by: Will Deacon
---
arch/arm64/include/asm/rwonce.h | 63 +++
arch/arm64/kernel/vdso/Makefile | 2 +-
arch/arm64/kernel/vdso32/Makefile | 2 +-
3 files changed, 65 insertions(+), 2 deletions(-)
create mode 100644 arch/arm64/include/asm/rwonce.h
diff --git a/arch/arm64/include/asm/rwonce.h b/arch/arm64/include/asm/rwonce.h
new file mode 100644
index ..515e360b01a1
--- /dev/null
+++ b/arch/arm64/include/asm/rwonce.h
@@ -0,0 +1,63 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2020 Google LLC.
+ */
+#ifndef __ASM_RWONCE_H
+#define __ASM_RWONCE_H
+
+#ifdef CONFIG_CLANG_LTO
+
+#include
+#include
+
+#ifndef BUILD_VDSO
+
+#ifdef CONFIG_AS_HAS_LDAPR
+#define __LOAD_RCPC(sfx, regs...) \
+ ALTERNATIVE(\
+ "ldar" #sfx "\t" #regs,\
+ ".arch_extension rcpc\n"\
+ "ldapr" #sfx "\t" #regs,\
+ ARM64_HAS_LDAPR)
+#else
+#define __LOAD_RCPC(sfx, regs...) "ldar" #sfx "\t" #regs
+#endif /* CONFIG_AS_HAS_LDAPR */
+
+#define __READ_ONCE(x) \
+({ \
+ int atomic = 1; \
+ union { __unqual_scalar_typeof(x) __val; char __c[1]; } __u;\
+ typeof(&(x)) __x = &(x);\
+ switch (sizeof(x)) {\
+ case 1: \
+ asm volatile(__LOAD_RCPC(b, %w0, %1)\
+ : "=r" (*(__u8 *)__u.__c) \
+ : "Q" (*__x) : "memory"); \
+ break; \
+ case 2: \
+ asm volatile(__LOAD_RCPC(h, %w0, %1)\
+ : "=r" (*(__u16 *)__u.__c) \
+ : "Q" (*__x) : "memory"); \
+ break; \
+ case 4: \
+ asm volatile(__LOAD_RCPC(, %w0, %1) \
+ : "=r" (*(__u32 *)__u.__c) \
+ : "Q" (*__x) : "memory"); \
+ break; \
+ case 8: \
+ asm volatile(__LOAD_RCPC(, %0, %1) \
+ : "=r" (*(__u64 *)__u.__c) \
+ : "Q" (*__x) : "memory"); \
+ break; \
+ default:\
+ atomic = 0; \
+ } \
+ atomic ? (typeof(x))__u.__val : (*(volatile typeof(x) *)__x); \
+})
+
+#endif /* !BUILD_VDSO */
+#endif /* CONFIG_CLANG_LTO */
+
+#include
+
+#endif /* __ASM_RWONCE_H */
diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile
index 45d5cfe46429..60df97f2e7de 100644
--- a/arch/arm64/kernel/vdso/Makefile
+++ b/arch/arm64/kernel/vdso/Makefile
@@ -28,7 +28,7 @@ ldflags-y := -shared -nostdlib -soname=linux-vdso.so.1
--hash-style=sysv \
$(btildflags-y) -T
ccflags-y := -fno-common -fno-builtin -fno-stack-protector -ffixed-x18
-ccflags-y += -DDISABLE_BRANCH_PROFILING
+ccflags-y += -DDISABLE_BRANCH_PROFILING -DBUILD_VDSO
CFLAGS_REMOVE_vgettimeofday.o = $(CC_FLAGS_FTRACE) -Os $(CC_FLAGS_SCS)
$(GCC_PLUGINS_CFLAGS)
KBUILD_CFLAGS += $(DISABLE_LTO)
diff --git a/arch/arm64/kernel/vdso32/Makefile
b/arch/arm64/kernel/vdso32/Makefile
index d88148bef6b0..4fdf3754a058 100644
--- a/arch/arm64/kernel/vdso32/Makefile
+++ b/arch/arm64/kernel/vdso32/Makefile
@@ -43,7 +43,7 @@ cc32-as-instr = $(call try-run,\
# As a result we set our own flags here.
# KBUILD_CPPFLAGS and NOSTDINC_FLAGS from top-level Makefile
-VDSO_CPPFLAGS := -D__KERNEL__ -nostdinc -isystem
