get rid of GCC __attribute__((optimize)) for BPF
Subsystems (4)
v2 cover get rid of GCC __attribute__((optimize)) for BPF
This is a followup to [0]:
[PATCH] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE[0]
Changes since v1:
- only use -fno-gcse when CONFIG_BPF_JIT_ALWAYS_ON=y and CONFIG_CC_IS_GCC=y (but ignore CONFIG_RETPOLINE since we want to avoid GCSE in all cases)
- to avoid potential impact of disabling GCSE on other code, put the interpreter in a separate file (patch #2)
Note that patch #1 is intended for backporting, as function scope GCC optimization attributes are really quite broken.
I don't have a strong opinion on whether the interpreter code should be split off or not, but it looks like it can be done fairly painlessly, so it is probably a good idea to do it anyway.
[0] https://lore.kernel.org/bpf/[email protected]/
Ard Biesheuvel (2):
bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE
bpf: move interpreter into separate source file
include/linux/compiler-gcc.h | 2 - include/linux/compiler_types.h | 4 - include/linux/filter.h | 1 + kernel/bpf/Makefile | 7 +- kernel/bpf/core.c | 567 ------------------ kernel/bpf/interp.c | 601 ++++++++++++++++++++ 6 files changed, 607 insertions(+), 575 deletions(-) create mode 100644 kernel/bpf/interp.c
Signature · 1 line
2.17.1
PATCH v2 1/2 bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE
Commit 3193c0836 ("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.
This reverts commit 3193c0836, and instead, it disables the -fgcse optimization for the entire source file, but only when building for X86 using GCC with CONFIG_BPF_JIT_ALWAYS_ON disabled. Note that the original commit states that CONFIG_RETPOLINE=n triggers the issue, whereas CONFIG_RETPOLINE=y performs better without the optimization, so it is kept disabled in both cases.
--- include/linux/compiler-gcc.h | 2 -- include/linux/compiler_types.h | 4 ---- kernel/bpf/Makefile | 6 +++++- kernel/bpf/core.c | 2 +- 4 files changed, 6 insertions(+), 8 deletions(-)
include/linux/compiler-gcc.h +0 -2
@@ -175,5 +175,3 @@#else #define __diag_GCC_8 (s)#endif - -#define __no_fgcse __attribute__((optimize("-fno-gcse")))
include/linux/compiler_types.h +0 -4
@@ -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))
kernel/bpf/Makefile +5 -1
@@ -1,6 +1,10 @@ # SPDX-License-Identifier: GPL-2.0 obj-y := core.o -CFLAGS_core.o += $(call cc-disable-warning, override-init) +ifneq ($(CONFIG_BPF_JIT_ALWAYS_ON),y) +# ___bpf_prog_run() needs GCSE disabled on x86; see 3193c0836f203 for details +cflags-nogcse-$(CONFIG_X86)$(CONFIG_CC_IS_GCC) := -fno-gcse +endif +CFLAGS_core.o += $(call cc-disable-warning, override-init) $(cflags-nogcse-yy) 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
kernel/bpf/core.c +1 -1
@@ -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
Signature · 1 line
2.17.1
Re: [PATCH v2 1/2] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE
Quoted 35 lines · Ard Biesheuvel
Commit
3193c0836("bpf: Disable GCC -fgcse optimization for___bpf_prog_run()") introduced a__no_fgcsemacro 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.
This reverts commit
3193c0836, and instead, it disables the -fgcse optimization for the entire source file, but only when building for X86 using GCC withCONFIG_BPF_JIT_ALWAYS_ONdisabled. Note that the original commit states thatCONFIG_RETPOLINE=ntriggers the issue, whereasCONFIG_RETPOLINE=yperforms better without the optimization, so it is kept disabled in both cases.Fixes 3193c0836 ("bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()")Link https://lore.kernel.org/lkml/CAMuHMdUg0WJHEcq6to0-eODpXPOywLot6UD2=GFHpzoj_hCoBQ@mail.gmail.com/ open hereSigned-off-by Ard Biesheuvel <[email protected]>--- include/linux/compiler-gcc.h | 2 -- include/linux/compiler_types.h | 4 ---- kernel/bpf/Makefile | 6 +++++- kernel/bpf/core.c | 2 +- 4 files changed, 6 insertions(+), 8 deletions(-)include/linux/compiler-gcc.h +0
-0
include/linux/compiler-gcc.h line 179
Quoted 5 lines
include/linux/compiler-gcc.h +0
-2@@ -175,5 +175,3 @@#else #define __diag_GCC_8 (s)#endif - -#define __no_fgcse __attribute__((optimize("-fno-gcse")))
See my reply in the other thread.
I prefer
-#define __no_fgcse __attribute__((optimize("-fno-gcse")))
+#define __no_fgcse __attribute__((optimize("-fno-gcse,-fno-omit-frame-pointer")))
Potentially with -fno-asynchronous-unwind-tables.
__attribute__((optimize("")) is not as broken as you're claiming to be. It has quirky gcc internal logic, but it's still widely used in many software projects.
Re: [PATCH v2 1/2] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE
Quoted 48 lines · Alexei Starovoitov
Quoted 35 lines · Ard Biesheuvel
Commit
3193c0836("bpf: Disable GCC -fgcse optimization for___bpf_prog_run()") introduced a__no_fgcsemacro 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.
This reverts commit
3193c0836, and instead, it disables the -fgcse optimization for the entire source file, but only when building for X86 using GCC withCONFIG_BPF_JIT_ALWAYS_ONdisabled. Note that the original commit states thatCONFIG_RETPOLINE=ntriggers the issue, whereasCONFIG_RETPOLINE=yperforms better without the optimization, so it is kept disabled in both cases.Fixes 3193c0836 ("bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()")Link https://lore.kernel.org/lkml/CAMuHMdUg0WJHEcq6to0-eODpXPOywLot6UD2=GFHpzoj_hCoBQ@mail.gmail.com/ open hereSigned-off-by Ard Biesheuvel <[email protected]>--- include/linux/compiler-gcc.h | 2 -- include/linux/compiler_types.h | 4 ---- kernel/bpf/Makefile | 6 +++++- kernel/bpf/core.c | 2 +- 4 files changed, 6 insertions(+), 8 deletions(-)include/linux/compiler-gcc.h +0
-0include/linux/compiler-gcc.h line 179
Quoted 5 lines
include/linux/compiler-gcc.h +0
-2@@ -175,5 +175,3 @@#else #define __diag_GCC_8 (s)#endif - -#define __no_fgcse __attribute__((optimize("-fno-gcse")))See my reply in the other thread.
I prefer(quoted diff) +1
-1-#define __no_fgcse __attribute__((optimize("-fno-gcse"))) +#define __no_fgcse __attribute__((optimize("-fno-gcse,-fno-omit-frame-pointer")))
Quoted 2 lines
Potentially with -fno-asynchronous-unwind-tables.
So how would that work? arm64 has the following:
KBUILD_CFLAGS += -fno-asynchronous-unwind-tables -fno-unwind-tables
ifeq ($(CONFIG_SHADOW_CALL_STACK), y)KBUILD_CFLAGS += -ffixed-x18
endif
and it adds -fpatchable-function-entry=2 for compilers that support it, but only when CONFIG_FTRACE is enabled.
Also, as Nick pointed out, -fno-gcse does not work on Clang.
Every architecture will have a different set of requirements here. And there is no way of knowing which -f options are disregarded when you use the function attribute.
So how on earth are you going to #define __no-fgcse correctly for every configuration imaginable?
Quoted 3 lines
__attribute__((optimize("")) is not as broken as you're claiming to be. It has quirky gcc internal logic, but it's still widely used in many software projects.
So it's fine because it is only a little bit broken? I'm sorry, but that makes no sense whatsoever.
If you insist on sticking with this broken construct, can you please make it GCC/x86-only at least?
Re: [PATCH v2 1/2] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE
Quoted 55 lines · Ard Biesheuvel
Quoted 48 lines · Alexei Starovoitov
Quoted 35 lines · Ard Biesheuvel
Commit
3193c0836("bpf: Disable GCC -fgcse optimization for___bpf_prog_run()") introduced a__no_fgcsemacro 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.
This reverts commit
3193c0836, and instead, it disables the -fgcse optimization for the entire source file, but only when building for X86 using GCC withCONFIG_BPF_JIT_ALWAYS_ONdisabled. Note that the original commit states thatCONFIG_RETPOLINE=ntriggers the issue, whereasCONFIG_RETPOLINE=yperforms better without the optimization, so it is kept disabled in both cases.Fixes 3193c0836 ("bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()")Link https://lore.kernel.org/lkml/CAMuHMdUg0WJHEcq6to0-eODpXPOywLot6UD2=GFHpzoj_hCoBQ@mail.gmail.com/ open hereSigned-off-by Ard Biesheuvel <[email protected]>--- include/linux/compiler-gcc.h | 2 -- include/linux/compiler_types.h | 4 ---- kernel/bpf/Makefile | 6 +++++- kernel/bpf/core.c | 2 +- 4 files changed, 6 insertions(+), 8 deletions(-)include/linux/compiler-gcc.h +0
-0include/linux/compiler-gcc.h line 179
Quoted 5 lines
include/linux/compiler-gcc.h +0
-2@@ -175,5 +175,3 @@#else #define __diag_GCC_8 (s)#endif - -#define __no_fgcse __attribute__((optimize("-fno-gcse")))See my reply in the other thread.
I prefer(quoted diff) +1
-1-#define __no_fgcse __attribute__((optimize("-fno-gcse"))) +#define __no_fgcse __attribute__((optimize("-fno-gcse,-fno-omit-frame-pointer")))Quoted 2 lines
Potentially with -fno-asynchronous-unwind-tables.
So how would that work? arm64 has the following:
Quoted 8 lines
KBUILD_CFLAGS+= -fno-asynchronous-unwind-tables -fno-unwind-tablesifeq ($(CONFIG_SHADOW_CALL_STACK), y)
KBUILD_CFLAGS+= -ffixed-x18
endifand it adds -fpatchable-function-entry=2 for compilers that support it, but only when
CONFIG_FTRACEis enabled.
I think you're assuming that GCC drops all flags when it sees __attribute__((optimize)). That's not the case.
Quoted 1 line
Also, as Nick pointed out, -fno-gcse does not work on Clang.
yes and what's the point?
#define __no_fgcse is GCC only. clang doesn't need this workaround.
Quoted 11 lines
Every architecture will have a different set of requirements here. And there is no way of knowing which -f options are disregarded when you use the function attribute.
So how on earth are you going to #define __no-fgcse correctly for every configuration imaginable?
Quoted 3 lines
__attribute__((optimize("")) is not as broken as you're claiming to be. It has quirky gcc internal logic, but it's still widely used in many software projects.
Quoted 5 lines
So it's fine because it is only a little bit broken? I'm sorry, but that makes no sense whatsoever.
If you insist on sticking with this broken construct, can you please make it GCC/x86-only at least?
I'm totally fine with making
#define __no_fgcse __attribute__((optimize("-fno-gcse,-fno-omit-frame-pointer"))) to be gcc+x86 only.
I'd like to get rid of it, but objtool is not smart enough to understand generated asm without it.
Re: [PATCH v2 1/2] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE
Quoted 63 lines · Alexei Starovoitov
Quoted 61 lines · Ard Biesheuvel
Quoted 48 lines · Alexei Starovoitov
Quoted 35 lines · Ard Biesheuvel
Commit
3193c0836("bpf: Disable GCC -fgcse optimization for___bpf_prog_run()") introduced a__no_fgcsemacro 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.
This reverts commit
3193c0836, and instead, it disables the -fgcse optimization for the entire source file, but only when building for X86 using GCC withCONFIG_BPF_JIT_ALWAYS_ONdisabled. Note that the original commit states thatCONFIG_RETPOLINE=ntriggers the issue, whereasCONFIG_RETPOLINE=yperforms better without the optimization, so it is kept disabled in both cases.Fixes 3193c0836 ("bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()")Link https://lore.kernel.org/lkml/CAMuHMdUg0WJHEcq6to0-eODpXPOywLot6UD2=GFHpzoj_hCoBQ@mail.gmail.com/ open hereSigned-off-by Ard Biesheuvel <[email protected]>--- include/linux/compiler-gcc.h | 2 -- include/linux/compiler_types.h | 4 ---- kernel/bpf/Makefile | 6 +++++- kernel/bpf/core.c | 2 +- 4 files changed, 6 insertions(+), 8 deletions(-)include/linux/compiler-gcc.h +0
-0include/linux/compiler-gcc.h line 179
Quoted 5 lines
include/linux/compiler-gcc.h +0
-2@@ -175,5 +175,3 @@#else #define __diag_GCC_8 (s)#endif - -#define __no_fgcse __attribute__((optimize("-fno-gcse")))See my reply in the other thread.
I prefer(quoted diff) +1
-1-#define __no_fgcse __attribute__((optimize("-fno-gcse"))) +#define __no_fgcse __attribute__((optimize("-fno-gcse,-fno-omit-frame-pointer")))Quoted 2 lines
Potentially with -fno-asynchronous-unwind-tables.
So how would that work? arm64 has the following:
KBUILD_CFLAGS+= -fno-asynchronous-unwind-tables -fno-unwind-tablesifeq ($(CONFIG_SHADOW_CALL_STACK), y)
KBUILD_CFLAGS+= -ffixed-x18
endif
Quoted 6 lines
Quoted 2 lines
and it adds -fpatchable-function-entry=2 for compilers that support it, but only when
CONFIG_FTRACEis enabled.I think you're assuming that GCC drops all flags when it sees __attribute__((optimize)). That's not the case.
So which flags does it drop, and which doesn't it drop? Is that documented somewhere? Is that the same for all versions of GCC?
Quoted 5 lines
Quoted 1 line
Also, as Nick pointed out, -fno-gcse does not work on Clang.
yes and what's the point?
#define__no_fgcseis GCC only. clang doesn't need this workaround.
Ah ok, that's at least something.
Quoted 14 lines
Quoted 14 lines
Every architecture will have a different set of requirements here. And there is no way of knowing which -f options are disregarded when you use the function attribute.
So how on earth are you going to #define __no-fgcse correctly for every configuration imaginable?
Quoted 3 lines
__attribute__((optimize("")) is not as broken as you're claiming to be. It has quirky gcc internal logic, but it's still widely used in many software projects.
So it's fine because it is only a little bit broken? I'm sorry, but that makes no sense whatsoever.
Quoted 8 lines
Quoted 2 lines
If you insist on sticking with this broken construct, can you please make it GCC/x86-only at least?
I'm totally fine with making
#define__no_fgcse__attribute__((optimize("-fno-gcse,-fno-omit-frame-pointer"))) to be gcc+x86 only.
I'd like to get rid of it, but objtool is not smart enough to understand generated asm without it.
I'll defer to the x86 folks to make the final call here, but I would be perfectly happy doing
index d1e3c6896b71..68ddb91fbcc6 100644
include/linux/compiler-gcc.h +2 -0
@@ -176,4 +176,6 @@#define __diag_GCC_8 (s)#endif +#ifdef CONFIG_X86 #define __no_fgcse __attribute__((optimize("-fno-gcse"))) +#endif
and end the conversation here, because I honestly cannot wrap my head around the fact that you are willing to work around an x86 specific objtool shortcoming by arbitrarily disabling some GCC optimization for all architectures, using a construct that may or may not affect other compiler settings in unpredictable ways, where the compiler is being used to compile a BPF language runtime for executing BPF programs inside the kernel.
What on earth could go wrong?
Re: [PATCH v2 1/2] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE
Quoted 113 lines · Ard Biesheuvel
Quoted 63 lines · Alexei Starovoitov
Quoted 61 lines · Ard Biesheuvel
Quoted 48 lines · Alexei Starovoitov
Quoted 35 lines · Ard Biesheuvel
Commit
3193c0836("bpf: Disable GCC -fgcse optimization for___bpf_prog_run()") introduced a__no_fgcsemacro 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.
This reverts commit
3193c0836, and instead, it disables the -fgcse optimization for the entire source file, but only when building for X86 using GCC withCONFIG_BPF_JIT_ALWAYS_ONdisabled. Note that the original commit states thatCONFIG_RETPOLINE=ntriggers the issue, whereasCONFIG_RETPOLINE=yperforms better without the optimization, so it is kept disabled in both cases.Fixes 3193c0836 ("bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()")Link https://lore.kernel.org/lkml/CAMuHMdUg0WJHEcq6to0-eODpXPOywLot6UD2=GFHpzoj_hCoBQ@mail.gmail.com/ open hereSigned-off-by Ard Biesheuvel <[email protected]>--- include/linux/compiler-gcc.h | 2 -- include/linux/compiler_types.h | 4 ---- kernel/bpf/Makefile | 6 +++++- kernel/bpf/core.c | 2 +- 4 files changed, 6 insertions(+), 8 deletions(-)include/linux/compiler-gcc.h +0
-0include/linux/compiler-gcc.h line 179
Quoted 5 lines
include/linux/compiler-gcc.h +0
-2@@ -175,5 +175,3 @@#else #define __diag_GCC_8 (s)#endif - -#define __no_fgcse __attribute__((optimize("-fno-gcse")))See my reply in the other thread.
I prefer(quoted diff) +1
-1-#define __no_fgcse __attribute__((optimize("-fno-gcse"))) +#define __no_fgcse __attribute__((optimize("-fno-gcse,-fno-omit-frame-pointer")))Quoted 2 lines
Potentially with -fno-asynchronous-unwind-tables.
So how would that work? arm64 has the following:
KBUILD_CFLAGS+= -fno-asynchronous-unwind-tables -fno-unwind-tablesifeq ($(CONFIG_SHADOW_CALL_STACK), y)
KBUILD_CFLAGS+= -ffixed-x18
endifQuoted 6 lines
Quoted 2 lines
and it adds -fpatchable-function-entry=2 for compilers that support it, but only when
CONFIG_FTRACEis enabled.I think you're assuming that GCC drops all flags when it sees __attribute__((optimize)). That's not the case.
So which flags does it drop, and which doesn't it drop? Is that documented somewhere? Is that the same for all versions of GCC?
Quoted 5 lines
Quoted 1 line
Also, as Nick pointed out, -fno-gcse does not work on Clang.
yes and what's the point?
#define__no_fgcseis GCC only. clang doesn't need this workaround.Ah ok, that's at least something.
Quoted 14 lines
Quoted 14 lines
Every architecture will have a different set of requirements here. And there is no way of knowing which -f options are disregarded when you use the function attribute.
So how on earth are you going to #define __no-fgcse correctly for every configuration imaginable?
Quoted 3 lines
__attribute__((optimize("")) is not as broken as you're claiming to be. It has quirky gcc internal logic, but it's still widely used in many software projects.
So it's fine because it is only a little bit broken? I'm sorry, but that makes no sense whatsoever.
Quoted 8 lines
Quoted 2 lines
If you insist on sticking with this broken construct, can you please make it GCC/x86-only at least?
I'm totally fine with making
#define__no_fgcse__attribute__((optimize("-fno-gcse,-fno-omit-frame-pointer"))) to be gcc+x86 only.
I'd like to get rid of it, but objtool is not smart enough to understand generated asm without it.I'll defer to the x86 folks to make the final call here, but I would be perfectly happy doing
index
d1e3c6896b71..68ddb91fbcc6100644
include/linux/compiler-gcc.h line 181
Quoted 6 lines
include/linux/compiler-gcc.h +2
-0@@ -176,4 +176,6 @@#define __diag_GCC_8 (s)#endif +#ifdef CONFIG_X86 #define __no_fgcse __attribute__((optimize("-fno-gcse"))) +#endif
If you're going to submit this patch could you please add ,-fno-omit-frame-pointer
to the above as well?
Quoted 9 lines
and end the conversation here, because I honestly cannot wrap my head around the fact that you are willing to work around an x86 specific objtool shortcoming by arbitrarily disabling some GCC optimization for all architectures, using a construct that may or may not affect other compiler settings in unpredictable ways, where the compiler is being used to compile a BPF language runtime for executing BPF programs inside the kernel.
What on earth could go wrong?
Frankly I'm move worried that -Os will generate incorrect code. All compilers have bugs. Kernel has bugs. What can go wrong?
Re: [PATCH v2 1/2] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE
Quoted 133 lines · Alexei Starovoitov
Quoted 113 lines · Ard Biesheuvel
Quoted 63 lines · Alexei Starovoitov
Quoted 61 lines · Ard Biesheuvel
Quoted 48 lines · Alexei Starovoitov
Quoted 35 lines · Ard Biesheuvel
Commit
3193c0836("bpf: Disable GCC -fgcse optimization for___bpf_prog_run()") introduced a__no_fgcsemacro 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.
This reverts commit
3193c0836, and instead, it disables the -fgcse optimization for the entire source file, but only when building for X86 using GCC withCONFIG_BPF_JIT_ALWAYS_ONdisabled. Note that the original commit states thatCONFIG_RETPOLINE=ntriggers the issue, whereasCONFIG_RETPOLINE=yperforms better without the optimization, so it is kept disabled in both cases.Fixes 3193c0836 ("bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()")Link https://lore.kernel.org/lkml/CAMuHMdUg0WJHEcq6to0-eODpXPOywLot6UD2=GFHpzoj_hCoBQ@mail.gmail.com/ open hereSigned-off-by Ard Biesheuvel <[email protected]>--- include/linux/compiler-gcc.h | 2 -- include/linux/compiler_types.h | 4 ---- kernel/bpf/Makefile | 6 +++++- kernel/bpf/core.c | 2 +- 4 files changed, 6 insertions(+), 8 deletions(-)include/linux/compiler-gcc.h +0
-0include/linux/compiler-gcc.h line 179
Quoted 5 lines
include/linux/compiler-gcc.h +0
-2@@ -175,5 +175,3 @@#else #define __diag_GCC_8 (s)#endif - -#define __no_fgcse __attribute__((optimize("-fno-gcse")))See my reply in the other thread.
I prefer(quoted diff) +1
-1-#define __no_fgcse __attribute__((optimize("-fno-gcse"))) +#define __no_fgcse __attribute__((optimize("-fno-gcse,-fno-omit-frame-pointer")))Quoted 2 lines
Potentially with -fno-asynchronous-unwind-tables.
So how would that work? arm64 has the following:
KBUILD_CFLAGS+= -fno-asynchronous-unwind-tables -fno-unwind-tablesifeq ($(CONFIG_SHADOW_CALL_STACK), y)
KBUILD_CFLAGS+= -ffixed-x18
endifQuoted 6 lines
Quoted 2 lines
and it adds -fpatchable-function-entry=2 for compilers that support it, but only when
CONFIG_FTRACEis enabled.I think you're assuming that GCC drops all flags when it sees __attribute__((optimize)). That's not the case.
So which flags does it drop, and which doesn't it drop? Is that documented somewhere? Is that the same for all versions of GCC?
Quoted 5 lines
Quoted 1 line
Also, as Nick pointed out, -fno-gcse does not work on Clang.
yes and what's the point?
#define__no_fgcseis GCC only. clang doesn't need this workaround.Ah ok, that's at least something.
Quoted 14 lines
Quoted 14 lines
Every architecture will have a different set of requirements here. And there is no way of knowing which -f options are disregarded when you use the function attribute.
So how on earth are you going to #define __no-fgcse correctly for every configuration imaginable?
Quoted 3 lines
__attribute__((optimize("")) is not as broken as you're claiming to be. It has quirky gcc internal logic, but it's still widely used in many software projects.
So it's fine because it is only a little bit broken? I'm sorry, but that makes no sense whatsoever.
Quoted 8 lines
Quoted 2 lines
If you insist on sticking with this broken construct, can you please make it GCC/x86-only at least?
I'm totally fine with making
#define__no_fgcse__attribute__((optimize("-fno-gcse,-fno-omit-frame-pointer"))) to be gcc+x86 only.
I'd like to get rid of it, but objtool is not smart enough to understand generated asm without it.I'll defer to the x86 folks to make the final call here, but I would be perfectly happy doing
index
d1e3c6896b71..68ddb91fbcc6100644include/linux/compiler-gcc.h line 181
Quoted 6 lines
include/linux/compiler-gcc.h +2
-0@@ -176,4 +176,6 @@#define __diag_GCC_8 (s)#endif +#ifdef CONFIG_X86 #define __no_fgcse __attribute__((optimize("-fno-gcse"))) +#endif If you're going to submit this patch could you please add ,-fno-omit-frame-pointer
to the above as well?Quoted 8 lines
and end the conversation here, because I honestly cannot wrap my head around the fact that you are willing to work around an x86 specific objtool shortcoming by arbitrarily disabling some GCC optimization for all architectures, using a construct that may or may not affect other compiler settings in unpredictable ways, where the compiler is being used to compile a BPF language runtime for executing BPF programs inside the kernel.
Quoted 4 lines
Quoted 1 line
What on earth could go wrong?
Frankly I'm move worried that -Os will generate incorrect code. All compilers have bugs. Kernel has bugs. What can go wrong?
+linux-toolchains. GCC updated the documentation in 7.x to discourage people from using the optimize attribute.
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=893100c3fa9b3049ce84dcc0c1a839ddc7a21387
Re: [PATCH v2 1/2] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE
Quoted 2 lines · Arvind Sankar
Quoted 1 line · Alexei Starovoitov
All compilers have bugs. Kernel has bugs. What can go wrong?
Heh.
Quoted 4 lines
+linux-toolchains. GCC updated the documentation in 7.x to discourage people from using the optimize attribute.
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=893100c3fa9b3049ce84dcc0c1a839ddc7a21387
https://patchwork.ozlabs.org/project/gcc/patch/20151213081911.GA320@x4/
has all the discussion around that GCC patch.
Segher
Re: [PATCH v2 1/2] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE
Quoted 7 lines · Segher Boessenkool
Quoted 2 lines · Arvind Sankar
Quoted 1 line · Alexei Starovoitov
All compilers have bugs. Kernel has bugs. What can go wrong?
Heh.
Quoted 8 lines
Quoted 4 lines
+linux-toolchains. GCC updated the documentation in 7.x to discourage people from using the optimize attribute.
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=893100c3fa9b3049ce84dcc0c1a839ddc7a21387
https://patchwork.ozlabs.org/project/gcc/patch/20151213081911.GA320@x4/
has all the discussion around that GCC patch.
For everyone's convenience, let me reproduce here how the GCC developers describe this attribute on their wiki [0]:
"""
Currently (2015), this attribute is known to have several critical bugs (PR37565, PR63401, PR60580, PR50782). Using it may produce not effect at all or lead to wrong-code.
Quoting one GCC maintainer: "I consider the optimize attribute code seriously broken and unmaintained (but sometimes useful for debugging
- and only that)." source
Unfortunately, the people who added it are either not working on GCC anymore or not interested in fixing it. Do not try to guess how it is supposed to work by trial-and-error. There is not a list of options that are safe to use or known to be broken. Bug reports about the optimize attribute being broken will probably be closed as WONTFIX (PR59262), thus it is not worth to open new ones. If it works for you for a given version of GCC, it doesn't mean it will work on a different machine or a different version.
The only realistic choices are to not use it, to use it and accept its brokenness (current or future one, since it is unmaintained), or join GCC and fix it (perhaps motivating other people along the way to join your effort).
"""
Re: [PATCH v2 1/2] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE
In reply to Alexei StarovoitovQuoted 21 lines · Alexei Starovoitov
Quoted 19 lines · Ard Biesheuvel
Quoted 6 lines · Alexei Starovoitov
I'm totally fine with making
#define__no_fgcse__attribute__((optimize("-fno-gcse,-fno-omit-frame-pointer"))) to be gcc+x86 only.
I'd like to get rid of it, but objtool is not smart enough to understand generated asm without it.I'll defer to the x86 folks to make the final call here, but I would be perfectly happy doing
index
d1e3c6896b71..68ddb91fbcc6100644include/linux/compiler-gcc.h +0
-0@@ -176,4 +176,6 @@#define __diag_GCC_8 (s)#endif
Quoted 7 lines
include/linux/compiler-gcc.h line 181
Quoted 3 lines
include/linux/compiler-gcc.h +2
-0+#ifdef CONFIG_X86 #define __no_fgcse __attribute__((optimize("-fno-gcse"))) +#endif If you're going to submit this patch could you please add ,-fno-omit-frame-pointer
to the above as well?
You'll be playing whack-a-mole with other -f flags that should have been used, which changes even based on the config. The -fsanitize= flags come to mind with the sanitizers.
defconfig shows:
$ make LLVM=1 -j71 kernel/bpf/core.o V=1 2>&1 | grep "\-f" the following -f flags set:
-fno-strict-aliasing
-fno-common
-fshort-wchar
-fno-PIE
-fno-asynchronous-unwind-tables
-fno-delete-null-pointer-checks
-fomit-frame-pointer
-fmacro-prefix-map=./=
-fstack-protector-strong
We already know that -fno-asynchronous-unwind-tables get dropped,
hence this patch. And we know -fomit-frame-pointer or
-fno-omit-frame-pointer I guess gets dropped, hence your ask. We
might not know the full extent which other flags get dropped with the
optimize attribute, but I'd argue that my list above can all result in
pretty bad bugs when accidentally omitted (ok, maybe not -fshort-wchar
or -fmacro-prefix-map, idk what those do) or when mixed with code that
has different values those flags control. Searching GCC's bug tracker
for __attribute__((optimize turns up plenty of reports to make me
think this attribute maybe doesn't work the way folks suspect or
intend: https://gcc.gnu.org/bugzilla/buglist.cgi?quicksearch=__attribute__%28%28optimize&list_id=283390.
There's plenty of folks arguing against the use of the optimize attribute in favor of the command line flag. I urge you to please reconsider the request.
Quoted 1 line
Frankly I'm more worried that -Os will generate incorrect code.
If you have observed bugs as a result of setting CONFIG_CC_OPTIMIZE_FOR_SIZE, we would love to help you get to the bottom of them and help you debug them. But we should also remain vigilant against rejecting progress on the status quo for known issues over hypothetical issues without proper regard for evidence. Correctness is the chief concern of a compiler; that it generates incorrect code unless default-on optimizations are explicitly disabled would be concerning, if that was in fact the case. Such a bug report would be invaluable to this code base, and likely others. I trust you've seen bugs here, but I would like to help verify this claim.
Quoted 1 line
All compilers have bugs. Kernel has bugs. What can go wrong?
This is more terrifyingly precise and infinitely wise than you may have initially intended. That my phone and laptop don't catch fire simultaneously now is nothing short of miraculous. I'm still holding my breath.
--
Thanks,
~Nick Desaulniers
Re: [PATCH v2 1/2] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE
Quoted 3 lines · Nick Desaulniers
We already know that -fno-asynchronous-unwind-tables get dropped, hence this patch.
On arm64 only. Not on x86
Quoted 2 lines
And we know -fomit-frame-pointer or -fno-omit-frame-pointer I guess gets dropped, hence your ask.
yep. that one is bugged.
Quoted 4 lines
We might not know the full extent which other flags get dropped with the optimize attribute, but I'd argue that my list above can all result in pretty bad bugs when accidentally omitted (ok, maybe not -fshort-wchar or -fmacro-prefix-map, idk what those do) or when mixed with code that
true.
Few month back I've checked that strict-aliasing and no-common flags from your list are not dropped by this attr in gcc [6789]. I've also checked that no-red-zone and model=kernel preserved as well.
Quoted 4 lines
has different values those flags control. Searching GCC's bug tracker
for__attribute__((optimizeturns up plenty of reports to make me
think this attribute maybe doesn't work the way folks suspect or
intend: https://gcc.gnu.org/bugzilla/buglist.cgi?quicksearch=__attribute__%28%28optimize&list_id=283390.
There is a risk.
Is it a footgun? Sure.
Yet. gcc testsuite is using __attribute__((optimize)).
And some of these tests were added _after_ offical gcc doc said that this attribute is broken.
imo it's like 'beware of the dog' sign.
Quoted 3 lines
There's plenty of folks arguing against the use of the optimize attribute in favor of the command line flag. I urge you to please reconsider the request.
ok. Applied this first patch to bpf tree and will get it to Linus soon. Second patch that is splitting interpreter out because of this mess is dropped. The effect of gcse on performance is questionable. iirc some interpreters used to do -fno-gcse to gain performance.
Re: [PATCH v2 1/2] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE
Quoted 35 lines · Alexei Starovoitov
Quoted 3 lines · Nick Desaulniers
We already know that -fno-asynchronous-unwind-tables get dropped, hence this patch.
On arm64 only. Not on x86
Quoted 2 lines
And we know -fomit-frame-pointer or -fno-omit-frame-pointer I guess gets dropped, hence your ask.
yep. that one is bugged.
Quoted 4 lines
We might not know the full extent which other flags get dropped with the optimize attribute, but I'd argue that my list above can all result in pretty bad bugs when accidentally omitted (ok, maybe not -fshort-wchar or -fmacro-prefix-map, idk what those do) or when mixed with code that
true.
Few month back I've checked that strict-aliasing and no-common flags from your list are not dropped by this attr in gcc [6789]. I've also checked that no-red-zone and model=kernel preserved as well.Quoted 4 lines
has different values those flags control. Searching GCC's bug tracker
for__attribute__((optimizeturns up plenty of reports to make me
think this attribute maybe doesn't work the way folks suspect or
intend: https://gcc.gnu.org/bugzilla/buglist.cgi?quicksearch=__attribute__%28%28optimize&list_id=283390.There is a risk.
Is it a footgun? Sure.
Yet. gcc testsuite is using __attribute__((optimize)).
And some of these tests were added _after_ offical gcc doc said that this attribute is broken.
imo it's like 'beware of the dog' sign.
Quoted 8 lines
Quoted 3 lines
There's plenty of folks arguing against the use of the optimize attribute in favor of the command line flag. I urge you to please reconsider the request.
ok. Applied this first patch to bpf tree and will get it to Linus soon. Second patch that is splitting interpreter out because of this mess is dropped. The effect of gcse on performance is questionable. iirc some interpreters used to do -fno-gcse to gain performance.
That is absolutely fine. I only included the second patch to address Daniel's concern that -fno-gcse could affect unrelated code living in the same source file as __bpf_prog_run(), but if you don't care about that, nor will I.
Re: [PATCH v2 1/2] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE
In reply to Ard BiesheuvelQuoted 20 lines · Ard Biesheuvel
Commit
3193c0836("bpf: Disable GCC -fgcse optimization for___bpf_prog_run()") introduced a__no_fgcsemacro 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.
This reverts commit
3193c0836, and instead, it disables the -fgcse optimization for the entire source file, but only when building for X86 using GCC withCONFIG_BPF_JIT_ALWAYS_ONdisabled. Note that the original commit states thatCONFIG_RETPOLINE=ntriggers the issue, whereasCONFIG_RETPOLINE=yperforms better without the optimization, so it is kept disabled in both cases.
Quoted 3 lines
Fixes 3193c0836 ("bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()")Link https://lore.kernel.org/lkml/CAMuHMdUg0WJHEcq6to0-eODpXPOywLot6UD2=GFHpzoj_hCoBQ@mail.gmail.com/ open hereSigned-off-by Ard Biesheuvel <[email protected]>
(probably you missed by tag on v1 due to kernel.org hickups)
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' fromkernel/bpf/core.o' being placed in section .eh_frame'
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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 v2 1/2] bpf: don't rely on GCC __attribute__((optimize)) to disable GCSE
In reply to Ard BiesheuvelQuoted 61 lines · Ard Biesheuvel
Commit
3193c0836("bpf: Disable GCC -fgcse optimization for___bpf_prog_run()") introduced a__no_fgcsemacro 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.
This reverts commit
3193c0836, and instead, it disables the -fgcse optimization for the entire source file, but only when building for X86 using GCC withCONFIG_BPF_JIT_ALWAYS_ONdisabled. Note that the original commit states thatCONFIG_RETPOLINE=ntriggers the issue, whereasCONFIG_RETPOLINE=yperforms better without the optimization, so it is kept disabled in both cases.Fixes 3193c0836 ("bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()")Link https://lore.kernel.org/lkml/CAMuHMdUg0WJHEcq6to0-eODpXPOywLot6UD2=GFHpzoj_hCoBQ@mail.gmail.com/ open hereSigned-off-by Ard Biesheuvel <[email protected]>--- include/linux/compiler-gcc.h | 2 -- include/linux/compiler_types.h | 4 ---- kernel/bpf/Makefile | 6 +++++- kernel/bpf/core.c | 2 +- 4 files changed, 6 insertions(+), 8 deletions(-)include/linux/compiler-gcc.h +0
-2@@ -175,5 +175,3 @@#else #define __diag_GCC_8 (s)#endif - -#define __no_fgcse __attribute__((optimize("-fno-gcse")))include/linux/compiler_types.h +0
-4@@ -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))kernel/bpf/Makefile +0
-0
kernel/bpf/Makefile line 7
Quoted 8 lines
kernel/bpf/Makefile +5
-1@@ -1,6 +1,10 @@ # SPDX-License-Identifier: GPL-2.0 obj-y := core.o -CFLAGS_core.o += $(call cc-disable-warning, override-init) +ifneq ($(CONFIG_BPF_JIT_ALWAYS_ON),y) +# ___bpf_prog_run() needs GCSE disabled on x86; see 3193c0836f203 for details +cflags-nogcse-$(CONFIG_X86)$(CONFIG_CC_IS_GCC) := -fno-gcse +endif +CFLAGS_core.o += $(call cc-disable-warning, override-init) $(cflags-nogcse-yy)
Writing multiple conditions in a conditional block in GNU make is painful, hence the double y trick. I feel like either 3 nested conditionals (one for CONFIG_BPF_JIT_ALWAYS_ON, CONFIG_X86, and CONFIG_CC_IS_GCC) would have been clearer, or using three y, rather than mixing and matching ifs with multiple ys, but regardless of what color I think we should paint the bikeshed:
This also doesn't resolve all issues here, but is a step in the right direction, IMO.
Quoted 19 lines
kernel/bpf/Makefile +0
-0obj-$(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.okernel/bpf/core.c +1
-2@@ -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
Signature · 2 lines
Thanks, ~Nick Desaulniers
PATCH v2 2/2 bpf: move interpreter into separate source file
In reply to Ard BiesheuvelTo reduce the impact of disabling certain compiler optimizations that are only needed for the interpreter, move it into its own source file, and apply the compiler command line override only to this file.
--- include/linux/filter.h | 1 + kernel/bpf/Makefile | 7 +- kernel/bpf/core.c | 567 ------------------ kernel/bpf/interp.c | 601 ++++++++++++++++++++ 4 files changed, 605 insertions(+), 571 deletions(-)
include/linux/filter.h +1 -0
@@ -841,6 +841,7 @@ static inline int sk_filter(struct sock *sk, struct sk_buff *skb) }struct bpf_prog *bpf_prog_select_runtime (struct bpf_prog *fp,int *err); +void bpf_prog_select_func (struct bpf_prog *fp);void bpf_prog_free (struct bpf_prog *fp);bool bpf_opcode_in_insntable (u8 code);
kernel/bpf/Makefile +3 -4
@@ -1,10 +1,9 @@ # SPDX-License-Identifier: GPL-2.0 -obj-y := core.o -ifneq ($(CONFIG_BPF_JIT_ALWAYS_ON),y) +obj-y := core.o interp.o + # ___bpf_prog_run() needs GCSE disabled on x86; see 3193c0836f203 for details cflags-nogcse-$(CONFIG_X86)$(CONFIG_CC_IS_GCC) := -fno-gcse -endif -CFLAGS_core.o += $(call cc-disable-warning, override-init) $(cflags-nogcse-yy) +CFLAGS_interp.o += $(call cc-disable-warning, override-init) $(cflags-nogcse-yy) 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
kernel/bpf/core.c +0 -567
585 lines not on this page · raw message
kernel/bpf/interp.c new file +601 -0
601 lines not on this page · raw message
Signature · 1 line
2.17.1