On Tue, Oct 27, 2020 at 4:04 PM Daniel Borkmann <dan...@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_hc...@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 <ndesaulni...@google.com> > > Cc: Arvind Sankar <nived...@alum.mit.edu> > > Cc: Randy Dunlap <rdun...@infradead.org> > > Cc: Josh Poimboeuf <jpoim...@redhat.com> > > Cc: Thomas Gleixner <t...@linutronix.de> > > Cc: Alexei Starovoitov <a...@kernel.org> > > Cc: Daniel Borkmann <dan...@iogearbox.net> > > Cc: Peter Zijlstra (Intel) <pet...@infradead.org> > > Cc: Geert Uytterhoeven <ge...@linux-m68k.org> > > Cc: Kees Cook <keesc...@chromium.org> > > Fixes: 3193c0836f203 ("bpf: Disable GCC -fgcse optimization for > > ___bpf_prog_run()") > > Signed-off-by: Ard Biesheuvel <a...@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