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

[Fuyu Zhao <zhaofuyu@vivo.com>]

On 8/24/2026 9:31 PM, bot+bpf-ci@kernel.org wrote:
>> 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.
> 

Yes, I may have missed some scenarios. I'll add a simple subtest
using __open_opts() to cover these invalid cases. Thanks.

>> +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?
> 

This is a good point. I'll check the expected -ESRCH error explicitly
in the negative subtests to make the tests 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