Date:   Wed, 28 Oct 2020 07:59:48 +0100
From:   Ard Biesheuvel <ardb@...nel.org>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:BPF JIT for MIPS (32-BIT AND 64-BIT)" 
        <netdev@...r.kernel.org>,
        "open list:BPF JIT for MIPS (32-BIT AND 64-BIT)" 
        <bpf@...r.kernel.org>, Arnd Bergmann <arnd@...db.de>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Arvind Sankar <nivedita@...m.mit.edu>,
        Randy Dunlap <rdunlap@...radead.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Alexei Starovoitov <ast@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH] bpf: don't rely on GCC __attribute__((optimize)) to
 disable GCSE

On Wed, 28 Oct 2020 at 07:51, Ard Biesheuvel <ardb@...nel.org> wrote:
>
> On Wed, 28 Oct 2020 at 00:04, Daniel Borkmann <daniel@...earbox.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@...gle.com>
> > > Cc: Arvind Sankar <nivedita@...m.mit.edu>
> > > Cc: Randy Dunlap <rdunlap@...radead.org>
> > > Cc: Josh Poimboeuf <jpoimboe@...hat.com>
> > > Cc: Thomas Gleixner <tglx@...utronix.de>
> > > Cc: Alexei Starovoitov <ast@...nel.org>
> > > Cc: Daniel Borkmann <daniel@...earbox.net>
> > > Cc: Peter Zijlstra (Intel) <peterz@...radead.org>
> > > Cc: Geert Uytterhoeven <geert@...ux-m68k.org>
> > > Cc: Kees Cook <keescook@...omium.org>
> > > Fixes: 3193c0836f203 ("bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()")
> > > Signed-off-by: Ard Biesheuvel <ardb@...nel.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
> > >
> >

Powered by blists - more mailing lists