Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201027205723.12514-1-ardb@kernel.org> <CAKwvOdmSaVcgq2eKRjRL+_StdFNG2QnNe3nGCs2PWfH=HceadA@mail.gmail.com>
In-Reply-To: <CAKwvOdmSaVcgq2eKRjRL+_StdFNG2QnNe3nGCs2PWfH=HceadA@mail.gmail.com>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Tue, 27 Oct 2020 22:49:55 +0100
Message-ID: <CAMj1kXHb8Fe9fqpj4-90ccEMB+NJ6cbuuog-2Vuo7tr7VjZaTA@mail.gmail.com>
Subject: Re: [PATCH] bpf: don't rely on GCC __attribute__((optimize)) to
 disable GCSE
To:     Nick Desaulniers <ndesaulniers@google.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Network Development <netdev@vger.kernel.org>,
        bpf <bpf@vger.kernel.org>, Arnd Bergmann <arnd@arndb.de>,
        Arvind Sankar <nivedita@alum.mit.edu>,
        Randy Dunlap <rdunlap@infradead.org>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Peter Zijlstra <peterz@infradead.org>,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        Kees Cook <keescook@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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