Re: [PATCH bpf-next v2 5/5] selftests/bpf: Cover BPF program dynamic loading

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Calling `bpf_program__load_dynamically()` multiple times on the same program overwrites `prog->fd` and leaks the previously loaded BPF program's file descriptor.
- [Medium] bpf_link leaked on error paths leaves system-wide raw tracepoint active.
- [Low] Multi-line comments do not follow the mandatory BPF subsystem style.
- [Low] `dynamicload_verify_override()` is skipped entirely if the preceding `test_dynamicload` logic encounters a failure.
- [Low] Missing early return check for state-altering assertion allows test execution to continue in an invalid state.
--

commit eb781cf99ff72cd5528638c785d25a5ff470c1c9
Author: Slava Imameev <slava.imameev@crowdstrike.com>

selftests/bpf: Cover BPF program dynamic loading

This commit adds load_type and dynamicload tests to verify the per-program load
type enum (disabled/auto/dynamic) and its transitions. It also tests the full
lifecycle of dynamic load/attach/reattach/unload.

> 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..799c43034ff92
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/dynamicload.c
> @@ -0,0 +1,222 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <test_progs.h>
> +#include <time.h>
> +#include "test_dynamicload.skel.h"
> +
> +/* prog4 is tagged __load_dynamic in the source instead of being set
> + * imperatively; verify that an explicit bpf_program__set_load_type() call
> + * before load overrides the tag's declarative default.
> + */

[Severity: Low]
This isn't a bug, but this multi-line comment does not follow the mandatory BPF
subsystem style, which requires the opening '/*' to be on its own line.

> +static void dynamicload_verify_override(void)
> +{
[ ... ]
> +void test_dynamicload(void)
> +{
> +	struct bpf_link *link;
> +	struct test_dynamicload *skel;
> +	int err;
> +
> +	skel = test_dynamicload__open();
> +	if (!ASSERT_OK_PTR(skel, "skel_open"))
> +		return;
> +
> +	/* the __load_dynamic tag alone, with no imperative call, must set
> +	 * prog4's load type before it is ever touched below
> +	 */

[Severity: Low]
This isn't a bug, but this multi-line comment also does not follow the
mandatory BPF subsystem style.

[ ... ]
> +	/* 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]
Does this code leak the manually managed link object and leave the system-wide
raw tracepoint active if this assertion fails? The cleanup path only destroys
the skeleton, which doesn't track this standalone bpf_link object.

[ ... ]
> +	err = bpf_program__unload_dynamically(skel->progs.prog4);
> +	ASSERT_OK(err, "prog4_unload_dynamically");
> +
> +	test_dynamicload__destroy(skel);
> +
> +	/* separate scenario: imperative override of the declarative tag */
> +	dynamicload_verify_override();
> +	return;
> +
> +cleanup:
> +	test_dynamicload__destroy(skel);
> +}

[Severity: Low]
Is it intentional that dynamicload_verify_override() is skipped entirely if the
preceding test_dynamicload() logic encounters a failure and jumps to cleanup?
This could mask test coverage since it won't be reported as executed or skipped.

> diff --git a/tools/testing/selftests/bpf/prog_tests/load_type.c b/tools/testing/selftests/bpf/prog_tests/load_type.c
> new file mode 100644
> index 0000000000000..fc91f6666c506
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/load_type.c
> @@ -0,0 +1,167 @@
[ ... ]
> +	/* bpf_program__set_autoload() is a thin forwarder to set_load_type(),
> +	 * restricted to AUTO/DISABLED to preserve its original bool on/off
> +	 * meaning; it does change the load type of a program that isn't
> +	 * currently BPF_PROG_LOAD_TYPE_AUTO.
> +	 */

[Severity: Low]
This isn't a bug, but this multi-line comment does not follow the mandatory BPF
subsystem style, which requires the opening '/*' to be on its own line.

[ ... ]
> +	/* loaded program type cannot be changed */
> +	err = bpf_program__set_load_type(skel->progs.prog3, BPF_PROG_LOAD_TYPE_DISABLED);
> +	ASSERT_ERR(err, "set_load_type_after_load");
> +
> +	err = test_load_type__attach(skel);

[Severity: Low]
Should there be an early return check here? Without it, if the underlying API
mistakenly succeeds, the program's load state is incorrectly modified to
DISABLED and the test continues running, which could cause cascading and
confusing failures.

[ ... ]
> +	err = bpf_program__load_dynamically(skel->progs.prog3, 0);
> +	if (!ASSERT_OK(err, "load_dynamically_1"))
> +		goto cleanup;
> +
> +	err = bpf_program__load_dynamically(skel->progs.prog3, 0);
> +	if (!ASSERT_OK(err, "load_dynamically_2"))
> +		goto cleanup;

[Severity: High]
Does calling bpf_program__load_dynamically() multiple times on the same program
overwrite prog->fd and leak the previously loaded BPF program's file descriptor?
The test seems to actively rely on this behavior by loading the program twice
without an intervening unload.

> +
> +	/* 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_again"))
> +		goto cleanup;

[Severity: Medium]
Will this bypass bpf_link__destroy(link) and leak the system-wide tracepoint
attachment if the assertion fails and jumps to cleanup?

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