Re: [PATCH bpf-next v5 02/11] bpf: Add helpers to describe the R0:R2 return register pair

On 8/13/26 2:11 PM, [email protected] wrote:
>> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
>> --- a/include/linux/bpf_verifier.h
>> +++ b/include/linux/bpf_verifier.h
>> @@ -814,6 +814,8 @@ struct bpf_subprog_info {
>>   	bool is_async_cb: 1;
>>   	bool is_exception_cb: 1;
>>   	bool args_cached: 1;
>> +	/* true if the return value is passed in the R0:R2 register pair */
>> +	bool ret_reg_pair: 1;
>>   	/* true if bpf_fastcall stack region is used by functions that can't be inlined */
>>   	bool keep_fastcall_stack: 1;
>>   	bool changes_pkt_data: 1;
>> @@ -1048,6 +1050,13 @@ static inline struct bpf_subprog_info *subprog_info(struct bpf_verifier_env *env, int subprog)
>>   	return &env->subprog_info[subprog];
>>   }
>>
>> +static inline bool bpf_ret_reg_pair(struct bpf_verifier_env *env, int subprog)
>> +{
>> +	return subprog_info(env, subprog)->ret_reg_pair;
>> +}
>> +
>> +bool bpf_kfunc_ret_reg_pair(struct bpf_verifier_env *env, struct bpf_insn *insn);
>> +
>>   struct bpf_call_summary {
>>   	u8 num_params;
>>   	bool is_void;
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -382,29 +382,62 @@ bool bpf_subprog_is_global(const struct bpf_verifier_env *env, int subprog)
>>   	return aux && aux[subprog].linkage == BTF_FUNC_GLOBAL;
>>   }
>>
>> -static bool subprog_returns_void(struct bpf_verifier_env *env, int subprog)
>> +static const struct btf_type *subprog_ret_type(struct bpf_verifier_env *env, int subprog)
>>   {
>> -	const struct btf_type *type, *func, *func_proto;
>> +	const struct btf_type *func, *func_proto;
>>   	const struct btf *btf = env->prog->aux->btf;
>>   	u32 btf_id;
>>
>> +	if (!btf || !env->prog->aux->func_info)
>> +		return NULL;
>> +
>>   	btf_id = env->prog->aux->func_info[subprog].type_id;
>>
>> +	/* Both already validated by prepare_btf_func() at prog load. */
>>   	func = btf_type_by_id(btf, btf_id);
>> -	if (verifier_bug_if(!func, env, "btf_id %u not found", btf_id))
>> -		return false;
>> -
>>   	func_proto = btf_type_by_id(btf, func->type);
>> -	if (!func_proto)
>> -		return false;
>>
>> -	type = btf_type_skip_modifiers(btf, func_proto->type, NULL);
>> -	if (!type)
>> -		return false;
>> +	return btf_type_skip_modifiers(btf, func_proto->type, NULL);
>> +}
>>
>> -	return btf_type_is_void(type);
>> +static bool subprog_returns_void(struct bpf_verifier_env *env, int subprog)
>> +{
>> +	const struct btf_type *type = subprog_ret_type(env, subprog);
>> +
>> +	return type && btf_type_is_void(type);
>> +}
>> +
>> +static u32 ret_regs_cnt(u32 size)
>> +{
>> +	return size > 8 && size <= 16 ? 2 : 1;
>>   }
>>
>> +static void bpf_compute_subprog_ret_regs(struct bpf_verifier_env *env)
>> +{
>> +	const struct btf *btf = env->prog->aux->btf;
>> +	const struct btf_type *type;
>> +	int subprog;
>> +	u32 size;
>> +
>> +	for (subprog = 0; subprog < env->subprog_cnt; subprog++) {
> Does this loop need to start at subprog 0?  The main program (subprog 0)
> delivers its exit code in R0 alone and never uses the R0:R2 calling
> convention, so checking its BTF return type can incorrectly trigger
> jit_required even though no R2 propagation is needed.

You are right. We only need to start from subprog 1.

>
>> +		type = subprog_ret_type(env, subprog);
>> +		if (!type || !(btf_type_is_struct(type) || btf_type_is_scalar(type)))
>> +			continue;
>> +		if (IS_ERR(btf_resolve_size(btf, type, &size)))
>> +			continue;
>> +		if (ret_regs_cnt(size) > 1) {
>> +			subprog_info(env, subprog)->ret_reg_pair = true;
>> +			/*
>> +			 * The R0:R2 return convention is only implemented in
>> +			 * the JIT: the interpreter propagates BPF_R0 alone out
>> +			 * of a subprogram, so a caller reading R2 would see a
>> +			 * stale value.
>> +			 */
>> +			env->prog->jit_required = 1;
>> +		}
>> +	}
>> +}
> Additionally, should jit_required be set for subprogs whose BTF prototypes
> are marked unreliable?  btf_check_subprog_call() can flag a mismatch with
> prog->aux->func_info_aux[subprog].unreliable = true, and check_func_call()
> only aborts on -EFAULT, so a static callee with an aggregate return can
> load today without using the BTF return type. Subprogs subsequently deleted
> by bpf_opt_remove_dead_code() (which runs after this pass) also don't need
> jit_required set.

We cannot just mark jit_required if BTF prototypes is marked unreliable.
This is too conservative. Marking unreliable may not have 2 return registers,
e.g., if a argument is 'int *p', the subprog will mark unreliable. but
it may not have 2 return registers.

>
> This changes behavior for existing programs: on CONFIG_BPF_JIT=n or
> net.core.bpf_jit_enable=0, such programs would now fail in
> bpf_fixup_call_args() with "program requires BPF JIT compiler but it is
> not available", whereas they previously verified and ran on the
> interpreter.

If jit is not enabled, let us just assume returning R0 only.

>
> [ ... ]
>
>> @@ -20333,6 +20381,9 @@
>>   	if (ret < 0)
>>   		goto skip_full_check;
>>
>> +	/* must precede the first bpf_ret_reg_pair() user below */
>> +	bpf_compute_subprog_ret_regs(env);
>> +
>>   	ret = bpf_compute_live_registers(env);
>>   	if (ret < 0)
>>   		goto skip_full_check;
>
> ---
> 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/31740414277