Re: [PATCH] bpf: Tweak BPF jump table optimizations for objtool compatibility

[Alexei Starovoitov]

On Thu, Apr 30, 2020 at 02:07:43PM -0500, Josh Poimboeuf wrote:
> Objtool decodes instructions and follows all potential code branches
> within a function.  But it's not an emulator, so it doesn't track
> register values.  For that reason, it usually can't follow
> intra-function indirect branches, unless they're using a jump table
> which follows a certain format (e.g., GCC switch statement jump tables).
> 
> In most cases, the generated code for the BPF jump table looks a lot
> like a GCC jump table, so objtool can follow it.  However, with
> RETPOLINE=n, GCC keeps the jump table address in a register, and then
> does 160+ indirect jumps with it.  When objtool encounters the indirect
> jumps, it can't tell which jump table is being used (or even whether
> they might be sibling calls instead).
> 
> This was fixed before by disabling an optimization in ___bpf_prog_run(),
> using the "optimize" function attribute.  However, that attribute is bad
> news.  It doesn't append options to the command-line arguments.  Instead
> it starts from a blank slate.  And according to recent GCC documentation
> it's not recommended for production use.  So revert the previous fix:
> 
>   3193c0836f20 ("bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()")
> 
> With that reverted, solve the original problem in a different way by
> getting rid of the "goto select_insn" indirection, and instead just goto
> the jump table directly.  This simplifies the code a bit and helps GCC
> generate saner code for the jump table branches, at least in the
> RETPOLINE=n case.
> 
> But, in the RETPOLINE=y case, this simpler code actually causes GCC to
> generate far worse code, ballooning the function text size by +40%.  So
> leave that code the way it was.  In fact Alexei prefers to leave *all*
> the code the way it was, except where needed by objtool.  So even
> non-x86 RETPOLINE=n code will continue to have "goto select_insn".
> 
> This stuff is crazy voodoo, and far from ideal.  But it works for now.
> Eventually, there's a plan to create a compiler plugin for annotating
> jump tables.  That will make this a lot less fragile.

I don't like this commit log.
Here you're saying that the code recognized by objtool is sane and good
whereas well optimized gcc code is somehow voodoo and bad.
That is just wrong.
goto select_insn; vs goto *jumptable[insn->code]; is not a contract that
compiler has to follow. The compiler is free to convert direct goto
into indirect and the other way around.
For all practical purposes this patch is a band aid for objtool that will fall
apart in the future. Just like the previous patch that survived less than a year.
It's not clear whether old one worked for clang.
It's not clear whether new one will work for clang.
retpoline=y causing code bloat is a different issue that can be investigated
separately. gcc/clang have different modes of generating retpoline thunks.
May be one of those flags can help.

In other words I'm ok with the patch, but commit log needs to be reworded.

> Fixes: 3193c0836f20 ("bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()")
> Reported-by: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
> Reported-by: Arnd Bergmann <arnd@xxxxxxxx>
> Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> ---
>  include/linux/compiler-gcc.h   |  2 --
>  include/linux/compiler_types.h |  4 ----
>  kernel/bpf/core.c              | 10 +++++++---
>  3 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index cf294faec2f8..2c8583eb5de8 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -176,5 +176,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 e970f97a7fcb..58105f1deb79 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -203,10 +203,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/core.c b/kernel/bpf/core.c
> index 916f5132a984..eec470c598ad 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -1364,7 +1364,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
> @@ -1384,11 +1384,15 @@ static u64 __no_fgcse ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u6
>  #undef BPF_INSN_2_LBL
>  	u32 tail_call_cnt = 0;
>  
> +#if defined(CONFIG_X86_64) && !defined(CONFIG_RETPOLINE)
> +#define CONT	 ({ insn++; goto *jumptable[insn->code]; })
> +#define CONT_JMP ({ insn++; goto *jumptable[insn->code]; })
> +#else
>  #define CONT	 ({ insn++; goto select_insn; })
>  #define CONT_JMP ({ insn++; goto select_insn; })
> -
>  select_insn:
>  	goto *jumptable[insn->code];
> +#endif
>  
>  	/* ALU */
>  #define ALU(OPCODE, OP)			\
> @@ -1547,7 +1551,7 @@ static u64 __no_fgcse ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u6
>  		 * where arg1_type is ARG_PTR_TO_CTX.
>  		 */
>  		insn = prog->insnsi;
> -		goto select_insn;
> +		CONT;

This is broken. I don't think you've run basic tests with this patch.