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

["Björn Töpel" <bjorn@kernel.org>]

Ilya Leoshkevich <iii@linux.ibm.com> writes:

> 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?

I've only seen it for kfuncs, yes.

>
>         /* 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?

Hmm, or rather sizeof(u64) if I'm reading you correctly?


Thanks for having a look!
Björn