Re: [PATCHSET SLOP RFC 3/6] bpf: Support __arena suffix on struct_ops stub arguments

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Tail-calling another struct_ops program causes double-subtraction of the arena base address, corrupting arena pointers in the context.
--

--- Patch [3]: [PATCHSET SLOP RFC 3/6] bpf: Support __arena suffix on struct_ops stub arguments ---
commit 7fff65a383d9791611afe2d53ac2633d4bac8dda
Author: Tejun Heo <[email protected]>

bpf: Support __arena suffix on struct_ops stub arguments

A struct_ops callback cannot receive an arena pointer directly, so passing
one takes two steps. The pointer arrives as a bare u64 that the callback
casts, and because the two sides address the arena through different bases
it also has to be rebased by hand on the way in. Add the __arena stub
argument suffix and both steps go away.

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
> @@ -680,6 +680,64 @@ int bpf_opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
[ ... ]
> +static int gen_arena_arg_prologue(struct bpf_verifier_env *env,
> +				  struct bpf_insn *insn_buf)
> +{
[ ... ]
> +	for (i = 0; i < aux->ctx_arg_info_size; i++) {
> +		int off;
> +
> +		if (base_type(aux->ctx_arg_info[i].reg_type) != PTR_TO_ARENA)
> +			continue;
> +		off = aux->ctx_arg_info[i].offset;
> +		insn_buf[cnt++] = BPF_LDX_MEM(BPF_DW, BPF_REG_2, BPF_REG_1, off);
> +		insn_buf[cnt++] = blinding ?
> +			BPF_JMP_REG(BPF_JEQ, BPF_REG_2, BPF_REG_AX, 2) :
> +			BPF_JMP_IMM(BPF_JEQ, BPF_REG_2, 0, 2);
> +		insn_buf[cnt++] = BPF_ALU64_REG(BPF_SUB, BPF_REG_2, base_reg);
> +		insn_buf[cnt++] = BPF_ZEXT_REG(BPF_REG_2);
> +		insn_buf[cnt++] = BPF_STX_MEM(BPF_DW, BPF_REG_1, BPF_REG_2, off);
> +	}
> +	insn_buf[cnt++] = env->prog->insnsi[0];
> +	return cnt;
> +}

[Severity: High]
Does this in-place modification cause an issue if the program performs a tail
call to another struct_ops program?

Since tail calls pass the exact same context array to the callee, and the
callee will also have this prologue injected, it seems the already-rebased
32-bit offset will be read and the arena base address will be subtracted a
second time. This could severely corrupt the arena pointer offset.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -18729,10 +18730,17 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
[ ... ]
> -	for (i = 0; i < st_ops_desc->arg_info[member_idx].cnt; i++) {
> -		if (st_ops_desc->arg_info[member_idx].info[i].refcounted) {
> +	arg_info = &st_ops_desc->arg_info[member_idx];
> +	for (i = 0; i < arg_info->cnt; i++) {
> +		const struct bpf_ctx_arg_aux *info = &arg_info->info[i];
> +
> +		if (info->refcounted)
>  			has_refcounted_arg = true;
> -			break;
> +		if (base_type(info->reg_type) == PTR_TO_ARENA && !prog->aux->arena) {
> +			verbose(env,
> +				"arena argument of %s requires a program with an associated arena\n",
> +				mname);
> +			return -EINVAL;
>  		}
>  	}

[Severity: High]
Should we also restrict tail calls for programs with __arena arguments here?

Right below this loop, check_struct_ops_btf_id() blocks tail calls if
has_refcounted_arg is true:

    for (i = 0; i < env->subprog_cnt; i++) {
        if (has_refcounted_arg && env->subprog_info[i].has_tail_call) {
            verbose(env, "program with __ref argument cannot tail call\n");
            return -EINVAL;
        }
    }

Since gen_arena_arg_prologue() modifies the context in-place, blocking
tail calls for programs with arena arguments would prevent the double
subtraction issue entirely.

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