Re: [PATCH bpf-next v4 2/2] selftests/bpf: add tests for selective module BTF loading

> diff --git a/tools/testing/selftests/bpf/prog_tests/btf_module_names.c b/tools/testing/selftests/bpf/prog_tests/btf_module_names.c
> new file mode 100644
> index 000000000000..d5674cad0017
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/btf_module_names.c
> @@ -0,0 +1,93 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <test_progs.h>
> +#include "btf_module_names.skel.h"
> +
> +static void btf_module_names_load(void)
> +{
> +	struct btf_module_names *skel = NULL;
> +	int ret;
> +	static const char *mod_names[] = { "bpf_testmod" };
> +
> +	LIBBPF_OPTS(bpf_object_open_opts, opts,
> +		.btf_module_names = mod_names,
> +		.nr_btf_module_names = 1,
> +	);
> +
> +	skel = btf_module_names__open_opts(&opts);
> +	if (!ASSERT_OK_PTR(skel, "btf_module_names__open_opts"))
> +		goto out;
> +
> +	ret = btf_module_names__load(skel);
> +	ASSERT_OK(ret, "btf_module_names__load");

The three subtests (btf_module_names_load, btf_module_names_skip, and
btf_module_names_empty) leave the input-validation paths from the
preceding commit completely unexercised.

That commit adds checks in bpf_object_init_btf_module_names() that are
documented in libbpf.h as part of the API contract, particularly:
"The list must not contain duplicate entries; otherwise -EINVAL is
returned."

Uncovered paths in bpf_object_init_btf_module_names():
- Empty or NULL entry detection (if (!names[i] || !names[i][0]))
- Duplicate name detection (the for loop with strcmp checking all
  previous entries)
- Error cleanup path (err_out:) that calls
  bpf_object_free_btf_module_names() after partial strdup() walk

Would it make sense to add subtests covering these validation paths? A
single extra __open_opts() call checked with ASSERT_ERR_PTR() or
ASSERT_NULL() would exercise each. The duplicate-detection loop is
non-trivial enough to deserve one.

Also, nothing in the series asserts the default btf_module_names == NULL
case (load everything, unchanged behaviour), so a regression that makes
the option mandatory would pass undetected.

> +out:
> +	btf_module_names__destroy(skel);
> +}
> +
> +/*
> + * Verify that an unrequested module BTF is skipped. The BPF program
> + * requires the BTF of bpf_testmod, but bpf_testmod is not specified in
> + * btf_module_names, so its BTF is skipped and the BPF program fails to load.
> + */
> +static void btf_module_names_skip(void)
> +{
> +	struct btf_module_names *skel = NULL;
> +	int ret;
> +	static const char *mod_names[] = { "module_nonexist" };
> +
> +	LIBBPF_OPTS(bpf_object_open_opts, opts,
> +		.btf_module_names = mod_names,
> +		.nr_btf_module_names = 1,
> +	);
> +
> +	skel = btf_module_names__open_opts(&opts);
> +	if (!ASSERT_OK_PTR(skel, "btf_module_names__open_opts"))
> +		goto out;
> +
> +	ret = btf_module_names__load(skel);
> +	ASSERT_ERR(ret, "btf_module_names__load");

A subsystem pattern flags this as potentially concerning: both negative
subtests assert only that the load failed, not the specific reason why.
ASSERT_ERR expands to check ___res < 0, so btf_module_names_skip() and
btf_module_names_empty() pass for any negative return from
btf_module_names__load().

The intended error is specific: with bpf_testmod's BTF filtered out,
find_kernel_btf_id() walks obj->btf_modules and falls through to return
-ESRCH, which bpf_object_load() propagates to __load(). Asserting that
value (for example, ASSERT_EQ(ret, -ESRCH, ...)) would pin the
behaviour.

As written, several unrelated conditions make both subtests report
success while proving nothing about the new filter:

- CONFIG_DEBUG_INFO_BTF_MODULES=n: bpf_testmod loads (so env.has_testmod
  is true and the test is not skipped) but has no BTF at all.
  Attach-target resolution then fails regardless of btf_module_names, so
  both negative subtests pass vacuously.

- Unprivileged/EPERM: load_module_btfs() returns 0 early without loading
  any module BTF (pr_debug skipping message), again yielding -ESRCH from
  an unrelated cause.

- Any future validation of the new opts that rejects the list at load
  time rather than at open time.

Would pinning the expected -ESRCH value make these subtests more robust?

> +
> +out:
> +	btf_module_names__destroy(skel);
> +}

[ ... ]


---
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/32730521630