Re: [PATCH bpf] bpf: Proper R0 zero-extension for BPF_CALL instructions

[Ilya Leoshkevich <iii@linux.ibm.com>]

On Fri, 2022-12-02 at 11:36 +0100, Björn Töpel wrote:
> From: Björn Töpel <bjorn@rivosinc.com>
> 
> A BPF call instruction can be, correctly, marked with zext_dst set to
> true. An example of this can be found in the BPF selftests
> progs/bpf_cubic.c:
> 
>   ...
>   extern __u32 tcp_reno_undo_cwnd(struct sock *sk) __ksym;
> 
>   __u32 BPF_STRUCT_OPS(bpf_cubic_undo_cwnd, struct sock *sk)
>   {
>           return tcp_reno_undo_cwnd(sk);
>   }
>   ...
> 
> which compiles to:
>   0:  r1 = *(u64 *)(r1 + 0x0)
>   1:  call -0x1
>   2:  exit
> 
> The call will be marked as zext_dst set to true, and for some
> backends
> (bpf_jit_needs_zext() returns true) expanded to:
>   0:  r1 = *(u64 *)(r1 + 0x0)
>   1:  call -0x1
>   2:  w0 = w0
>   3:  exit

In the verifier, the marking is done by check_kfunc_call() (added in
e6ac2450d6de), right? So the problem occurs only for kfuncs?

        /* Check return type */
        t = btf_type_skip_modifiers(desc_btf, func_proto->type, NULL);

        ...

        if (btf_type_is_scalar(t)) {
                mark_reg_unknown(env, regs, BPF_REG_0);
                mark_btf_func_reg_size(env, BPF_REG_0, t->size);

I tried to find some official information whether the eBPF calling
convention requires sign- or zero- extending return values and
arguments, but unfortunately [1] doesn't mention this.

LLVM's lib/Target/BPF/BPFCallingConv.td mentions both R* and W*
registers, but since assigning to W* leads to zero-extension, it seems
to me that this is the case.

If the above is correct, then shouldn't we rather use sizeof(void *) in
the mark_btf_func_reg_size() call above?

> The opt_subreg_zext_lo32_rnd_hi32() function which is responsible for
> the zext patching, relies on insn_def_regno() to fetch the register
> to
> zero-extend. However, this function does not handle call instructions
> correctly, and opt_subreg_zext_lo32_rnd_hi32() fails the
> verification.
> 
> Make sure that R0 is correctly resolved for (BPF_JMP | BPF_CALL)
> instructions.
> 
> Fixes: 83a2881903f3 ("bpf: Account for BPF_FETCH in
> insn_has_def32()")
> Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
> ---
> I'm not super happy about the additional special case -- first
> cmpxchg, and now call. :-( A more elegant/generic solution is
> welcome!
> ---
>  kernel/bpf/verifier.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 264b3dc714cc..4f9660eafc72 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -13386,6 +13386,9 @@ static int
> opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
>                 if (!bpf_jit_needs_zext() && !is_cmpxchg_insn(&insn))
>                         continue;
>  
> +               if (insn.code == (BPF_JMP | BPF_CALL))
> +                       load_reg = BPF_REG_0;
> +
>                 if (WARN_ON(load_reg == -1)) {
>                         verbose(env, "verifier bug. zext_dst is set,
> but no reg is defined\n");
>                         return -EFAULT;
> 
> base-commit: 01f856ae6d0ca5ad0505b79bf2d22d7ca439b2a1

[1]
https://docs.kernel.org/bpf/instruction-set.html#registers-and-calling-convention