Yonghong Song <yhs@xxxxxxxx> writes:
>>>>>> 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;
>>>
>>> Want to double check. Do we actually have a problem here?
>>> For example, on x64, we probably won't have this issue.
>>
>> The "problem" is that I hit this:
>> if (WARN_ON(load_reg == -1)) {
>> verbose(env, "verifier bug. zext_dst is set, but no reg is defined\n");
>> return -EFAULT;
>> }
>>
>> This path is only taken for archs which have bpf_jit_needs_zext() ==
>> true. In my case it's riscv64, but it should hit i386, sparc, s390, ppc,
>> mips, and arm.
>>
>> My reading of this thread has been that "marking the call has
>> zext_dst=true, is incorrect", i.e. that LLVM will insert the correct
>> zext instructions.
>
> Your interpretation is correct. Yes, for func return values, the
> llvm will insert correct zext/sext instructions if the return
> value is used. Otherwise, if the return value simply passes
> through, the caller call site should handle that properly.
>
> So, yes changing t->size to sizeof(u64) in below code in
> check_kfunc_call() should work. But the fix sounds like a hack
> and we might have some side effect during verification, now
> or future.
>
> Maybe we could check BPF_PSEUDO_KFUNC_CALL in appropriate place to
> prevent zext.
Thanks for all the input! I'll digest it, and get back with a v2.
