[v1,4/6] bpf, core: Add weak arch_prepare_goto()

Commit Message

Tiezhu Yang Oct. 15, 2024, 11:39 a.m. UTC

The objtool program needs to analysis the control flow of each
object file generated by compiler toolchain, it needs to know
all the locations that a branch instruction may jump into.

In the past, objtool only works on x86, where objtool can find
the relocation against the nearest instruction before the jump
instruction, which points to the goto table, because there is
only one table jump instruction even if there is more than one
computed goto in a function such as ___bpf_prog_run().

In fact, the compiler behaviors are different for various archs.
On RISC machines (for example LoongArch) this approach does not
work: with -fsection-anchors (often enabled at -O1 or above) the
relocation entry may actually points to the section anchor instead
of the table. Furthermore, objdump kernel/bpf/core.o shows that
there are many table jump instructions in ___bpf_prog_run() with
more than one computed gotos, but there are no relocations which
actually points to the table for some table jump instructions on
LoongArch.

For the jump table of switch cases, a GCC patch "LoongArch: Add
support to annotate tablejump" has been merged into the upstream
mainline, it makes life much easier with the additional section
".discard.tablejump_annotate" which stores the jump info as pairs
of addresses, each pair contains the address of jump instruction
and the address of jump table.

For the jump table of computed gotos, it is indeed not so easy
to implement in the compiler, especially if there is more than
one computed goto in a function.

Without the help of compiler, in order to figure out the address
of goto table by interpreting the LoongArch machine code, add a
function arch_prepare_goto() for goto table, it is an empty weak
definition and is only overridden by archs that have special
requirements.

This is preparation for later patch on LoongArch, there is no any
effect for the other archs with this patch.

Suggested-by: Xi Ruoyao <[e-mail]>
Signed-off-by: Tiezhu Yang <[e-mail]>
---
 kernel/bpf/core.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Alexei Starovoitov Oct. 15, 2024, 6:36 p.m. UTC | #1

On Tue, Oct 15, 2024 at 4:50 AM Tiezhu Yang <[e-mail]> wrote:
>
> The objtool program needs to analysis the control flow of each
> object file generated by compiler toolchain, it needs to know
> all the locations that a branch instruction may jump into.
>
> In the past, objtool only works on x86, where objtool can find
> the relocation against the nearest instruction before the jump
> instruction, which points to the goto table, because there is
> only one table jump instruction even if there is more than one
> computed goto in a function such as ___bpf_prog_run().
>
> In fact, the compiler behaviors are different for various archs.
> On RISC machines (for example LoongArch) this approach does not
> work: with -fsection-anchors (often enabled at -O1 or above) the
> relocation entry may actually points to the section anchor instead
> of the table. Furthermore, objdump kernel/bpf/core.o shows that
> there are many table jump instructions in ___bpf_prog_run() with
> more than one computed gotos, but there are no relocations which
> actually points to the table for some table jump instructions on
> LoongArch.
>
> For the jump table of switch cases, a GCC patch "LoongArch: Add
> support to annotate tablejump" has been merged into the upstream
> mainline, it makes life much easier with the additional section
> ".discard.tablejump_annotate" which stores the jump info as pairs
> of addresses, each pair contains the address of jump instruction
> and the address of jump table.
>
> For the jump table of computed gotos, it is indeed not so easy
> to implement in the compiler, especially if there is more than
> one computed goto in a function.
>
> Without the help of compiler, in order to figure out the address
> of goto table by interpreting the LoongArch machine code, add a
> function arch_prepare_goto() for goto table, it is an empty weak
> definition and is only overridden by archs that have special
> requirements.
>
> This is preparation for later patch on LoongArch, there is no any
> effect for the other archs with this patch.
>
> Suggested-by: Xi Ruoyao <[e-mail]>
> Signed-off-by: Tiezhu Yang <[e-mail]>
> ---
>  kernel/bpf/core.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 5e77c58e0601..81e5d42619d5 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -1706,6 +1706,14 @@ bool bpf_opcode_in_insntable(u8 code)
>  }
>
>  #ifndef CONFIG_BPF_JIT_ALWAYS_ON
> +/*
> + * This symbol is an empty weak definition and is only overridden
> + * by archs that have special requirements.
> + */
> +#ifndef arch_prepare_goto
> +#define arch_prepare_goto()
> +#endif
> +
>  /**
>   *     ___bpf_prog_run - run eBPF program on a given context
>   *     @regs: is the array of MAX_BPF_EXT_REG eBPF pseudo-registers
> @@ -1743,6 +1751,7 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn)
>  #define CONT_JMP ({ insn++; goto select_insn; })
>
>  select_insn:
> +       arch_prepare_goto();
>         goto *jumptable[insn->code];

That looks fragile. There is no guarantee that compiler will keep
asm statement next to indirect goto.
It has all rights to move/copy such goto around.
There are other parts in the kernel which are not annotated either:
drm_exec_retry_on_contention(),
drivers/misc/lkdtm/cfi.c

You're arguing that it's hard to properly in the compiler,
but that's the only option. It has to be done by the compiler.

Tiezhu Yang Oct. 24, 2024, 9:03 a.m. UTC | #2

On 10/16/2024 02:36 AM, Alexei Starovoitov wrote:
> On Tue, Oct 15, 2024 at 4:50 AM Tiezhu Yang <[e-mail]> wrote:
>>
>> The objtool program needs to analysis the control flow of each
>> object file generated by compiler toolchain, it needs to know
>> all the locations that a branch instruction may jump into.

...

>> +       arch_prepare_goto();
>>         goto *jumptable[insn->code];
>
> That looks fragile. There is no guarantee that compiler will keep
> asm statement next to indirect goto.
> It has all rights to move/copy such goto around.
> There are other parts in the kernel which are not annotated either:
> drm_exec_retry_on_contention(),
> drivers/misc/lkdtm/cfi.c
>
> You're arguing that it's hard to properly in the compiler,
> but that's the only option. It has to be done by the compiler.

Thank you very much for your reply. I will drop this patch
and try to find a proper way to handle this case.

By the way, I spent more time to test and analysis with gcc
and clang on x86 and loongarch, it needs to fix some corner
issues for the other patches compiled with clang.

Anyway, I will submit v2 series without changing bpf file,
patch #4 and patch #5 will be removed.

Thanks,
Tiezhu

Patch

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 5e77c58e0601..81e5d42619d5 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1706,6 +1706,14 @@  bool bpf_opcode_in_insntable(u8 code)
 }
 
 #ifndef CONFIG_BPF_JIT_ALWAYS_ON
+/*
+ * This symbol is an empty weak definition and is only overridden
+ * by archs that have special requirements.
+ */
+#ifndef arch_prepare_goto
+#define arch_prepare_goto()
+#endif
+
 /**
  *	___bpf_prog_run - run eBPF program on a given context
  *	@regs: is the array of MAX_BPF_EXT_REG eBPF pseudo-registers
@@ -1743,6 +1751,7 @@  static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn)
 #define CONT_JMP ({ insn++; goto select_insn; })
 
 select_insn:
+	arch_prepare_goto();
 	goto *jumptable[insn->code];
 
 	/* Explicitly mask the register-based shift amounts with 63 or 31