Re: [PATCH bpf-next v3 10/45] bpf: Find the registers and stack slots that later checks depend on

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bpf: false R0 dependency for BPF_END | BPF_TO_BE in flow_compute_need_const()
--

commit 6774fce732def6414deae7e4151e138be57cfe46
Author: Alexei Starovoitov <ast@kernel.org>

bpf: Find the registers and stack slots that later checks depend on

This commit adds backward dataflow analysis in kernel/bpf/flow.c to identify
registers and stack slots that subsequent verifier checks depend on, used both
to prevent joining states with differing constants and to determine whether
loop-modified values are needed after a counted loop.

> diff --git a/kernel/bpf/flow.c b/kernel/bpf/flow.c
> index 837d5054c2a3d..1eecd9ee5123b 100644
> --- a/kernel/bpf/flow.c
> +++ b/kernel/bpf/flow.c

[ ... ]

> @@ -821,3 +889,259 @@ int bpf_flow_compute_kinds(struct bpf_verifier_env *env)
>  	kvfree(ret);
>  	return err;
>  }
> +

[ ... ]

> +static int flow_compute_need_const(struct bpf_verifier_env *env, struct flow_need **pneed,
> +				   enum flow_need_kind kind)
> +{

[ ... ]

> +			} else if ((class == BPF_ALU || class == BPF_ALU64) &&
> +				   (out.regs & BIT(insn->dst_reg))) {
> +				op = BPF_OP(insn->code);
> +				reg = BPF_SRC(insn->code) == BPF_X;
> +				/* the result is known when what it is made of is */
> +				keep = op != BPF_MOV;
> +				from_src = reg;
> +				if (jump)
> +					from_src = flow_is_copy(insn);
> +				if (kind == FLOW_NEED_JUMP)
> +					keep = flow_is_shift(insn) && !reg;
> +				if (kind == FLOW_NEED_BASE) {
> +					keep = !reg && (op == BPF_ADD || op == BPF_SUB);
> +					from_src = reg && op == BPF_MOV;
> +				}
> +				if (!keep)
> +					in.regs &= ~BIT(insn->dst_reg);
> +				if (from_src)
> +					in.regs |= BIT(insn->src_reg);

[Severity: Medium]
As reported by bot+bpf-ci@kernel.org on v2 [1], in flow_compute_need_const()
in kernel/bpf/flow.c, does a BPF_ALU | BPF_END | BPF_TO_BE instruction falsely
mark BPF_REG_0 as needed in in.regs?

Because BPF_TO_BE (0x08) shares the same bit value as BPF_X (0x08) in
BPF_SRC(insn->code), reg evaluates to true on a big-endian byte-swap
instruction even though BPF_END is a unary in-place operation on dst_reg with
insn->src_reg == BPF_REG_0.

For FLOW_NEED_ARG, FLOW_NEED_STACK_OFF, FLOW_NEED_PTR_OFF, and
FLOW_NEED_VALUE_OFF, from_src remains equal to reg, so:

	if (from_src)
		in.regs |= BIT(insn->src_reg);

sets BIT(BPF_REG_0) whenever the byte-swapped dst_reg is needed downstream.
Wouldn't this prevent joining states where R0 holds different constants prior
to a byte-swap and cause bpf_flow_is_needed() to report R0 as needed at loop
headers?

Should reg exclude op == BPF_END (for example, reg = BPF_SRC(insn->code) ==
BPF_X && op != BPF_END)?

[1] https://lore.kernel.org/r/506d7ef52474ac7df140bd5f84ac7b7bcf359769a1a46b05aba0751fcdf065af@mail.kernel.org

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