Re: [PATCH bpf-next v4 30/46] bpf: What a call returns in each iteration is like what the loop loads

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] bpf: missing call OPAQUE handling in
  made_from_partial_spill()
--

commit 17f294cedba72c038cc095f26f37091b91a7fbf0
Author: Alexei Starovoitov <ast@kernel.org>

bpf: What a call returns in each iteration is like what the loop loads

This commit assigns an OPAQUE scalar evolution expression with parameter 1
to r0 after a BPF_CALL, allowing loop registers updated from call return
values to be marked in fresh_regs after walking short loops.

> diff --git a/kernel/bpf/scev.c b/kernel/bpf/scev.c
> index d8c847efbd7f3..06f485853df13 100644
> --- a/kernel/bpf/scev.c
> +++ b/kernel/bpf/scev.c
> @@ -852,8 +852,11 @@ static int transfer(struct bpf_verifier_env *env, struct env *e, int idx)
>  		break;
>  	case BPF_JMP:
>  	case BPF_JMP32:
> -		if (opcode == BPF_CALL)
> +		if (opcode == BPF_CALL) {
>  			forget_call_regs(e);
> +			/* made anew, like what is loaded. The parameter tells the two apart. */
> +			return replace_reg(scev, e, BPF_REG_0, expr1(scev, OPAQUE, 1));
> +		}

[Severity: Medium]
Does made_from_partial_spill() also need to treat a call's OPAQUE
expression like UNKNOWN?

Before this change, BPF_CALL left r0 as UNKNOWN_EXPR_ID, so a known live
partial stack spill updated from a subprogram or helper return value had
made_from_partial_spill() return true and jumped to cant_widen in
bpf_compute_loop_iters():

kernel/bpf/scev.c:made_from_partial_spill() {
	const struct expr *e = &scev->exprs[id];
	u32 i;

	if (lvl >= EXPR_STACK_DEPTH || e->op == UNKNOWN)
		return true;
    ...
}

kernel/bpf/scev.c:bpf_compute_loop_iters() {
    ...
		if (is_unbound_scev_reg(env, cur_func, r)) {
			if (is_known_partial_spill(cur_func, r) &&
			    bpf_stack_slot_alive(env, st->curframe, (r - MAX_BPF_REG) * 2 + 1) &&
			    made_from_partial_spill(scev, cur_func, header_env,
						    header_env->reg2expr[r], r, 0))
				goto cant_widen;
			continue;
		}
    ...
}

Because op_params_num(OPAQUE) is 0 and e->op is no longer UNKNOWN,
made_from_partial_spill() now returns false when a partial spill is set
from r0 after a call.  bpf_compute_loop_iters() then skips cant_widen,
leaving walk false and the slot out of fresh_regs, which causes join_slot()
to widen the partial spill at the loop head and causes a regression where
valid programs with bounded memory accesses using that spill are rejected.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107033758.1547725-1-alexei.starovoitov@gmail.com?part=30