Re: [PATCH bpf-next v2 34/35] selftests/bpf: Test progs that run out of budget

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests/bpf: use RUN_JITED() in serial_test_verifier_budget()
- [Medium] selftests/bpf: loop_in_main() and loop_in_main_rcu() never run out of budget
--

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

selftests/bpf: Test progs that run out of budget

This commit adds BPF verifier selftests for programs that exhaust their loop
or recursion runtime budget, along with a test for packet pointer invalidation
after indirect calls in verifier_callx.c.

> diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
> index 1424dc3036c5d..5b3eacb36a6cc 100644
> --- a/tools/testing/selftests/bpf/prog_tests/verifier.c
> +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c

[ ... ]

> @@ -235,6 +236,8 @@ void test_verifier_bounds_mix_sign_unsign(void) { RUN(verifier_bounds_mix_sign_u
>  void test_verifier_bpf_get_stack(void)        { RUN(verifier_bpf_get_stack); }
>  void test_verifier_bpf_trap(void)             { RUN(verifier_bpf_trap); }
>  void test_verifier_bswap(void)                { RUN(verifier_bswap); }
> +/* Serial: many progs spin for 250 ms in an RCU read-side critical section, see timed_may_goto.c */
> +void serial_test_verifier_budget(void)        { RUN(verifier_budget); }

[Severity: Medium]
Should serial_test_verifier_budget() use RUN_JITED(verifier_budget) instead of
RUN(verifier_budget) (matching test_verifier_callx() and
test_verifier_callx_rodata())?

When BPF JIT is disabled (for example with bpf_jit_enable=0),
is_jit_enabled() is false and prog->jit_requested is 0, while none of the
subtests in verifier_budget.c specify __load_if_JITed():

1. callback_arms_itself() has no architecture filter and passes a callback to
bpf_timer_set_callback(), which jit_subprogs() rejects with "callbacks are not
allowed in non-JITed programs".

2. Subtests with recursive subprograms (such as recursion(),
recursion_too_deep(), recursion_ends_in_the_middle(), and others) fail in
check_recursion() with -EOPNOTSUPP because bpf_prog_has_invocation_budget()
requires prog->jit_requested.

3. Subtests using unwind_goto or unwind_exit (such as loop_in_subprog_caught()
and loop_in_subprog_locked()) set prog->jit_required = 1 in
check_unwind_insns() and fail to load without JIT.

4. guard_all_regs_live() asserts per-cpu timed may_goto instructions
("r0 = &(void __percpu *)(r0)") via __xlated, which bpf_do_misc_fixups() only
emits when timed_may_goto is enabled.

5. In may_goto_stays_in_loop(), without timed may_goto in JIT,
bpf_do_misc_fixups() expands both may_goto_self and the verifier-added
may_goto at the loop head with separate stack counters via may_goto_expand().
Each time may_goto_self counts down 8M iterations and jumps to the loop head,
the outer may_goto decrements once and may_goto_self re-initializes to 8M,
resulting in 2^46 iterations in the interpreter.

> diff --git a/tools/testing/selftests/bpf/progs/verifier_budget.c b/tools/testing/selftests/bpf/progs/verifier_budget.c
> new file mode 100644
> index 0000000000000..1aa5b3eff712d
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/verifier_budget.c
> @@ -0,0 +1,1873 @@

[ ... ]

> +/* The verifier doesn't know what is in there */
> +unsigned long zero;
> +unsigned long one = 1;

[ ... ]

> +SEC("socket")
> +__description("budget: a loop in the main prog ends it")
> +__arch_x86_64
> +__arch_arm64
> +__success __retval(1)
> +__naked void loop_in_main(void)
> +{
> +	asm volatile (
> +		"r6 = %[zero] ll;"
> +	"1:"
> +		"r1 = *(u64 *)(r6 + 0);"
> +		"if r1 != 0 goto 1b;"
> +		"r0 = 1;"
> +		"exit;"
> +		:
> +		: __imm_addr(zero)
> +		: __clobber_all
> +	);
> +}
> +
> +SEC("socket")
> +__description("budget: a loop in the main prog that holds something is left")
> +__arch_x86_64
> +__arch_arm64
> +__success __retval(1)
> +__naked void loop_in_main_rcu(void)
> +{
> +	asm volatile (
> +		"r6 = %[zero] ll;"
> +		"call %[bpf_rcu_read_lock];"
> +	"1:"
> +		"r1 = *(u64 *)(r6 + 0);"
> +		"if r1 != 0 goto 1b;"
> +		"call %[bpf_rcu_read_unlock];"
> +		"r0 = 1;"
> +		"exit;"
> +		:
> +		: __imm_addr(zero),
> +		  __imm(bpf_rcu_read_lock),
> +		  __imm(bpf_rcu_read_unlock)
> +		: __clobber_all
> +	);
> +}

[Severity: Medium]
Should loop_in_main() and loop_in_main_rcu() load the address of one instead
of zero (and should loop_in_main() expect __retval(0))?

Because r6 is initialized from zero, "r1 = *(u64 *)(r6 + 0);" loads 0 on the
first trip and "if r1 != 0 goto 1b;" falls through immediately without
spinning or running out of budget.

In loop_in_main(), falling through executes "r0 = 1; exit;" instead of timing
out on the verifier-added may_goto and jumping to the budget exit ("r0 = 0;
exit;") when the main prog holds nothing. In loop_in_main_rcu(), the loop
exits via the normal branch condition on the first trip rather than timing out
via the verifier-added may_goto while holding the RCU read lock.

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