Re: [PATCH bpf-next v10 6/6] selftests/bpf: Add tests for sleepable tracepoint programs

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [Medium] The test uses libc's `struct timespec` and `__NR_nanosleep`, causing out-of-bounds reads in the BPF program and test failures on 32-bit architectures.
- [Medium] Resource leak in `test_attach_reject()` when program attachment unexpectedly succeeds.
--

commit 464b100e0185655df5851c1f5731b7cd7fb84bb1
Author: Mykyta Yatsenko <yatsenko@meta.com>

selftests/bpf: Add tests for sleepable tracepoint programs

This commit adds tests for sleepable tracepoint programs covering various
attachment types. It verifies functionality using bpf_copy_from_user()
on nanosleep and checks attachment rejection for non-faultable tracepoints.

> diff --git a/tools/testing/selftests/bpf/prog_tests/sleepable_tracepoints.c b/tools/testing/selftests/bpf/prog_tests/sleepable_tracepoints.c
> new file mode 100644
> index 0000000000000..bdc7fe294e7c9
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/sleepable_tracepoints.c
[ ... ]
> +static void run_test(struct test_sleepable_tracepoints *skel)
> +{
> +	skel->bss->target_pid = getpid();
> +	skel->bss->prog_triggered = 0;
> +	skel->bss->err = 0;
> +	skel->bss->copied_tv_nsec = 0;
> +
> +	syscall(__NR_nanosleep, &(struct timespec){ .tv_nsec = 555 }, NULL);

Could this cause out-of-bounds reads in the BPF program and test failures on
32-bit architectures? On 32-bit systems, __NR_nanosleep invokes the legacy
32-bit syscall which expects an 8-byte struct old_timespec32 argument.

Because the BPF program in test_sleepable_tracepoints.c casts this to
struct __kernel_timespec * and reads from it:

static int copy_nanosleep_arg(struct __kernel_timespec *ts)
{
	long tv_nsec;

	err = bpf_copy_from_user(&tv_nsec, sizeof(tv_nsec), &ts->tv_nsec);
	...

it would read uninitialized stack memory past the end of the provided 8-byte
struct, potentially resulting in garbage data or an -EFAULT.

[ ... ]
> +static void test_attach_reject(struct bpf_program *prog)
> +{
> +	struct bpf_link *link;
> +
> +	link = bpf_program__attach(prog);
> +	ASSERT_ERR_PTR(link, "attach_should_fail");
> +}

Does this leak the bpf_link if the attachment unexpectedly succeeds?

If bpf_program__attach() succeeds when it shouldn't, ASSERT_ERR_PTR() will mark
the test as failed, but execution will continue. Since test_progs is a long-lived
process, the unreleased file descriptor would keep the sleepable BPF program
attached to global tracepoints (like sched_switch) for the remainder of the
test suite execution, potentially causing cascading failures or side effects.

Should this link be destroyed if ASSERT_ERR_PTR() fails?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260415-sleepable_tracepoints-v10-0-161f40b33dd7@meta.com?part=6