Re: [PATCH bpf-next v4 03/13] bpf: Wire up JIT support for 16-byte kfunc returns

On 8/10/26 6:24 PM, [email protected] wrote:
>> LLVM 23 returns an __int128, or a struct/union larger than 8 bytes and no
>> larger than 16 bytes, in the BPF R0:R2 register pair. The previous patch
>> taught the verifier about that convention; wire up the JIT side so that the
>> second half of the return value actually lands in R2.
> This isn't a bug, but should this say the previous patch added the shared
> R0:R2 helpers, rather than that it taught the verifier the convention?
> Looking at 882cac43a2cef ("bpf: Add helpers to describe the R0:R2 return
> register pair"), its changelog states "No functional change,
> bpf_ret_reg_pair() has no callers yet" and "Later patches teach the JIT,
> precision backtracking, live register analysis and the verifier itself
> about that convention." The verifier actually learns the convention in
> d280e9c973c59 ("bpf: Add verifier support for 16-byte returns in R0:R2"),
> two patches later.

Okay, will update the commit message.

>
>> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
>> index 8dddb5d7af21b..01c34114c8502 100644
>> --- a/arch/x86/net/bpf_jit_comp.c
>> +++ b/arch/x86/net/bpf_jit_comp.c
>> @@ -2647,6 +2647,22 @@ st:			insn_off = insn->off;
>>   				return -EINVAL;
>>   			if (priv_frame_ptr)
>>   				pop_r9(&prog);
>> +			if (src_reg == BPF_PSEUDO_KFUNC_CALL) {
>> +				const struct btf_func_model *fm;
>> +
>> +				/*
>> +				 * A kfunc returning a >8 byte aggregate hands the
>> +				 * second half back in RDX (the native ABI's second
>> +				 * return reg), but BPF expects it in R0:R2. BPF R0
>> +				 * is RAX (no move needed), while BPF R2 is RSI, so
>> +				 * copy RDX into RSI.
>> +				 */
> This isn't a bug, but since struct/union returns are still rejected by
> btf_distill_func_proto() at this point in the series (kernel/bpf/btf.c
> checks "if (ret < 0 || btf_type_is_struct(t))"), would "a kfunc
> returning more than 8 bytes" read more accurately here than ">8 byte
> aggregate"? Aggregates only become reachable later in b12c58bbc5e59
> ("bpf: Enable aggregate return types up to 16 bytes"), so the only
> return type that can reach fm->ret_size > 8 at this commit is __int128.

Will update commit message.

>
>> +				fm = bpf_jit_find_kfunc_model(bpf_prog, insn);
>> +				if (!fm)
>> +					return -EFAULT;
>> +				if (fm->ret_size > 8)
>> +					emit_mov_reg(&prog, true, BPF_REG_2, BPF_REG_3);
>> +			}
>>   			break;
>>   		}
> This isn't a bug, but emit_kfunc_arena_args() already does this same
> bpf_jit_find_kfunc_model() lookup earlier in do_jit() and bails out on
> NULL. Looking at emit_kfunc_arena_args() in arch/x86/net/bpf_jit_comp.c:
>
>      fm = bpf_jit_find_kfunc_model(prog, insn);
>      if (!fm)
>          return -EINVAL;
>
> Since bpf_jit_find_kfunc_model() is a pure bsearch over an immutable
> table, if the first lookup succeeded the second cannot fail. Could the
> two BPF_PSEUDO_KFUNC_CALL blocks share one lookup, so the second bsearch
> and the unreachable "if (!fm)" guard go away? The two sites also disagree
> on the errno for the identical condition (-EINVAL vs -EFAULT).

Good point. Let us just one 'fm = bpf_jit_find_kfunc_model(prog, insn)'.
Will change.

>
>
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
>
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31446101762