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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The newly added support for kfuncs and global subprograms returning structs up to 16 bytes by value is unreachable dead code because existing BTF validation logic unconditionally rejects struct return types.
--

--- Patch [6]: [PATCH bpf-next v5 06/11] bpf: Add verifier support for 16-byte returns in R0:R2 ---
commit 9e421b8a810e50b00fab2bcdb9c1dbd32e705d84
Author: Yonghong Song <yonghong.song@linux.dev>

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

This commit adds verifier support for returning values in both R0 and R2 for
types greater than 8 bytes and up to 16 bytes. It models the register pair
for global and static subprograms as well as kfunc calls.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 161d77791bc62..0c68ab3bd6cec 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -13265,10 +13295,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 new logic for kfuncs returning structs actually reachable?

Looking at btf_distill_func_proto() in kernel/bpf/btf.c, it unconditionally
rejects struct return types, meaning programs would fail to load early on:

	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;
	}

Similarly, btf_validate_return_type() blocks global subprograms from returning
structs:

	/* We always accept void or scalars. */
	if (btf_type_is_void(t) || btf_type_is_int(t) || btf_is_any_enum(t))
		return 0;

	return -EOPNOTSUPP;

Does this mean the new struct return handling logic is dead code, and only
static subprograms or __int128 returns can actually use the new convention?

> +		/*
> +		 * 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)) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813200210.1991507-1-yonghong.song@linux.dev?part=6