Re: [PATCH bpf-next v4 46/46] selftests/bpf: Test progs without bpf_for(), zero, can_loop and such

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] selftests/bpf: missing BTF kfunc skip check in
  test_plain_c_test_wakeup_source()
- [Low] selftests/bpf: missing __skip_without_arena in plain_c
  arena_htab and arena_list
--

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

selftests/bpf: Test progs without bpf_for(), zero, can_loop and such

This commit adds plain-C versions of existing BPF selftest programs
without verifier-oriented loop macros and workarounds, along with test
runners in prog_tests/plain_c.c.

> diff --git a/tools/testing/selftests/bpf/prog_tests/plain_c.c b/tools/testing/selftests/bpf/prog_tests/plain_c.c
> new file mode 100644
> index 0000000000000..b7305118a4541
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/plain_c.c
> @@ -0,0 +1,160 @@

[ ... ]

> +/* prog_tests/arena_list.c: add 1000 elems, delete them, compare the sums */
> +static void arena_list_add_del(void)
> +{
> +	LIBBPF_OPTS(bpf_test_run_opts, opts);
> +	struct plain_c_arena_list *skel;
> +	int cnt = 1000, sum = cnt * (cnt - 1) / 2;
> +
> +	skel = plain_c_arena_list__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "open_and_load"))
> +		return;
> +	skel->bss->cnt = cnt;
> +	ASSERT_OK(bpf_prog_test_run_opts(bpf_program__fd(skel->progs.arena_list_add), &opts),
> +		  "add");
> +	if (skel->bss->skip) {
> +		test__skip();
> +		goto out;
> +	}

[ ... ]

> +void test_plain_c_arena_htab(void) { RUN(arena_htab); }
> +void test_plain_c_arena_htab_asm(void) { RUN(arena_htab_asm); }
> +
> +void test_plain_c_arena_list(void)
> +{
> +	RUN(arena_list);
> +	if (test__start_subtest("run"))
> +		arena_list_add_del();
> +}

[ ... ]

> +void test_plain_c_test_wakeup_source(void) { RUN(test_wakeup_source); }

[Severity: Medium]
Does test_plain_c_test_wakeup_source() fail when CONFIG_PM_SLEEP is not
enabled in the kernel (for example on s390x, where ARCH_SUSPEND_POSSIBLE
and ARCH_HIBERNATION_POSSIBLE are not supported)?

plain_c_test_wakeup_source.c declares bpf_wakeup_sources_read_lock(),
bpf_wakeup_sources_read_unlock(), and bpf_wakeup_sources_get_head() as
strong __ksym externs.  Unlike test_wakeup_source() in
prog_tests/wakeup_source.c, which checks vmlinux BTF for
bpf_wakeup_sources_get_head and calls test__skip() when absent,
test_plain_c_test_wakeup_source() unconditionally invokes
RUN(test_wakeup_source), causing bpf_object__load() to fail with
unresolved kfunc externs.

> diff --git a/tools/testing/selftests/bpf/progs/plain_c_arena_htab.c b/tools/testing/selftests/bpf/progs/plain_c_arena_htab.c
> new file mode 100644
> index 0000000000000..10a50b5058ddb
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/plain_c_arena_htab.c
> @@ -0,0 +1,222 @@

[ ... ]

> +SEC("syscall")
> +__success __retval(0)
> +#if !defined(__clang__) && defined(BPF_ARENA_FORCE_ASM)
> +__skip("GCC calls htab_init(): a global function gets an arena pointer as a scalar")
> +#endif
> +int arena_htab_llvm(void *ctx)
> +{
> +#if defined(__BPF_FEATURE_ADDR_SPACE_CAST) || defined(BPF_ARENA_FORCE_ASM)

[ ... ]

> +#else
> +	skip = true;
> +#endif
> +	return 0;
> +}

[Severity: Low]
Should arena_htab_llvm() (when BPF_ARENA_FORCE_ASM is not defined) and
arena_list_add() / arena_list_del() in plain_c_arena_list.c be annotated
with __skip_without_arena, similar to plain_c_arena_strsearch.c and
plain_c_verifier_arena_large.c?

Without __BPF_FEATURE_ADDR_SPACE_CAST (such as in test_progs-bpf_gcc or
test_progs-no_alu32), these programs compile to `skip = true; return 0;`,
and RUN(arena_htab) and RUN(arena_list) load the no-op stubs via
test_loader without checking skel->bss->skip, reporting OK instead of
SKIP.  Also, arena_list_add_del() in prog_tests/plain_c.c calls
test__skip() without printing a reason message.

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