Re: [PATCH bpf-next v2] selftests/bpf: Add test for indirect struct_ops trampoline

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The introduction of global state (`st_ops_trampoline` and its mutex) in `bpf_testmod` causes parallel test breakages, permanent DoS, and Use-After-Free (UAF).
- [Medium] Removal of `RUN_TESTS` drops declarative verifier failure message matching, masking verifier bugs.
--

commit 932e6a2ea3847d584c9e377639c762f03a9fe713
Author: Tiezhu Yang <yangtiezhu@loongson.cn>

selftests/bpf: Add test for indirect struct_ops trampoline

Add a test case to verify that arguments passed on the stack are correctly
read by an indirect struct_ops trampoline.

> diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_multi_args.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_multi_args.c
> index 0f321e8898620..7f6f9d61c9e07 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_multi_args.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_multi_args.c
> @@ -3,7 +3,54 @@
>  #include <test_progs.h>
>  #include "struct_ops_multi_args.skel.h"
>  
> +static void test_refcounted_multi(void)
> +{
> +	struct struct_ops_multi_args *skel;
> +	int err;
> +
> +	skel = struct_ops_multi_args__open();
> +	if (!ASSERT_OK_PTR(skel, "struct_ops_multi_args__open"))
> +		return;
> +
> +	err = struct_ops_multi_args__load(skel);
> +	ASSERT_EQ(err, -EINVAL, "struct_ops_multi_args__load");

[Severity: Medium]
Does this replace the test_loader's verifier log parsing with a manual
assertion?

By dropping RUN_TESTS in test_struct_ops_multi_args(), the test now silently
passes on any verifier error (such as invalid BTF or a missing map) returning
-EINVAL rather than validating the exact failure condition. This masks
potential verifier bugs.

[ ... ]

> diff --git a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
> index 2380b6cbdeadc..8f0744e6075b3 100644
> --- a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
[ ... ]
> @@ -1691,6 +1694,18 @@ static int bpf_testmod_ops_init_member(const struct btf_type *t,
>  		((struct bpf_testmod_ops *)kdata)->data = ((struct bpf_testmod_ops *)udata)->data;
>  		return 1;
>  	}
> +
> +	if (member->offset == offsetof(struct bpf_testmod_ops, test_trampoline_stack_args) * 8) {
> +		mutex_lock(&st_ops_trampoline_mutex);
> +		if (st_ops_trampoline) {
> +			pr_err("st_ops_trampoline has already been registered\n");
> +			mutex_unlock(&st_ops_trampoline_mutex);
> +			return -EEXIST;
> +		}
> +		st_ops_trampoline = (struct bpf_testmod_ops *)kdata;

[Severity: High]
Can this cause a permanent denial of service in the test module?

This assigns the global pointer st_ops_trampoline to map-owned memory (kdata)
in bpf_testmod_ops_init_member() without establishing a teardown hook to clear
it if map registration fails after this point. If a failure occurs, the
pointer remains dangling, causing permanent -EEXIST failures for all
subsequent tests.

Additionally, this logic actively rejects concurrent map updates across all
parallel test executions.

> +		mutex_unlock(&st_ops_trampoline_mutex);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -1720,11 +1735,20 @@ static int bpf_dummy_reg(void *kdata, struct bpf_link *link)
>  	if (ops->test_2)
>  		ops->test_2(4, ops->data);
>  
> +	mutex_lock(&st_ops_trampoline_mutex);
> +	if (st_ops_trampoline && st_ops_trampoline->test_trampoline_stack_args)
> +		st_ops_trampoline->test_trampoline_stack_args(1, 2, 3, 4, 5, 6, 7, 8, 9999);

[Severity: High]
Could this lead to a use-after-free?

This dereferences the global st_ops_trampoline which may point to freed memory
if a previous map update failed or if a map was concurrently detached. Since
the local ops pointer (kdata) is already passed directly to bpf_dummy_reg(),
is there any reason not to use ops directly here instead of relying on the
global state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831085705.13747-1-yangtiezhu@loongson.cn?part=1