Re: [PATCH v3 bpf-next 03/14] bpf: Add x86-64 JIT support for PROBE_MEM32 pseudo instructions.

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Thu, Mar 7, 2024 at 5:08 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> From: Alexei Starovoitov <ast@kernel.org>
>
> Add support for [LDX | STX | ST], PROBE_MEM32, [B | H | W | DW] instructions.
> They are similar to PROBE_MEM instructions with the following differences:
> - PROBE_MEM has to check that the address is in the kernel range with
>   src_reg + insn->off >= TASK_SIZE_MAX + PAGE_SIZE check
> - PROBE_MEM doesn't support store
> - PROBE_MEM32 relies on the verifier to clear upper 32-bit in the register
> - PROBE_MEM32 adds 64-bit kern_vm_start address (which is stored in %r12 in the prologue)
>   Due to bpf_arena constructions such %r12 + %reg + off16 access is guaranteed
>   to be within arena virtual range, so no address check at run-time.
> - PROBE_MEM32 allows STX and ST. If they fault the store is a nop.
>   When LDX faults the destination register is zeroed.
>
> Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
>  arch/x86/net/bpf_jit_comp.c | 191 +++++++++++++++++++++++++++++++++++-
>  include/linux/bpf.h         |   1 +
>  include/linux/filter.h      |   3 +
>  3 files changed, 194 insertions(+), 1 deletion(-)
>

[...]

> +static u8 add_3mod(u8 byte, u32 r1, u32 r2, u32 index)
> +{
> +       if (is_ereg(r1))
> +               byte |= 1;
> +       if (is_ereg(index))
> +               byte |= 2;
> +       if (is_ereg(r2))
> +               byte |= 4;
> +       return byte;
> +}
> +
>  /* Encode 'dst_reg' register into x86-64 opcode 'byte' */
>  static u8 add_1reg(u8 byte, u32 dst_reg)
>  {
> @@ -645,6 +659,8 @@ static void emit_bpf_tail_call_indirect(struct bpf_prog *bpf_prog,
>                 pop_r12(&prog);
>         } else {
>                 pop_callee_regs(&prog, callee_regs_used);
> +               if (bpf_arena_get_kern_vm_start(bpf_prog->aux->arena))

ah, I guess this is where NULL is expected?.. But isn't `if
(bpf_prog->aux->arena)` equivalent and more straightforward check?

> +                       pop_r12(&prog);
>         }
>
>         EMIT1(0x58);                              /* pop rax */
> @@ -704,6 +720,8 @@ static void emit_bpf_tail_call_direct(struct bpf_prog *bpf_prog,
>                 pop_r12(&prog);
>         } else {
>                 pop_callee_regs(&prog, callee_regs_used);
> +               if (bpf_arena_get_kern_vm_start(bpf_prog->aux->arena))
> +                       pop_r12(&prog);
>         }
>
>         EMIT1(0x58);                                  /* pop rax */

[...]

> @@ -1147,11 +1276,14 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image, u8 *rw_image
>         bool tail_call_seen = false;
>         bool seen_exit = false;
>         u8 temp[BPF_MAX_INSN_SIZE + BPF_INSN_SAFETY];
> +       u64 arena_vm_start;
>         int i, excnt = 0;
>         int ilen, proglen = 0;
>         u8 *prog = temp;
>         int err;
>
> +       arena_vm_start = bpf_arena_get_kern_vm_start(bpf_prog->aux->arena);

and I'm guessing here you didn't want that check... I'd probably go
with explicit pointer checks, but ok, it's fine

> +
>         detect_reg_usage(insn, insn_cnt, callee_regs_used,
>                          &tail_call_seen);
>

[...]