[PATCH] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE

[PATCH] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE

[Ard Biesheuvel]

Commit 3193c0836f203 ("bpf: Disable GCC -fgcse optimization for
___bpf_prog_run()") introduced a __no_fgcse macro that expands to a
function scope __attribute__((optimize("-fno-gcse"))), to disable a
GCC specific optimization that was causing trouble on x86 builds, and
was not expected to have any positive effect in the first place.

However, as the GCC manual documents, __attribute__((optimize))
is not for production use, and results in all other optimization
options to be forgotten for the function in question. This can
cause all kinds of trouble, but in one particular reported case,
it causes -fno-asynchronous-unwind-tables to be disregarded,
resulting in .eh_frame info to be emitted for the function
inadvertently.

This reverts commit 3193c0836f203, and instead, it disables the -fgcse
optimization for the entire source file, but only when building for
X86.

Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Arvind Sankar <nivedita@alum.mit.edu>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Kees Cook <keescook@chromium.org>
Fixes: 3193c0836f203 ("bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()")
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 include/linux/compiler-gcc.h   | 2 --
 include/linux/compiler_types.h | 4 ----
 kernel/bpf/Makefile            | 4 +++-
 kernel/bpf/core.c              | 2 +-
 4 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index d1e3c6896b71..5deb37024574 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -175,5 +175,3 @@
 #else
 #define __diag_GCC_8(s)
 #endif
-
-#define __no_fgcse __attribute__((optimize("-fno-gcse")))
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 6e390d58a9f8..ac3fa37a84f9 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -247,10 +247,6 @@ struct ftrace_likely_data {
 #define asm_inline asm
 #endif
 
-#ifndef __no_fgcse
-# define __no_fgcse
-#endif
-
 /* Are two types/vars the same type (ignoring qualifiers)? */
 #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
 
diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
index bdc8cd1b6767..02b58f44c479 100644
--- a/kernel/bpf/Makefile
+++ b/kernel/bpf/Makefile
@@ -1,6 +1,8 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-y := core.o
-CFLAGS_core.o += $(call cc-disable-warning, override-init)
+# ___bpf_prog_run() needs GCSE disabled on x86; see 3193c0836f203 for details
+cflags-core-$(CONFIG_X86) := -fno-gcse
+CFLAGS_core.o += $(call cc-disable-warning, override-init) $(cflags-core-y)
 
 obj-$(CONFIG_BPF_SYSCALL) += syscall.o verifier.o inode.o helpers.o tnum.o bpf_iter.o map_iter.o task_iter.o prog_iter.o
 obj-$(CONFIG_BPF_SYSCALL) += hashtab.o arraymap.o percpu_freelist.o bpf_lru_list.o lpm_trie.o map_in_map.o
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 9268d77898b7..55454d2278b1 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1369,7 +1369,7 @@ u64 __weak bpf_probe_read_kernel(void *dst, u32 size, const void *unsafe_ptr)
  *
  * Decode and execute eBPF instructions.
  */
-static u64 __no_fgcse ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
+static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
 {
 #define BPF_INSN_2_LBL(x, y)    [BPF_##x | BPF_##y] = &&x##_##y
 #define BPF_INSN_3_LBL(x, y, z) [BPF_##x | BPF_##y | BPF_##z] = &&x##_##y##_##z
-- 
2.17.1

Re: [PATCH] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE

[Nick Desaulniers]

On Tue, Oct 27, 2020 at 1:57 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Commit 3193c0836f203 ("bpf: Disable GCC -fgcse optimization for
> ___bpf_prog_run()") introduced a __no_fgcse macro that expands to a
> function scope __attribute__((optimize("-fno-gcse"))), to disable a
> GCC specific optimization that was causing trouble on x86 builds, and
> was not expected to have any positive effect in the first place.
>
> However, as the GCC manual documents, __attribute__((optimize))
> is not for production use, and results in all other optimization
> options to be forgotten for the function in question. This can
> cause all kinds of trouble, but in one particular reported case,
> it causes -fno-asynchronous-unwind-tables to be disregarded,
> resulting in .eh_frame info to be emitted for the function
> inadvertently.
>
> This reverts commit 3193c0836f203, and instead, it disables the -fgcse
> optimization for the entire source file, but only when building for
> X86.
>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Arvind Sankar <nivedita@alum.mit.edu>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Kees Cook <keescook@chromium.org>
> Fixes: 3193c0836f203 ("bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()")
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  include/linux/compiler-gcc.h   | 2 --
>  include/linux/compiler_types.h | 4 ----
>  kernel/bpf/Makefile            | 4 +++-
>  kernel/bpf/core.c              | 2 +-
>  4 files changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index d1e3c6896b71..5deb37024574 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -175,5 +175,3 @@
>  #else
>  #define __diag_GCC_8(s)
>  #endif
> -
> -#define __no_fgcse __attribute__((optimize("-fno-gcse")))
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 6e390d58a9f8..ac3fa37a84f9 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -247,10 +247,6 @@ struct ftrace_likely_data {
>  #define asm_inline asm
>  #endif
>
> -#ifndef __no_fgcse
> -# define __no_fgcse
> -#endif
> -
>  /* Are two types/vars the same type (ignoring qualifiers)? */
>  #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
>
> diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
> index bdc8cd1b6767..02b58f44c479 100644
> --- a/kernel/bpf/Makefile
> +++ b/kernel/bpf/Makefile
> @@ -1,6 +1,8 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-y := core.o
> -CFLAGS_core.o += $(call cc-disable-warning, override-init)
> +# ___bpf_prog_run() needs GCSE disabled on x86; see 3193c0836f203 for details
> +cflags-core-$(CONFIG_X86) := -fno-gcse

-fno-gcse is not recognized by clang and will produce
-Wignored-optimization-argument.  It should at least be wrapped in
cc-option, though since it's unlikely to ever not be compiler
specific, I think it might be ok to guard with `ifdef
CONFIG_CC_IS_GCC`.  Also, might we want to only do this for `ifndef
CONFIG_RETPOLINE`, based on 3193c0836f203?

Finally, this is going to disable GCSE for the whole translation unit,
which may be overkill.   Previously it was isolated to one function
definition.  You could lower the definition of the preprocessor define
into kernel/bpf/core.c to keep its use isolated as far as possible.

I'm fine with either approach, but we should avoid new warnings for
clang.  Thanks for the patch!

> +CFLAGS_core.o += $(call cc-disable-warning, override-init) $(cflags-core-y)
>
>  obj-$(CONFIG_BPF_SYSCALL) += syscall.o verifier.o inode.o helpers.o tnum.o bpf_iter.o map_iter.o task_iter.o prog_iter.o
>  obj-$(CONFIG_BPF_SYSCALL) += hashtab.o arraymap.o percpu_freelist.o bpf_lru_list.o lpm_trie.o map_in_map.o
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 9268d77898b7..55454d2278b1 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -1369,7 +1369,7 @@ u64 __weak bpf_probe_read_kernel(void *dst, u32 size, const void *unsafe_ptr)
>   *
>   * Decode and execute eBPF instructions.
>   */
> -static u64 __no_fgcse ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
> +static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
>  {
>  #define BPF_INSN_2_LBL(x, y)    [BPF_##x | BPF_##y] = &&x##_##y
>  #define BPF_INSN_3_LBL(x, y, z) [BPF_##x | BPF_##y | BPF_##z] = &&x##_##y##_##z
> --
> 2.17.1
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE

[Ard Biesheuvel]

On Tue, 27 Oct 2020 at 22:20, Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> On Tue, Oct 27, 2020 at 1:57 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > Commit 3193c0836f203 ("bpf: Disable GCC -fgcse optimization for
> > ___bpf_prog_run()") introduced a __no_fgcse macro that expands to a
> > function scope __attribute__((optimize("-fno-gcse"))), to disable a
> > GCC specific optimization that was causing trouble on x86 builds, and
> > was not expected to have any positive effect in the first place.
> >
> > However, as the GCC manual documents, __attribute__((optimize))
> > is not for production use, and results in all other optimization
> > options to be forgotten for the function in question. This can
> > cause all kinds of trouble, but in one particular reported case,
> > it causes -fno-asynchronous-unwind-tables to be disregarded,
> > resulting in .eh_frame info to be emitted for the function
> > inadvertently.
> >
> > This reverts commit 3193c0836f203, and instead, it disables the -fgcse
> > optimization for the entire source file, but only when building for
> > X86.
> >
> > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > Cc: Arvind Sankar <nivedita@alum.mit.edu>
> > Cc: Randy Dunlap <rdunlap@infradead.org>
> > Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Fixes: 3193c0836f203 ("bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()")
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  include/linux/compiler-gcc.h   | 2 --
> >  include/linux/compiler_types.h | 4 ----
> >  kernel/bpf/Makefile            | 4 +++-
> >  kernel/bpf/core.c              | 2 +-
> >  4 files changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> > index d1e3c6896b71..5deb37024574 100644
> > --- a/include/linux/compiler-gcc.h
> > +++ b/include/linux/compiler-gcc.h
> > @@ -175,5 +175,3 @@
> >  #else
> >  #define __diag_GCC_8(s)
> >  #endif
> > -
> > -#define __no_fgcse __attribute__((optimize("-fno-gcse")))
> > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> > index 6e390d58a9f8..ac3fa37a84f9 100644
> > --- a/include/linux/compiler_types.h
> > +++ b/include/linux/compiler_types.h
> > @@ -247,10 +247,6 @@ struct ftrace_likely_data {
> >  #define asm_inline asm
> >  #endif
> >
> > -#ifndef __no_fgcse
> > -# define __no_fgcse
> > -#endif
> > -
> >  /* Are two types/vars the same type (ignoring qualifiers)? */
> >  #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
> >
> > diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
> > index bdc8cd1b6767..02b58f44c479 100644
> > --- a/kernel/bpf/Makefile
> > +++ b/kernel/bpf/Makefile
> > @@ -1,6 +1,8 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  obj-y := core.o
> > -CFLAGS_core.o += $(call cc-disable-warning, override-init)
> > +# ___bpf_prog_run() needs GCSE disabled on x86; see 3193c0836f203 for details
> > +cflags-core-$(CONFIG_X86) := -fno-gcse
>
> -fno-gcse is not recognized by clang and will produce
> -Wignored-optimization-argument.  It should at least be wrapped in
> cc-option, though since it's unlikely to ever not be compiler
> specific, I think it might be ok to guard with `ifdef
> CONFIG_CC_IS_GCC`.  Also, might we want to only do this for `ifndef
> CONFIG_RETPOLINE`, based on 3193c0836f203?
>
> Finally, this is going to disable GCSE for the whole translation unit,
> which may be overkill.   Previously it was isolated to one function
> definition.  You could lower the definition of the preprocessor define
> into kernel/bpf/core.c to keep its use isolated as far as possible.
>

Which preprocessor define?

> I'm fine with either approach, but we should avoid new warnings for
> clang.  Thanks for the patch!
>
> > +CFLAGS_core.o += $(call cc-disable-warning, override-init) $(cflags-core-y)
> >
> >  obj-$(CONFIG_BPF_SYSCALL) += syscall.o verifier.o inode.o helpers.o tnum.o bpf_iter.o map_iter.o task_iter.o prog_iter.o
> >  obj-$(CONFIG_BPF_SYSCALL) += hashtab.o arraymap.o percpu_freelist.o bpf_lru_list.o lpm_trie.o map_in_map.o
> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index 9268d77898b7..55454d2278b1 100644
> > --- a/kernel/bpf/core.c
> > +++ b/kernel/bpf/core.c
> > @@ -1369,7 +1369,7 @@ u64 __weak bpf_probe_read_kernel(void *dst, u32 size, const void *unsafe_ptr)
> >   *
> >   * Decode and execute eBPF instructions.
> >   */
> > -static u64 __no_fgcse ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
> > +static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
> >  {
> >  #define BPF_INSN_2_LBL(x, y)    [BPF_##x | BPF_##y] = &&x##_##y
> >  #define BPF_INSN_3_LBL(x, y, z) [BPF_##x | BPF_##y | BPF_##z] = &&x##_##y##_##z
> > --
> > 2.17.1
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers

Re: [PATCH] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE

[Nick Desaulniers]

On Tue, Oct 27, 2020 at 2:50 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 27 Oct 2020 at 22:20, Nick Desaulniers <ndesaulniers@google.com> wrote:
> >
> > On Tue, Oct 27, 2020 at 1:57 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> > > index 6e390d58a9f8..ac3fa37a84f9 100644
> > > --- a/include/linux/compiler_types.h
> > > +++ b/include/linux/compiler_types.h
> > > @@ -247,10 +247,6 @@ struct ftrace_likely_data {
> > >  #define asm_inline asm
> > >  #endif
> > >
> > > -#ifndef __no_fgcse
> > > -# define __no_fgcse
> > > -#endif
> > > -
> > Finally, this is going to disable GCSE for the whole translation unit,
> > which may be overkill.   Previously it was isolated to one function
> > definition.  You could lower the definition of the preprocessor define
> > into kernel/bpf/core.c to keep its use isolated as far as possible.
> >
>
> Which preprocessor define?

__no_fgcse

>
> > I'm fine with either approach, but we should avoid new warnings for
> > clang.  Thanks for the patch!

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE

[Ard Biesheuvel]

On Tue, 27 Oct 2020 at 23:03, Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> On Tue, Oct 27, 2020 at 2:50 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Tue, 27 Oct 2020 at 22:20, Nick Desaulniers <ndesaulniers@google.com> wrote:
> > >
> > > On Tue, Oct 27, 2020 at 1:57 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >
> > > > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> > > > index 6e390d58a9f8..ac3fa37a84f9 100644
> > > > --- a/include/linux/compiler_types.h
> > > > +++ b/include/linux/compiler_types.h
> > > > @@ -247,10 +247,6 @@ struct ftrace_likely_data {
> > > >  #define asm_inline asm
> > > >  #endif
> > > >
> > > > -#ifndef __no_fgcse
> > > > -# define __no_fgcse
> > > > -#endif
> > > > -
> > > Finally, this is going to disable GCSE for the whole translation unit,
> > > which may be overkill.   Previously it was isolated to one function
> > > definition.  You could lower the definition of the preprocessor define
> > > into kernel/bpf/core.c to keep its use isolated as far as possible.
> > >
> >
> > Which preprocessor define?
>
> __no_fgcse
>

But we can't use that, that is the whole point of this patch.

Re: [PATCH] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE

[Nick Desaulniers]

On Tue, Oct 27, 2020 at 3:23 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 27 Oct 2020 at 23:03, Nick Desaulniers <ndesaulniers@google.com> wrote:
> >
> > On Tue, Oct 27, 2020 at 2:50 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Tue, 27 Oct 2020 at 22:20, Nick Desaulniers <ndesaulniers@google.com> wrote:
> > > >
> > > > On Tue, Oct 27, 2020 at 1:57 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > >
> > > > > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> > > > > index 6e390d58a9f8..ac3fa37a84f9 100644
> > > > > --- a/include/linux/compiler_types.h
> > > > > +++ b/include/linux/compiler_types.h
> > > > > @@ -247,10 +247,6 @@ struct ftrace_likely_data {
> > > > >  #define asm_inline asm
> > > > >  #endif
> > > > >
> > > > > -#ifndef __no_fgcse
> > > > > -# define __no_fgcse
> > > > > -#endif
> > > > > -
> > > > Finally, this is going to disable GCSE for the whole translation unit,
> > > > which may be overkill.   Previously it was isolated to one function
> > > > definition.  You could lower the definition of the preprocessor define
> > > > into kernel/bpf/core.c to keep its use isolated as far as possible.
> > > >
> > >
> > > Which preprocessor define?
> >
> > __no_fgcse
> >
>
> But we can't use that, that is the whole point of this patch.

Ah, right because the attribute drops other command line flags...ok,
then -fno-gcse the whole translation unit it is then.

Still need to avoid new warnings with clang though.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE

[Daniel Borkmann]

On 10/27/20 9:57 PM, Ard Biesheuvel wrote:
> Commit 3193c0836f203 ("bpf: Disable GCC -fgcse optimization for
> ___bpf_prog_run()") introduced a __no_fgcse macro that expands to a
> function scope __attribute__((optimize("-fno-gcse"))), to disable a
> GCC specific optimization that was causing trouble on x86 builds, and
> was not expected to have any positive effect in the first place.
> 
> However, as the GCC manual documents, __attribute__((optimize))
> is not for production use, and results in all other optimization
> options to be forgotten for the function in question. This can
> cause all kinds of trouble, but in one particular reported case,

Looks like there are couple more as well aside from __no_fgcse, are you
also planning to fix them?

   arch/powerpc/kernel/setup.h:14:#define __nostackprotector __attribute__((__optimize__("no-stack-protector")))
   tools/include/linux/compiler-gcc.h:37:#define __no_tail_call	__attribute__((optimize("no-optimize-sibling-calls")))

> it causes -fno-asynchronous-unwind-tables to be disregarded,
> resulting in .eh_frame info to be emitted for the function
> inadvertently.

Would have been useful to add a pointer to the original discussion with
Link tag.

Link: https://lore.kernel.org/lkml/CAMuHMdUg0WJHEcq6to0-eODpXPOywLot6UD2=GFHpzoj_hCoBQ@mail.gmail.com/

> This reverts commit 3193c0836f203, and instead, it disables the -fgcse
> optimization for the entire source file, but only when building for
> X86.
> 
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Arvind Sankar <nivedita@alum.mit.edu>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Kees Cook <keescook@chromium.org>
> Fixes: 3193c0836f203 ("bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()")
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
[...]
> diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
> index bdc8cd1b6767..02b58f44c479 100644
> --- a/kernel/bpf/Makefile
> +++ b/kernel/bpf/Makefile
> @@ -1,6 +1,8 @@
>   # SPDX-License-Identifier: GPL-2.0
>   obj-y := core.o
> -CFLAGS_core.o += $(call cc-disable-warning, override-init)
> +# ___bpf_prog_run() needs GCSE disabled on x86; see 3193c0836f203 for details
> +cflags-core-$(CONFIG_X86) := -fno-gcse
> +CFLAGS_core.o += $(call cc-disable-warning, override-init) $(cflags-core-y)

Also, this needs to be guarded behind !CONFIG_RETPOLINE and !CONFIG_BPF_JIT_ALWAYS_ON
in particular the latter since only in this case interpreter is compiled in ... most
distros have the CONFIG_BPF_JIT_ALWAYS_ON set these days for x86.

Do you have an analysis for the commit log on what else this penalizes in core.c if
it's now for the entire translation unit?

>   obj-$(CONFIG_BPF_SYSCALL) += syscall.o verifier.o inode.o helpers.o tnum.o bpf_iter.o map_iter.o task_iter.o prog_iter.o
>   obj-$(CONFIG_BPF_SYSCALL) += hashtab.o arraymap.o percpu_freelist.o bpf_lru_list.o lpm_trie.o map_in_map.o
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 9268d77898b7..55454d2278b1 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -1369,7 +1369,7 @@ u64 __weak bpf_probe_read_kernel(void *dst, u32 size, const void *unsafe_ptr)
>    *
>    * Decode and execute eBPF instructions.
>    */
> -static u64 __no_fgcse ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
> +static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
>   {
>   #define BPF_INSN_2_LBL(x, y)    [BPF_##x | BPF_##y] = &&x##_##y
>   #define BPF_INSN_3_LBL(x, y, z) [BPF_##x | BPF_##y | BPF_##z] = &&x##_##y##_##z
> 

Re: [PATCH] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE

[Nick Desaulniers]

On Tue, Oct 27, 2020 at 4:04 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 10/27/20 9:57 PM, Ard Biesheuvel wrote:
> > Commit 3193c0836f203 ("bpf: Disable GCC -fgcse optimization for
> > ___bpf_prog_run()") introduced a __no_fgcse macro that expands to a
> > function scope __attribute__((optimize("-fno-gcse"))), to disable a
> > GCC specific optimization that was causing trouble on x86 builds, and
> > was not expected to have any positive effect in the first place.
> >
> > However, as the GCC manual documents, __attribute__((optimize))
> > is not for production use, and results in all other optimization
> > options to be forgotten for the function in question. This can
> > cause all kinds of trouble, but in one particular reported case,
>
> Looks like there are couple more as well aside from __no_fgcse, are you
> also planning to fix them?
>
>    arch/powerpc/kernel/setup.h:14:#define __nostackprotector __attribute__((__optimize__("no-stack-protector")))

GCC literally just landed support for
__attribute__((no_stack_protector)) a few days ago.  I was planning on
sending a patch adding it to compiler_attributes.h, but we won't be
able to rely on it for a while.  Now I see I'll have to clean up ppc a
bit. Surely they've had bugs related to optimize attribute
unexpectedly dropping flags.

>    tools/include/linux/compiler-gcc.h:37:#define __no_tail_call __attribute__((optimize("no-optimize-sibling-calls")))

Only used in perf?
tools/perf/tests/dwarf-unwind.c

>
> > it causes -fno-asynchronous-unwind-tables to be disregarded,
> > resulting in .eh_frame info to be emitted for the function
> > inadvertently.
>
> Would have been useful to add a pointer to the original discussion with
> Link tag.
>
> Link: https://lore.kernel.org/lkml/CAMuHMdUg0WJHEcq6to0-eODpXPOywLot6UD2=GFHpzoj_hCoBQ@mail.gmail.com/
>
> > This reverts commit 3193c0836f203, and instead, it disables the -fgcse
> > optimization for the entire source file, but only when building for
> > X86.
> >
> > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > Cc: Arvind Sankar <nivedita@alum.mit.edu>
> > Cc: Randy Dunlap <rdunlap@infradead.org>
> > Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Fixes: 3193c0836f203 ("bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()")
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> [...]
> > diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
> > index bdc8cd1b6767..02b58f44c479 100644
> > --- a/kernel/bpf/Makefile
> > +++ b/kernel/bpf/Makefile
> > @@ -1,6 +1,8 @@
> >   # SPDX-License-Identifier: GPL-2.0
> >   obj-y := core.o
> > -CFLAGS_core.o += $(call cc-disable-warning, override-init)
> > +# ___bpf_prog_run() needs GCSE disabled on x86; see 3193c0836f203 for details
> > +cflags-core-$(CONFIG_X86) := -fno-gcse
> > +CFLAGS_core.o += $(call cc-disable-warning, override-init) $(cflags-core-y)
>
> Also, this needs to be guarded behind !CONFIG_RETPOLINE and !CONFIG_BPF_JIT_ALWAYS_ON
> in particular the latter since only in this case interpreter is compiled in ... most
> distros have the CONFIG_BPF_JIT_ALWAYS_ON set these days for x86.
>
> Do you have an analysis for the commit log on what else this penalizes in core.c if
> it's now for the entire translation unit?
>
> >   obj-$(CONFIG_BPF_SYSCALL) += syscall.o verifier.o inode.o helpers.o tnum.o bpf_iter.o map_iter.o task_iter.o prog_iter.o
> >   obj-$(CONFIG_BPF_SYSCALL) += hashtab.o arraymap.o percpu_freelist.o bpf_lru_list.o lpm_trie.o map_in_map.o
> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index 9268d77898b7..55454d2278b1 100644
> > --- a/kernel/bpf/core.c
> > +++ b/kernel/bpf/core.c
> > @@ -1369,7 +1369,7 @@ u64 __weak bpf_probe_read_kernel(void *dst, u32 size, const void *unsafe_ptr)
> >    *
> >    * Decode and execute eBPF instructions.
> >    */
> > -static u64 __no_fgcse ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
> > +static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
> >   {
> >   #define BPF_INSN_2_LBL(x, y)    [BPF_##x | BPF_##y] = &&x##_##y
> >   #define BPF_INSN_3_LBL(x, y, z) [BPF_##x | BPF_##y | BPF_##z] = &&x##_##y##_##z
> >
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE

[Ard Biesheuvel]

On Wed, 28 Oct 2020 at 00:04, Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 10/27/20 9:57 PM, Ard Biesheuvel wrote:
> > Commit 3193c0836f203 ("bpf: Disable GCC -fgcse optimization for
> > ___bpf_prog_run()") introduced a __no_fgcse macro that expands to a
> > function scope __attribute__((optimize("-fno-gcse"))), to disable a
> > GCC specific optimization that was causing trouble on x86 builds, and
> > was not expected to have any positive effect in the first place.
> >
> > However, as the GCC manual documents, __attribute__((optimize))
> > is not for production use, and results in all other optimization
> > options to be forgotten for the function in question. This can
> > cause all kinds of trouble, but in one particular reported case,
>
> Looks like there are couple more as well aside from __no_fgcse, are you
> also planning to fix them?
>
>    arch/powerpc/kernel/setup.h:14:#define __nostackprotector __attribute__((__optimize__("no-stack-protector")))
>    tools/include/linux/compiler-gcc.h:37:#define __no_tail_call __attribute__((optimize("no-optimize-sibling-calls")))
>

No, but we can notify the respective maintainers.

> > it causes -fno-asynchronous-unwind-tables to be disregarded,
> > resulting in .eh_frame info to be emitted for the function
> > inadvertently.
>
> Would have been useful to add a pointer to the original discussion with
> Link tag.
>
> Link: https://lore.kernel.org/lkml/CAMuHMdUg0WJHEcq6to0-eODpXPOywLot6UD2=GFHpzoj_hCoBQ@mail.gmail.com/
>

Agreed.

> > This reverts commit 3193c0836f203, and instead, it disables the -fgcse
> > optimization for the entire source file, but only when building for
> > X86.
> >
> > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > Cc: Arvind Sankar <nivedita@alum.mit.edu>
> > Cc: Randy Dunlap <rdunlap@infradead.org>
> > Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Fixes: 3193c0836f203 ("bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()")
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> [...]
> > diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
> > index bdc8cd1b6767..02b58f44c479 100644
> > --- a/kernel/bpf/Makefile
> > +++ b/kernel/bpf/Makefile
> > @@ -1,6 +1,8 @@
> >   # SPDX-License-Identifier: GPL-2.0
> >   obj-y := core.o
> > -CFLAGS_core.o += $(call cc-disable-warning, override-init)
> > +# ___bpf_prog_run() needs GCSE disabled on x86; see 3193c0836f203 for details
> > +cflags-core-$(CONFIG_X86) := -fno-gcse
> > +CFLAGS_core.o += $(call cc-disable-warning, override-init) $(cflags-core-y)
>
> Also, this needs to be guarded behind !CONFIG_RETPOLINE and !CONFIG_BPF_JIT_ALWAYS_ON
> in particular the latter since only in this case interpreter is compiled in ... most
> distros have the CONFIG_BPF_JIT_ALWAYS_ON set these days for x86.
>

Is that a new requirement? Because before this patch, -fno-gcse was
applied unconditionally.

> Do you have an analysis for the commit log on what else this penalizes in core.c if
> it's now for the entire translation unit?
>

No, I simply observed the regression this caused on non-x86
architectures, and proposed a way to fix it.

Do you have any concerns in particular regarding other things in
core.c? Would you prefer ___bpf_prog_run() to be moved into a separate
.c file?


> >   obj-$(CONFIG_BPF_SYSCALL) += syscall.o verifier.o inode.o helpers.o tnum.o bpf_iter.o map_iter.o task_iter.o prog_iter.o
> >   obj-$(CONFIG_BPF_SYSCALL) += hashtab.o arraymap.o percpu_freelist.o bpf_lru_list.o lpm_trie.o map_in_map.o
> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index 9268d77898b7..55454d2278b1 100644
> > --- a/kernel/bpf/core.c
> > +++ b/kernel/bpf/core.c
> > @@ -1369,7 +1369,7 @@ u64 __weak bpf_probe_read_kernel(void *dst, u32 size, const void *unsafe_ptr)
> >    *
> >    * Decode and execute eBPF instructions.
> >    */
> > -static u64 __no_fgcse ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
> > +static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
> >   {
> >   #define BPF_INSN_2_LBL(x, y)    [BPF_##x | BPF_##y] = &&x##_##y
> >   #define BPF_INSN_3_LBL(x, y, z) [BPF_##x | BPF_##y | BPF_##z] = &&x##_##y##_##z
> >
>

Re: [PATCH] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE

[Ard Biesheuvel]

On Wed, 28 Oct 2020 at 07:51, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 28 Oct 2020 at 00:04, Daniel Borkmann <daniel@iogearbox.net> wrote:
> >
> > On 10/27/20 9:57 PM, Ard Biesheuvel wrote:
> > > Commit 3193c0836f203 ("bpf: Disable GCC -fgcse optimization for
> > > ___bpf_prog_run()") introduced a __no_fgcse macro that expands to a
> > > function scope __attribute__((optimize("-fno-gcse"))), to disable a
> > > GCC specific optimization that was causing trouble on x86 builds, and
> > > was not expected to have any positive effect in the first place.
> > >
> > > However, as the GCC manual documents, __attribute__((optimize))
> > > is not for production use, and results in all other optimization
> > > options to be forgotten for the function in question. This can
> > > cause all kinds of trouble, but in one particular reported case,
> >
> > Looks like there are couple more as well aside from __no_fgcse, are you
> > also planning to fix them?
> >
> >    arch/powerpc/kernel/setup.h:14:#define __nostackprotector __attribute__((__optimize__("no-stack-protector")))
> >    tools/include/linux/compiler-gcc.h:37:#define __no_tail_call __attribute__((optimize("no-optimize-sibling-calls")))
> >
>
> No, but we can notify the respective maintainers.
>
> > > it causes -fno-asynchronous-unwind-tables to be disregarded,
> > > resulting in .eh_frame info to be emitted for the function
> > > inadvertently.
> >
> > Would have been useful to add a pointer to the original discussion with
> > Link tag.
> >
> > Link: https://lore.kernel.org/lkml/CAMuHMdUg0WJHEcq6to0-eODpXPOywLot6UD2=GFHpzoj_hCoBQ@mail.gmail.com/
> >
>
> Agreed.
>
> > > This reverts commit 3193c0836f203, and instead, it disables the -fgcse
> > > optimization for the entire source file, but only when building for
> > > X86.
> > >
> > > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > > Cc: Arvind Sankar <nivedita@alum.mit.edu>
> > > Cc: Randy Dunlap <rdunlap@infradead.org>
> > > Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Cc: Alexei Starovoitov <ast@kernel.org>
> > > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > > Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Fixes: 3193c0836f203 ("bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()")
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > [...]
> > > diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
> > > index bdc8cd1b6767..02b58f44c479 100644
> > > --- a/kernel/bpf/Makefile
> > > +++ b/kernel/bpf/Makefile
> > > @@ -1,6 +1,8 @@
> > >   # SPDX-License-Identifier: GPL-2.0
> > >   obj-y := core.o
> > > -CFLAGS_core.o += $(call cc-disable-warning, override-init)
> > > +# ___bpf_prog_run() needs GCSE disabled on x86; see 3193c0836f203 for details
> > > +cflags-core-$(CONFIG_X86) := -fno-gcse
> > > +CFLAGS_core.o += $(call cc-disable-warning, override-init) $(cflags-core-y)
> >
> > Also, this needs to be guarded behind !CONFIG_RETPOLINE and !CONFIG_BPF_JIT_ALWAYS_ON
> > in particular the latter since only in this case interpreter is compiled in ... most
> > distros have the CONFIG_BPF_JIT_ALWAYS_ON set these days for x86.
> >
>
> Is that a new requirement? Because before this patch, -fno-gcse was
> applied unconditionally.
>

Ah never mind. You are saying ___bpf_prog_run() does not even exist if
CONFIG_BPF_JIT_ALWAYS_ON=y, right?


> > Do you have an analysis for the commit log on what else this penalizes in core.c if
> > it's now for the entire translation unit?
> >
>
> No, I simply observed the regression this caused on non-x86
> architectures, and proposed a way to fix it.
>
> Do you have any concerns in particular regarding other things in
> core.c? Would you prefer ___bpf_prog_run() to be moved into a separate
> .c file?
>
>
> > >   obj-$(CONFIG_BPF_SYSCALL) += syscall.o verifier.o inode.o helpers.o tnum.o bpf_iter.o map_iter.o task_iter.o prog_iter.o
> > >   obj-$(CONFIG_BPF_SYSCALL) += hashtab.o arraymap.o percpu_freelist.o bpf_lru_list.o lpm_trie.o map_in_map.o
> > > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > > index 9268d77898b7..55454d2278b1 100644
> > > --- a/kernel/bpf/core.c
> > > +++ b/kernel/bpf/core.c
> > > @@ -1369,7 +1369,7 @@ u64 __weak bpf_probe_read_kernel(void *dst, u32 size, const void *unsafe_ptr)
> > >    *
> > >    * Decode and execute eBPF instructions.
> > >    */
> > > -static u64 __no_fgcse ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
> > > +static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
> > >   {
> > >   #define BPF_INSN_2_LBL(x, y)    [BPF_##x | BPF_##y] = &&x##_##y
> > >   #define BPF_INSN_3_LBL(x, y, z) [BPF_##x | BPF_##y | BPF_##z] = &&x##_##y##_##z
> > >
> >

[PATCH] tools/perf: Remove broken __no_tail_call attribute

[Peter Zijlstra]

On Tue, Oct 27, 2020 at 04:11:27PM -0700, Nick Desaulniers wrote:
> On Tue, Oct 27, 2020 at 4:04 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >
> > On 10/27/20 9:57 PM, Ard Biesheuvel wrote:
> > > Commit 3193c0836f203 ("bpf: Disable GCC -fgcse optimization for
> > > ___bpf_prog_run()") introduced a __no_fgcse macro that expands to a
> > > function scope __attribute__((optimize("-fno-gcse"))), to disable a
> > > GCC specific optimization that was causing trouble on x86 builds, and
> > > was not expected to have any positive effect in the first place.
> > >
> > > However, as the GCC manual documents, __attribute__((optimize))
> > > is not for production use, and results in all other optimization
> > > options to be forgotten for the function in question. This can
> > > cause all kinds of trouble, but in one particular reported case,
> >
> > Looks like there are couple more as well aside from __no_fgcse, are you
> > also planning to fix them?
> >
> >    arch/powerpc/kernel/setup.h:14:#define __nostackprotector __attribute__((__optimize__("no-stack-protector")))
> 
> GCC literally just landed support for
> __attribute__((no_stack_protector)) a few days ago.  I was planning on
> sending a patch adding it to compiler_attributes.h, but we won't be
> able to rely on it for a while.  Now I see I'll have to clean up ppc a
> bit. Surely they've had bugs related to optimize attribute
> unexpectedly dropping flags.
> 
> >    tools/include/linux/compiler-gcc.h:37:#define __no_tail_call __attribute__((optimize("no-optimize-sibling-calls")))
> 
> Only used in perf?
> tools/perf/tests/dwarf-unwind.c

Right, that should probably be fixed. It also probably doesn't matter
too much since its an unwinder tests, but still, having that attribute
is dangerous.

The only cross-compiler way of doing this is like in commit
a9a3ed1eff360.

---
Subject: tools/perf: Remove broken __no_tail_call attribute

The GCC specific __attribute__((optimize)) attribute does not what is
commonly expected and is explicitly recommended against using in
production code by the GCC people.

Unlike what is often expected, it doesn't add to the optimization flags,
but it fully replaces them, loosing any and all optimization flags
provided by the compiler commandline.

The only guaranteed upon means of inhibiting tail-calls is by placing a
volatile asm with side-effects after the call such that the tail-call
simply cannot be done.

Given the original commit wasn't specific on which calls were the
problem, this removal might re-introduce the problem, which can then be
re-analyzed and cured properly.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/include/linux/compiler-gcc.h | 12 ------------
 tools/include/linux/compiler.h     |  3 ---
 tools/perf/tests/dwarf-unwind.c    | 10 +++++-----
 3 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/tools/include/linux/compiler-gcc.h b/tools/include/linux/compiler-gcc.h
index b9d4322e1e65..95c072b70d0e 100644
--- a/tools/include/linux/compiler-gcc.h
+++ b/tools/include/linux/compiler-gcc.h
@@ -27,18 +27,6 @@
 #define  __pure		__attribute__((pure))
 #endif
 #define  noinline	__attribute__((noinline))
-#ifdef __has_attribute
-#if __has_attribute(disable_tail_calls)
-#define __no_tail_call	__attribute__((disable_tail_calls))
-#endif
-#endif
-#ifndef __no_tail_call
-#if GCC_VERSION > 40201
-#define __no_tail_call	__attribute__((optimize("no-optimize-sibling-calls")))
-#else
-#define __no_tail_call
-#endif
-#endif
 #ifndef __packed
 #define __packed	__attribute__((packed))
 #endif
diff --git a/tools/include/linux/compiler.h b/tools/include/linux/compiler.h
index 2b3f7353e891..d22a974372c0 100644
--- a/tools/include/linux/compiler.h
+++ b/tools/include/linux/compiler.h
@@ -47,9 +47,6 @@
 #ifndef noinline
 #define noinline
 #endif
-#ifndef __no_tail_call
-#define __no_tail_call
-#endif
 
 /* Are two types/vars the same type (ignoring qualifiers)? */
 #ifndef __same_type
diff --git a/tools/perf/tests/dwarf-unwind.c b/tools/perf/tests/dwarf-unwind.c
index 2491d167bf76..83638097c3bc 100644
--- a/tools/perf/tests/dwarf-unwind.c
+++ b/tools/perf/tests/dwarf-unwind.c
@@ -95,7 +95,7 @@ static int unwind_entry(struct unwind_entry *entry, void *arg)
 	return strcmp((const char *) symbol, funcs[idx]);
 }
 
-__no_tail_call noinline int test_dwarf_unwind__thread(struct thread *thread)
+noinline int test_dwarf_unwind__thread(struct thread *thread)
 {
 	struct perf_sample sample;
 	unsigned long cnt = 0;
@@ -126,7 +126,7 @@ __no_tail_call noinline int test_dwarf_unwind__thread(struct thread *thread)
 
 static int global_unwind_retval = -INT_MAX;
 
-__no_tail_call noinline int test_dwarf_unwind__compare(void *p1, void *p2)
+noinline int test_dwarf_unwind__compare(void *p1, void *p2)
 {
 	/* Any possible value should be 'thread' */
 	struct thread *thread = *(struct thread **)p1;
@@ -145,7 +145,7 @@ __no_tail_call noinline int test_dwarf_unwind__compare(void *p1, void *p2)
 	return p1 - p2;
 }
 
-__no_tail_call noinline int test_dwarf_unwind__krava_3(struct thread *thread)
+noinline int test_dwarf_unwind__krava_3(struct thread *thread)
 {
 	struct thread *array[2] = {thread, thread};
 	void *fp = &bsearch;
@@ -164,12 +164,12 @@ __no_tail_call noinline int test_dwarf_unwind__krava_3(struct thread *thread)
 	return global_unwind_retval;
 }
 
-__no_tail_call noinline int test_dwarf_unwind__krava_2(struct thread *thread)
+noinline int test_dwarf_unwind__krava_2(struct thread *thread)
 {
 	return test_dwarf_unwind__krava_3(thread);
 }
 
-__no_tail_call noinline int test_dwarf_unwind__krava_1(struct thread *thread)
+noinline int test_dwarf_unwind__krava_1(struct thread *thread)
 {
 	return test_dwarf_unwind__krava_2(thread);
 }

Re: [PATCH] tools/perf: Remove broken __no_tail_call attribute

[Ard Biesheuvel]

On Wed, 28 Oct 2020 at 09:11, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Oct 27, 2020 at 04:11:27PM -0700, Nick Desaulniers wrote:
> > On Tue, Oct 27, 2020 at 4:04 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > >
> > > On 10/27/20 9:57 PM, Ard Biesheuvel wrote:
> > > > Commit 3193c0836f203 ("bpf: Disable GCC -fgcse optimization for
> > > > ___bpf_prog_run()") introduced a __no_fgcse macro that expands to a
> > > > function scope __attribute__((optimize("-fno-gcse"))), to disable a
> > > > GCC specific optimization that was causing trouble on x86 builds, and
> > > > was not expected to have any positive effect in the first place.
> > > >
> > > > However, as the GCC manual documents, __attribute__((optimize))
> > > > is not for production use, and results in all other optimization
> > > > options to be forgotten for the function in question. This can
> > > > cause all kinds of trouble, but in one particular reported case,
> > >
> > > Looks like there are couple more as well aside from __no_fgcse, are you
> > > also planning to fix them?
> > >
> > >    arch/powerpc/kernel/setup.h:14:#define __nostackprotector __attribute__((__optimize__("no-stack-protector")))
> >
> > GCC literally just landed support for
> > __attribute__((no_stack_protector)) a few days ago.  I was planning on
> > sending a patch adding it to compiler_attributes.h, but we won't be
> > able to rely on it for a while.  Now I see I'll have to clean up ppc a
> > bit. Surely they've had bugs related to optimize attribute
> > unexpectedly dropping flags.
> >
> > >    tools/include/linux/compiler-gcc.h:37:#define __no_tail_call __attribute__((optimize("no-optimize-sibling-calls")))
> >
> > Only used in perf?
> > tools/perf/tests/dwarf-unwind.c
>
> Right, that should probably be fixed. It also probably doesn't matter
> too much since its an unwinder tests, but still, having that attribute
> is dangerous.
>
> The only cross-compiler way of doing this is like in commit
> a9a3ed1eff360.
>
> ---
> Subject: tools/perf: Remove broken __no_tail_call attribute
>
> The GCC specific __attribute__((optimize)) attribute does not what is
> commonly expected and is explicitly recommended against using in
> production code by the GCC people.
>
> Unlike what is often expected, it doesn't add to the optimization flags,
> but it fully replaces them, loosing any and all optimization flags
> provided by the compiler commandline.
>
> The only guaranteed upon means of inhibiting tail-calls is by placing a
> volatile asm with side-effects after the call such that the tail-call
> simply cannot be done.
>
> Given the original commit wasn't specific on which calls were the
> problem, this removal might re-introduce the problem, which can then be
> re-analyzed and cured properly.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  tools/include/linux/compiler-gcc.h | 12 ------------
>  tools/include/linux/compiler.h     |  3 ---
>  tools/perf/tests/dwarf-unwind.c    | 10 +++++-----
>  3 files changed, 5 insertions(+), 20 deletions(-)
>

Acked-by: Ard Biesheuvel <ardb@kernel.org>

> diff --git a/tools/include/linux/compiler-gcc.h b/tools/include/linux/compiler-gcc.h
> index b9d4322e1e65..95c072b70d0e 100644
> --- a/tools/include/linux/compiler-gcc.h
> +++ b/tools/include/linux/compiler-gcc.h
> @@ -27,18 +27,6 @@
>  #define  __pure                __attribute__((pure))
>  #endif
>  #define  noinline      __attribute__((noinline))
> -#ifdef __has_attribute
> -#if __has_attribute(disable_tail_calls)
> -#define __no_tail_call __attribute__((disable_tail_calls))
> -#endif
> -#endif
> -#ifndef __no_tail_call
> -#if GCC_VERSION > 40201
> -#define __no_tail_call __attribute__((optimize("no-optimize-sibling-calls")))
> -#else
> -#define __no_tail_call
> -#endif
> -#endif
>  #ifndef __packed
>  #define __packed       __attribute__((packed))
>  #endif
> diff --git a/tools/include/linux/compiler.h b/tools/include/linux/compiler.h
> index 2b3f7353e891..d22a974372c0 100644
> --- a/tools/include/linux/compiler.h
> +++ b/tools/include/linux/compiler.h
> @@ -47,9 +47,6 @@
>  #ifndef noinline
>  #define noinline
>  #endif
> -#ifndef __no_tail_call
> -#define __no_tail_call
> -#endif
>
>  /* Are two types/vars the same type (ignoring qualifiers)? */
>  #ifndef __same_type
> diff --git a/tools/perf/tests/dwarf-unwind.c b/tools/perf/tests/dwarf-unwind.c
> index 2491d167bf76..83638097c3bc 100644
> --- a/tools/perf/tests/dwarf-unwind.c
> +++ b/tools/perf/tests/dwarf-unwind.c
> @@ -95,7 +95,7 @@ static int unwind_entry(struct unwind_entry *entry, void *arg)
>         return strcmp((const char *) symbol, funcs[idx]);
>  }
>
> -__no_tail_call noinline int test_dwarf_unwind__thread(struct thread *thread)
> +noinline int test_dwarf_unwind__thread(struct thread *thread)
>  {
>         struct perf_sample sample;
>         unsigned long cnt = 0;
> @@ -126,7 +126,7 @@ __no_tail_call noinline int test_dwarf_unwind__thread(struct thread *thread)
>
>  static int global_unwind_retval = -INT_MAX;
>
> -__no_tail_call noinline int test_dwarf_unwind__compare(void *p1, void *p2)
> +noinline int test_dwarf_unwind__compare(void *p1, void *p2)
>  {
>         /* Any possible value should be 'thread' */
>         struct thread *thread = *(struct thread **)p1;
> @@ -145,7 +145,7 @@ __no_tail_call noinline int test_dwarf_unwind__compare(void *p1, void *p2)
>         return p1 - p2;
>  }
>
> -__no_tail_call noinline int test_dwarf_unwind__krava_3(struct thread *thread)
> +noinline int test_dwarf_unwind__krava_3(struct thread *thread)
>  {
>         struct thread *array[2] = {thread, thread};
>         void *fp = &bsearch;
> @@ -164,12 +164,12 @@ __no_tail_call noinline int test_dwarf_unwind__krava_3(struct thread *thread)
>         return global_unwind_retval;
>  }
>
> -__no_tail_call noinline int test_dwarf_unwind__krava_2(struct thread *thread)
> +noinline int test_dwarf_unwind__krava_2(struct thread *thread)
>  {
>         return test_dwarf_unwind__krava_3(thread);
>  }
>
> -__no_tail_call noinline int test_dwarf_unwind__krava_1(struct thread *thread)
> +noinline int test_dwarf_unwind__krava_1(struct thread *thread)
>  {
>         return test_dwarf_unwind__krava_2(thread);
>  }

Re: [PATCH] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE

[Geert Uytterhoeven]

Hi Ard,

On Tue, Oct 27, 2020 at 9:57 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> Commit 3193c0836f203 ("bpf: Disable GCC -fgcse optimization for
> ___bpf_prog_run()") introduced a __no_fgcse macro that expands to a
> function scope __attribute__((optimize("-fno-gcse"))), to disable a
> GCC specific optimization that was causing trouble on x86 builds, and
> was not expected to have any positive effect in the first place.
>
> However, as the GCC manual documents, __attribute__((optimize))
> is not for production use, and results in all other optimization
> options to be forgotten for the function in question. This can
> cause all kinds of trouble, but in one particular reported case,
> it causes -fno-asynchronous-unwind-tables to be disregarded,
> resulting in .eh_frame info to be emitted for the function
> inadvertently.
>
> This reverts commit 3193c0836f203, and instead, it disables the -fgcse
> optimization for the entire source file, but only when building for
> X86.
>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Arvind Sankar <nivedita@alum.mit.edu>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Kees Cook <keescook@chromium.org>
> Fixes: 3193c0836f203 ("bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()")
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Thanks, this gets rid of the following warning, which you may
want to quote in the patch description:

    aarch64-linux-gnu-ld: warning: orphan section `.eh_frame' from
`kernel/bpf/core.o' being placed in section `.eh_frame'

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] tools/perf: Remove broken __no_tail_call attribute

[Miguel Ojeda]

On Wed, Oct 28, 2020 at 9:11 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> Subject: tools/perf: Remove broken __no_tail_call attribute

Acked-by: Miguel Ojeda <ojeda@kernel.org>

Cheers,
Miguel

RE: [PATCH] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE

[David Laight]

From: Ard Biesheuvel
> Sent: 27 October 2020 20:57
> 
> Commit 3193c0836f203 ("bpf: Disable GCC -fgcse optimization for
> ___bpf_prog_run()") introduced a __no_fgcse macro that expands to a
> function scope __attribute__((optimize("-fno-gcse"))), to disable a
> GCC specific optimization that was causing trouble on x86 builds, and
> was not expected to have any positive effect in the first place.

Surely it is possibly to 'adjust' the bpf code so that gcc doesn't
apply (and can't apply) the gcse optimisation?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)