Re: [PATCH bpf-next v6 06/10] bpf: Add verifier support for 16-byte returns in R0:R2

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The newly added support for kfuncs returning structs by value is mathematically unreachable.
--

--- Patch [6]: [PATCH bpf-next v6 06/10] bpf: Add verifier support for 16-byte returns in R0:R2 ---
commit 438f13b526c411cf77d7536a3436a0cc4625dbe4
Author: Yonghong Song <[email protected]>

bpf: Add verifier support for 16-byte returns in R0:R2

This patch adds BPF verifier support for handling 16-byte returns in the
R0:R2 register pair, utilizing LLVM 23 capabilities. It models this
register pair at calls to global/static subprograms and kfuncs, allowing
kfuncs to return structs composed of scalars.

Link: https://github.com/llvm/llvm-project/pull/190894
Link: https://github.com/llvm/llvm-project/pull/206876

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -13967,10 +13997,25 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  	}
>  
>  	if (btf_type_is_scalar(t)) {
> -		mark_reg_unknown(env, regs, BPF_REG_0);
> +		mark_kfunc_ret_regs(env, regs, t->size);
>  		if (meta.btf == btf_vmlinux && (meta.func_id == special_kfunc_list[KF_bpf_res_spin_lock] ||
>  		    meta.func_id == special_kfunc_list[KF_bpf_res_spin_lock_irqsave]))
>  			__mark_reg_const_zero(env, &regs[BPF_REG_0]);
> +	} else if (btf_type_is_struct(t)) {

[Severity: Medium]
Is this newly added code for kfunc struct returns actually reachable
during program load?

Looking at how kfuncs are processed earlier in the verifier,
bpf_add_kfunc_call() calls btf_distill_func_proto(), which appears to
still explicitly reject struct return types:

kernel/bpf/btf.c:btf_distill_func_proto() {
    ...
    if (ret < 0 || btf_type_is_struct(t)) {
        bpf_log(log,
            "The function %s return type %s is unsupported.\n",
            tname, btf_type_str(t));
        return -EINVAL;
    }
    ...
}

Since btf_distill_func_proto() is called during program load before
check_kfunc_call() is ever reached, won't any BPF program calling a
struct-returning kfunc be unconditionally rejected, making this new
branch dead code?

> +		/*
> +		 * The returned struct comes back as raw register bits modeled
> +		 * as an unknown scalar, so it must contain only scalars:
> +		 * otherwise a pointer field would be laundered into a scalar
> +		 * and escape provenance and reference tracking.
> +		 */
> +		if (!__btf_type_is_scalar_struct(env, desc_btf, t, 0)) {
> +			verbose(env,
> +				"kernel function %s returns %s %s that is not composed of scalars\n",
> +				func_name, btf_type_str(t),
> +				btf_name_by_offset(desc_btf, t->name_off));
> +			return -EINVAL;
> +		}
> +		mark_kfunc_ret_regs(env, regs, t->size);
>  	} else if (btf_type_is_ptr(t)) {
>  		ptr_type = btf_type_skip_modifiers(desc_btf, t->type, &ptr_type_id);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6