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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Use-After-Free and data races due to statically assigned and uncleared struct_ops pointer.
--

commit 23e6150eb62c7059e7721142219a9a87264e2e46
Author: Tiezhu Yang <yangtiezhu@loongson.cn>

selftests/bpf: Add test for indirect struct_ops trampoline

This commit adds a test case to verify that arguments passed on the stack
are correctly read by the indirect struct_ops trampoline. It ensures the
correctness of stack offsets on architectures like LoongArch and RISC-V.

> diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_trampoline.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_trampoline.c
> new file mode 100644
> index 0000000000000..5a21aedd02fc5
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_trampoline.c
> @@ -0,0 +1,41 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <test_progs.h>
> +#include "struct_ops_trampoline.skel.h"
> +
> +#if defined(__loongarch__) || defined(__riscv)
> +static void run_struct_ops_trampoline(void)
> +{
[ ... ]
> +	bpf_link__destroy(link);
> +cleanup:
> +	struct_ops_trampoline__destroy(skel);

[Severity: High]
Does this call to struct_ops_trampoline__destroy() free the map?

Later in this diff, we see that st_ops_trampoline in bpf_testmod.c is
assigned dynamically allocated memory from this map. When the map is freed
here, st_ops_trampoline appears to become a dangling pointer.

> +}
> +#endif

[ ... ]

> diff --git a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
> index 850cf4f830c48..be96bbc1b7c04 100644
> --- a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
[ ... ]
> @@ -637,6 +640,9 @@ bpf_testmod_test_read(struct file *file, struct kobject *kobj,
>  
>  	bpf_testmod_test_struct_ops3();
>  
> +	if (st_ops_trampoline && st_ops_trampoline->test_trampoline)
> +		st_ops_trampoline->test_trampoline(1, 2, 3, 4, 5, 6, 7, 8, 9999);

[Severity: High]
Since st_ops_trampoline is never cleared after the map is freed, could this
result in a use-after-free?

Any subsequent or parallel test in the suite that triggers
trigger_module_test_read() will invoke bpf_testmod_test_read(), incorrectly
evaluate st_ops_trampoline as valid, and dereference the freed memory.

> +
>  	struct_arg3 = kmalloc((sizeof(struct bpf_testmod_struct_arg_3) +
>  				sizeof(int)), GFP_KERNEL);
[ ... ]
> @@ -1619,6 +1625,10 @@ 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) * 8)
> +		st_ops_trampoline = (struct bpf_testmod_ops *)kdata;

[Severity: High]
Is it safe to unconditionally assign this global pointer to the map's
dynamically allocated memory during initialization without setting up a
cleanup mechanism?

Additionally, this assignment appears to occur without locking at map creation
time, meaning parallel test runs could race to overwrite this pointer.

> +
>  	return 0;
>  }

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