Re: [RFC PATCH bpf-next v2 2/2] selftests/bpf: add tests for selective kmod BTF loading

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 3297 bytes --]

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

[ ... ]

> +static void kmod_btfs_nonexist(void)
> +{
> +	struct kmod_btfs_nonexist *kmod_btfs_nonexist_skel;
> +
> +	kmod_btfs_nonexist_skel = kmod_btfs_nonexist__open_and_load();
> +	ASSERT_NULL(kmod_btfs_nonexist_skel, "kmod_btfs_nonexist__open_and_load");
> +}

On the path where this assertion unexpectedly fails (i.e. the skeleton
loads when it should not), does the bpf_object leak?  There is no
'kmod_btfs_nonexist__destroy()' call before the function returns.

The other two subtests (kmod_btfs_pass and kmod_btfs_mix) call
'__destroy()'.  Test_progs runs hundreds of subtests in one process,
so leaked fds stay open for the rest of the run.  The usual pattern is:

	if (!ASSERT_NULL(skel, "kmod_btfs_nonexist__open_and_load"))
		kmod_btfs_nonexist__destroy(skel);

Also, the ASSERT_NULL check accepts any load failure at all.  Without
privileges, load_module_btfs() returns 0 early ('skipping module BTFs
loading, missing privileges'), so '__open_and_load()' returns NULL and
this subtest passes without exercising '.kmod_btfs' handling.  The same
is true if vmlinux BTF is unavailable, ftrace cannot hook the target,
or there is a typo in the SEC() name.

The positive baseline that rules those out lives in kmod_btfs_pass, but
test_progs subtests are individually selectable:

	./test_progs -t kmod_btfs/kmod_btfs_nonexist

so running this subtest alone gives green with no baseline.  Would it
be clearer to check the error more precisely (find_kernel_btf_id()
returns -ESRCH here), or load a known-good variant inside the same
subtest?

[ ... ]

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

[ ... ]

> +/* mix of duplicated and unneeded modules */
> +DEFINE_KMOD_BTFS(_needed_kmods) = { "bpf_testmod", "bpf_testmod", "bpf_test_no_cfi" };

Is 'bpf_test_no_cfi' loaded when this subtest runs?  Looking at
test_progs.c, load_bpf_testmod() at lines 2083-2092 loads only
bpf_testmod.ko.  The bpf_test_no_cfi.ko module is inserted and
immediately removed by prog_tests/test_struct_ops_no_cfi.c via
open()/finit_module()/delete_module().

That means the third entry has no module BTF in the system and exercises
the same path as the 'module_nonexist' entry in kmod_btfs_nonexist.c:
the name is simply never matched in load_module_btfs().

That leaves the interesting case untested: a module whose BTF is present
but which the program does not need, which would prove libbpf actually
skips it as the commit message advertises ('providing a mix of repeated
and extra module names').  Would naming a module that is guaranteed
loaded exercise that path?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31664172915