Re: [PATCH bpf-next v2 35/35] selftests/bpf: Test callx of one of a set of functions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/bpf: missing __load_if_JITed() in callx_one_of_two_not_written()
--

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

selftests/bpf: Test callx of one of a set of functions

This commit adds verifier selftests covering indirect calls (callx) to a set
of functions loaded from read-only tables, callback verification, state limits,
stack arguments, and BTF-declared argument liveness.

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/verifier_stack_arg.c b/tools/testing/selftests/bpf/progs/verifier_stack_arg.c
> index 9cd4fee561b23..dfb16434572dc 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_stack_arg.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_stack_arg.c
> @@ -530,6 +530,60 @@ __naked void null_check_of_stack_arg(void)
>  	: __clobber_all);
>  }
>  
> +static __naked __noinline __used long adds_arg6_arg7(long a, long b, long c, long d, long e,
> +						     long f, long g)
> +{
> +	asm volatile (
> +	"r0 = *(u64 *)(r11 + 8);"
> +	"r1 = *(u64 *)(r11 + 16);"
> +	"r0 += r1;"
> +	"exit;"
> +	::: __clobber_all);
> +}
> +
> +static __naked __noinline __used long subs_arg6_arg7(long a, long b, long c, long d, long e,
> +						     long f, long g)
> +{
> +	asm volatile (
> +	"r0 = *(u64 *)(r11 + 8);"
> +	"r1 = *(u64 *)(r11 + 16);"
> +	"r0 -= r1;"
> +	"exit;"
> +	::: __clobber_all);
> +}
> +
> +SEC("tc")
> +__description("stack_arg: callx of one of two functions, args are not written")
> +__failure __msg("callee expects 7 args, stack arg1 is not initialized")
> +__naked void callx_one_of_two_not_written(void)
> +{
> +	asm volatile (
> +	".pushsection .rodata,\"a\",@progbits;"
> +	".balign 8;"
> +"two_funcs_%=:"
> +	".quad adds_arg6_arg7;"
> +	".quad subs_arg6_arg7;"
> +	".type two_funcs_%=, @object;"
> +	".size two_funcs_%=, .-two_funcs_%=;"
> +	".popsection;"
> +	"call %[bpf_get_prandom_u32];"
> +	"r0 &= 1;"
> +	"r0 <<= 3;"
> +	"r2 = two_funcs_%= ll;"
> +	"r2 += r0;"
> +	"r6 = *(u64 *)(r2 + 0);"
> +	"r1 = 1;"
> +	"r2 = 2;"
> +	"r3 = 3;"
> +	"r4 = 4;"
> +	"r5 = 5;"
> +	"callx r6;"
> +	"exit;"
> +	:
> +	: __imm(bpf_get_prandom_u32)
> +	: __clobber_all);
> +}

[Severity: Medium]
Should callx_one_of_two_not_written() be marked with __load_if_JITed() (and
guarded by CAN_USE_CALLX)?

In tools/testing/selftests/bpf/prog_tests/verifier.c, test_verifier_stack_arg()
runs the suite with RUN(verifier_stack_arg) rather than RUN_JITED():

    void test_verifier_stack_arg(void)            { RUN(verifier_stack_arg); }

When BPF JIT is disabled, env->prog->jit_requested is false. Unlike direct-call
failure tests in verifier_stack_arg.c that reach check_outgoing_stack_args()
during do_check() before bpf_fixup_call_args() checks jit_requested, the
.rodata function pointer read and callx in callx_one_of_two_not_written() hit
check_func_ptr_read() -> require_callx_jit() (and check_func_callx() ->
require_callx_jit()) during do_check():

kernel/bpf/verifier.c:require_callx_jit() {
	if (!env->prog->jit_requested) {
		verbose(env, "JIT is required to use callx\n");
		return -EOPNOTSUPP;
	}
    ...
}

Wouldn't running verifier_stack_arg with JIT disabled fail because the verifier
rejects the program with "JIT is required to use callx" before reaching
check_func_callx_set() -> check_outgoing_stack_args() to emit "callee expects
7 args, stack arg1 is not initialized"?

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