Re: [PATCH bpf-next v3 01/45] bpf: Add may_goto to the loops that the verifier marks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] bpf: bpf_insert_loop_guards() logs shifted insn index instead of orig_idx
--

commit 4a3883a17feaa6708a44e871a9a6eab87f5197bc
Author: Alexei Starovoitov <ast@kernel.org>

bpf: Add may_goto to the loops that the verifier marks

This commit introduces bpf_insert_loop_guards() to insert may_goto
instructions in front of loop instructions marked by the verifier, along
with per-subprogram budget exit sequences when needed.

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index f95af2ca3591b..6fd79c2fbdc1e 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
> @@ -587,6 +587,225 @@ static int bpf_adj_moved_insn(struct bpf_insn *insn, u32 delta)

[ ... ]

> +int bpf_insert_loop_guards(struct bpf_verifier_env *env)
> +{
> +	struct bpf_insn_aux_data *aux = env->insn_aux_data;
> +	int len = env->prog->len, i, r, s, to, n, jmp, err = 0;
> +	struct bpf_insn *patch, *insn;
> +	int *target, *leave, *moved;
> +	bool indirect;
> +	u16 *zero;
> +	s64 off;
> +
> +	for (i = 0; i < len; i++)
> +		if (aux[i].loop_guard)
> +			break;
> +	if (i == len)
> +		return 0;
> +
> +	/*
> +	 * Where the subprog has 'r0 = 0; exit', 0 if it has none. And how far that moved
> +	 * the subprog: guard_to is from the time before.
> +	 */
> +	leave = kvcalloc(2 * env->subprog_cnt, sizeof(int), GFP_KERNEL_ACCOUNT);
> +	if (!leave)
> +		return -ENOMEM;
> +	moved = leave + env->subprog_cnt;
> +	for (s = 0; s < env->subprog_cnt && !err; s++) {
> +		leave[s] = append_budget_exit(env, s);
> +		if (leave[s] < 0)
> +			err = leave[s];
> +	}
> +	/* A guard cannot jump into a patch that is in the list: 'r0 = 0' has no index yet. */
> +	err = err ?: bpf_patch_list_commit(env);

[ ... ]

> +	for (i = 0; i < len; i++) {
> +		to = target[i];
> +		if (to == -1)
> +			continue;
> +		/*
> +		 *	may_goto to		or	may_goto +1
> +		 *	insn				goto +N
> +		 *					r0 = 0, ...
> +		 *					exit
> +		 *					insn
> +		 */
> +		n = 0;
> +		if (zero[i]) {
> +			patch[n++] = BPF_RAW_INSN(BPF_JMP | BPF_JCOND, 0, 0, 1, 0);
> +			patch[n++] = BPF_JMP_A(hweight16(zero[i]) + 1);
> +			for (r = BPF_REG_0; r < BPF_REG_FP; r++)
> +				if (zero[i] & BIT(r))
> +					patch[n++] = BPF_MOV64_IMM(r, 0);
> +		}
> +		jmp = n++;
> +		/* What is behind moves with this patch. The other patches are not there yet. */
> +		if (to == -2)
> +			off = 0;
> +		else if (to > i)
> +			off = to - i + n - jmp - 1;
> +		else
> +			off = to - (i + jmp) - 1;
> +		if (off > S16_MAX || off < S16_MIN) {
> +			verbose(env, "exit of the loop is too far from insn %d for may_goto\n", i);
> +			err = -ERANGE;
> +			break;
> +		}
> +		patch[jmp] = BPF_RAW_INSN(BPF_JMP | BPF_JCOND, 0, 0, off, 0);
> +		if (to == -2)
> +			patch[jmp] = budget_exit_insn(env, bpf_find_containing_subprog(env, i));
> +		patch[n] = env->prog->insnsi[i];
> +		/* A jump to itself is a back edge. It goes through may_goto like the others. */
> +		if (jumps_to_itself(&patch[n]))
> +			patch[n].off -= n;
> +		else
> +			err = bpf_adj_moved_insn(&patch[n], n);
> +		if (err) {
> +			verbose(env, "insn %d cannot be patched due to 16-bit range\n", i);
> +			break;
> +		}

[Severity: Low]
As reported by Sashiko on v2 [1], should these two verbose() calls in
bpf_insert_loop_guards() log aux[i].orig_idx instead of i?

The first pass in bpf_insert_loop_guards() calls append_budget_exit() for
each subprogram with a near loop guard that leaves the subprogram, and
commits those patches via bpf_patch_list_commit(env). Each appended budget
exit adds 2 or 3 instructions to the end of its subprogram, shifting the
instruction index i in all subsequent subprograms by moved[s]. If the
second pass then fails because off overflows 16 bits or
bpf_adj_moved_insn() returns -ERANGE, logging i reports the shifted index
rather than the original instruction index from the user program (unlike
bpf_patch_list_insert(), bpf_patch_list_commit(), and
bpf_convert_ctx_accesses() in kernel/bpf/fixups.c, which log orig_idx).

Similarly, in kernel/bpf/verifier.c:bpf_check():

	if (ret == 0)
		ret = bpf_insert_loop_guards(env);

skip_full_check:
	kvfree(env->explored_states);

	/* might decrease stack depth, keep it before passes that
	 * allocate additional slots.
	 */
	if (ret == 0)
		ret = bpf_remove_fastcall_spills_fills(env);

	if (ret == 0)
		ret = check_max_stack_depth(env);

	/* instruction rewrites happen after this point */
	if (ret == 0)
		ret = bpf_optimize_bpf_loop(env);

bpf_insert_loop_guards() is called before check_max_stack_depth(), ahead of
the "instruction rewrites happen after this point" boundary. Because helpers
called by check_max_stack_depth() (such as find_call_insn() in
check_exception_cb() and check_recursion(), as well as check_callbacks())
log raw instruction indices in env->prog->insnsi rather than orig_idx,
rewriting the program in bpf_insert_loop_guards() before
check_max_stack_depth() will also shift the instruction indices reported by
those diagnostics. Should bpf_insert_loop_guards() be moved after
check_max_stack_depth() with the other instruction rewrite passes?

[1] https://lore.kernel.org/r/20261105204513.2230344-2-alexei.starovoitov@gmail.com

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