Re: [PATCH bpf-next v4 09/13] bpf: Enable aggregate return types up to 16 bytes

On 8/10/26 6:24 PM, [email protected] wrote:
>> 8 bytes, R0 alone for <=8 bytes), enabling the R0:R2 register-pair return
> convention end-to-end.
>
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index 6606187ed4f43..5551abcea1d39 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
> [ ... ]
>
>> @@ -7965,7 +7970,7 @@ static int btf_scan_type_tags(struct bpf_verifier_env *env,
>>
>>   /* Check whether the type is a valid return type. */
>>   static int btf_validate_return_type(struct bpf_verifier_env *env, struct btf *btf,
>> -		const struct btf_type *t, int subprog)
>> +		const struct btf_type *t, int subprog, bool is_global)
>>   {
>>   	u32 tags = 0;
>>   	int err;
>> @@ -7988,6 +7993,35 @@ static int btf_validate_return_type(struct bpf_verifier_env *env, struct btf *bt
>>   	if (btf_type_is_void(t) || btf_type_is_int(t) || btf_is_any_enum(t))
>>   		return 0;
>>
>> +	if (btf_type_is_struct(t) && t->size <= 16) {
>> +		/*
>> +		 * A >8 byte struct/union is returned in the R0:R2 register pair.
>> +		 * A global function is verified in isolation, so its caller models
>> +		 * the return as an opaque R0:R2 scalar pair; it must therefore
>> +		 * contain only scalars, otherwise a pointer field would be
>> +		 * laundered into a scalar and escape provenance and reference
>> +		 * tracking. That requirement is enforced here: do_check_common()
>> +		 * propagates the error for global functions and for the main
>> +		 * program.
>> +		 *
>> +		 * A local (static) function is verified inline and its R0:R2 are
>> +		 * copied as precise register state (with the JIT forced on when
>> +		 * the pair is consumed), so a pointer field stays tracked and needs
>> +		 * no such restriction. Accepting it here is not by itself what
>> +		 * makes it legal: btf_check_subprog_call() drops any error other
>> +		 * than -EFAULT. What it avoids is needlessly marking the
>> +		 * subprogram's BTF unreliable.
>> +		 *
>> +		 * The main program (subprog 0) takes the scalar-only path as well,
>> +		 * but its return value is the program's exit code, so a >8 byte
>> +		 * return is rejected separately at BPF_EXIT.
>> +		 */
>> +		bool local_func = subprog && !is_global;
>> +
>> +		if (local_func || __btf_type_is_scalar_struct(env, btf, t, 0))
>> +			return 0;
>> +	}
>> +
>>   	return -EOPNOTSUPP;
>>   }
> Does the comment accurately describe the enforcement mechanism?
>
> The comment states "do_check_common() propagates the error for global
> functions and for the main program." That's true for global functions but
> appears incorrect for the main program. Looking at do_check_common() in
> kernel/bpf/verifier.c:18624:
>
>    if (subprog || env->prog->type == BPF_PROG_TYPE_EXT) {
>            ...
>            ret = btf_prepare_func_args(env, subprog);
>            if (ret)
>                    goto out;            /* <- error propagated */
>    } else {
>            if (env->prog->aux->func_info_aux) {
>                    ret = btf_prepare_func_args(env, 0);
>                    if (ret || sub->arg_cnt != 1 || ...) {
>                            env->prog->aux->func_info_aux[0].unreliable = true;
>                            ...
>                    }
>            }
>            ...
>    }
>    ret = do_check(env);
>
> The main-program arm (subprog 0 of a non-EXT program) uses the return
> value only to set func_info_aux[0].unreliable, then unconditionally
> overwrites ret with do_check(env). The -EOPNOTSUPP from
> btf_validate_return_type() is therefore discarded.
>
> Concretely: a program whose entry function returns a pointer-containing
> struct like 'struct { void *p; }' (size 8) now takes the new struct
> branch, fails __btf_type_is_scalar_struct(), but the resulting error is
> silently dropped. The program continues to be verified.
>
> No memory-safety hole results: check_return_code() still refuses a pointer
> in R0 at exit ('R0 leaks addr as return value'), so the pointer-laundering
> scenario the comment worries about does not actually occur for the main
> program. The issue is that the comment states an enforcement mechanism that
> does not exist.
>
> Should the comment drop "and for the main program" and point at the
> BPF_EXIT check instead?

Yes, will do.

>
> Also: "btf_check_subprog_call() drops any error other than -EFAULT" is
> slightly inaccurate. btf_check_subprog_call() actually returns the error
> and marks the BTF unreliable; its callers, check_func_call() and
> push_callback_call(), do 'if (err == -EFAULT) return err;' and otherwise
> continue.
>
>
> ---
> 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