Re: [PATCH bpf-next v3 6/6] selftests/bpf: Cover BPF program manual loading

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/bpf: bpf_link leak on assertion error paths in dynamicload
--

commit 0fd345ea8ecacebc3bb75a7415c7f51a2637fd25
Author: Andrey Grodzovsky <andrey.grodzovsky@crowdstrike.com>

selftests/bpf: Cover BPF program manual loading

Add dynamicload test covering the manual load/attach/detach/reload
cycle, declarative MANUAL via SEC("!...") and its imperative
override, bpf_object__prepare() alone being sufficient for manual
load, and a deferred load of a module BTF attach target.

> diff --git a/tools/testing/selftests/bpf/prog_tests/dynamicload.c b/tools/testing/selftests/bpf/prog_tests/dynamicload.c
> new file mode 100644
> index 0000000000000..979bdc14f4ffd
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/dynamicload.c

[ ... ]

> +static void dynamicload_verify_prepare_only(void)
> +{
> +	struct test_dynamicload *skel;
> +	struct bpf_link *link;
> +	int err;

[ ... ]

> +	link = bpf_program__attach(skel->progs.prog4);
> +	if (!ASSERT_OK_PTR(link, "attach_after_prepare"))
> +		goto cleanup;
> +
> +	usleep(1);
> +
> +	if (!ASSERT_TRUE(skel->bss->prog4_called, "prog4_called_after_prepare"))
> +		goto cleanup;

[Severity: Medium]
If this assertion fails, we jump to cleanup which calls
test_dynamicload__destroy(skel). Because test_dynamicload__destroy()
doesn't detach manually loaded programs, does this leak the bpf_link
and its file descriptor by bypassing bpf_link__destroy(link)?

> +
> +	err = bpf_link__destroy(link);
> +	if (!ASSERT_OK(err, "link_destroy_after_prepare"))
> +		goto cleanup;
> +
> +	err = bpf_program__unload_manually(skel->progs.prog4);
> +	ASSERT_OK(err, "unload_manually_after_prepare");
> +
> +cleanup:
> +	test_dynamicload__destroy(skel);
> +}

[ ... ]

> +void test_dynamicload(void)
> +{
> +	struct bpf_link *link;
> +	struct test_dynamicload *skel;
> +	int err;

[ ... ]

> +	/* load prog3 */
> +	err = bpf_program__load_manually(skel->progs.prog3);
> +	if (!ASSERT_OK(err, "load_manually"))
> +		goto cleanup;
> +
> +	/* attach prog3 */
> +	link = bpf_program__attach(skel->progs.prog3);
> +	if (!ASSERT_OK_PTR(link, "attach"))
> +		goto cleanup;
> +
> +	usleep(1);
> +
> +	if (!ASSERT_TRUE(skel->bss->prog3_called, "prog3_called"))
> +		goto cleanup;

[Severity: Medium]
Similar to the issue above, if this assertion fails, we jump directly to
cleanup and bypass bpf_link__destroy(link). Could this leak the link resource
for prog3?

> +
> +	/* detach prog3 as test_dynamicload__destroy doesn't detach manually loaded programs */
> +	err = bpf_link__destroy(link);
> +	if (!ASSERT_OK(err, "link_destroy"))
> +		goto cleanup;

[ ... ]

> +	/* reattach prog3 */
> +	link = bpf_program__attach(skel->progs.prog3);
> +	if (!ASSERT_OK_PTR(link, "reattach"))
> +		goto cleanup;
> +
> +	usleep(1);
> +
> +	if (!ASSERT_TRUE(skel->bss->prog3_called, "prog3_called_reattach"))
> +		goto cleanup;

[Severity: Medium]
Does this error path also leak the reattached bpf_link when jumping
to cleanup?

> +
> +	/* detach prog3 as test_dynamicload__destroy doesn't detach manually loaded programs */
> +	err = bpf_link__destroy(link);
> +	if (!ASSERT_OK(err, "link_destroy_reattach"))
> +		goto cleanup;

[ ... ]

> +	/*
> +	 * run prog4 (declaratively marked) through the same manual
> +	 * load/attach/trigger/detach/unload cycle as prog3
> +	 */
> +	err = bpf_program__load_manually(skel->progs.prog4);
> +	if (!ASSERT_OK(err, "prog4_load_manually"))
> +		goto cleanup;
> +
> +	link = bpf_program__attach(skel->progs.prog4);
> +	if (!ASSERT_OK_PTR(link, "prog4_attach"))
> +		goto cleanup;
> +
> +	usleep(1);
> +
> +	if (!ASSERT_TRUE(skel->bss->prog4_called, "prog4_called"))
> +		goto cleanup;

[Severity: Medium]
Can jumping to cleanup here leave the prog4 bpf_link leaked as well?

> +
> +	err = bpf_link__destroy(link);
> +	if (!ASSERT_OK(err, "prog4_link_destroy"))
> +		goto cleanup;
> +
> +	err = bpf_program__unload_manually(skel->progs.prog4);
> +	ASSERT_OK(err, "prog4_unload_manually");
> +
> +	test_dynamicload__destroy(skel);
> +
> +	/* separate scenario: imperative override of the declarative marker */
> +	dynamicload_verify_override();
> +
> +	/* separate scenario: prepare()-only is sufficient for manual load */
> +	dynamicload_verify_prepare_only();
> +
> +	/* separate scenario: deferred manual load with a module BTF attach target */
> +	dynamicload_verify_module_btf();
> +	return;
> +
> +cleanup:
> +	test_dynamicload__destroy(skel);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917203040.3150212-1-andrey.grodzovsky@crowdstrike.com?part=6