Re: [PATCH v4 2/2] selftests/bpf: Add tests for wakeup_sources kfuncs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] BPF verifier rejection due to direct memory access on scalar register
- [High] Incorrect expected failure messages will cause negative tests to fail
- [Medium] Early return upon skeleton load failure skips the negative test suite
- [Medium] Architecture portability issue due to layout mismatch in shared structure
- [Medium] Uninitialized memory leak in ring buffer
--

commit 4b176327f91f44d20f491d6779d845f94357d717
Author: Samuel Wu <wusamuel@google.com>

selftests/bpf: Add tests for wakeup_sources kfuncs

This commit introduces a set of BPF selftests to verify the safety and
functionality of wakeup_source kfuncs. It includes a functional test iterating
over the global wakeup_sources list and a negative test suite for verifier
checks.

> diff --git a/tools/testing/selftests/bpf/prog_tests/wakeup_source.c b/tools/testing/selftests/bpf/prog_tests/wakeup_source.c
> index 0000000000000..ebfdc03271b9a 100644
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/wakeup_source.c
[ ... ]
> @@ -73,46 +73,46 @@ void test_wakeup_source(void)
>  		return;
>  	}
>  
>  	if (test__start_subtest("iterate_and_verify_times")) {
>  		struct test_wakeup_source *skel;
>  		struct ring_buffer *rb = NULL;
>  		struct rb_ctx rb_ctx = {
>  			.name = "bpf_selftest_ws_times",
>  			.found = false,
>  		};
>  		int err;
>  
>  		skel = test_wakeup_source__open_and_load();
>  		if (!ASSERT_OK_PTR(skel, "skel_open_and_load"))
>  			return;

If test_wakeup_source__open_and_load() fails, the code executes an early
return. Does this silently skip running the negative test suite via
RUN_TESTS(wakeup_source_fail) at the end of the function?

[ ... ]
> diff --git a/tools/testing/selftests/bpf/progs/test_wakeup_source.c b/tools/testing/selftests/bpf/progs/test_wakeup_source.c
> index 0000000000000..fd2fb6aebd82b 100644
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_wakeup_source.c
[ ... ]
> @@ -32,61 +32,61 @@ int iterate_wakeupsources(void *ctx)
>  
>  	bpf_for(i, 0, MAX_LOOP_ITER) {
>  		if (bpf_core_read(&pos, sizeof(pos), &pos->next) || !pos || pos == head)
>  			break;
>  
>  		struct wakeup_event_t *e = bpf_ringbuf_reserve(&rb, sizeof(*e), 0);
>  
>  		if (!e)
>  			break;
>  
>  		struct wakeup_source *ws = bpf_core_cast(
>  				(void *)pos - bpf_core_field_offset(struct wakeup_source, entry),
>  				struct wakeup_source);
>  		s64 active_time = 0;
>  		bool active = BPF_CORE_READ_BITFIELD(ws, active);
>  		bool autosleep_enable = BPF_CORE_READ_BITFIELD(ws, autosleep_enabled);
>  		s64 last_time = ws->last_time;

Since pos is read from memory via bpf_core_read(), the verifier tracks it as
a SCALAR_VALUE. Because ws is derived from pos via pointer arithmetic,
will the BPF verifier reject direct memory dereferences like ws->last_time
with an "invalid mem access 'scalar'" error?

Should these accesses be wrapped in BPF_CORE_READ() instead?

[ ... ]
> @@ -76,17 +76,17 @@ int iterate_wakeupsources(void *ctx)
>  		e->last_time_ns = last_time;
>  		e->max_time_ns = max_time;
>  		e->prevent_sleep_time_ns = prevent_sleep_time;
>  		e->total_time_ns = total_time;
>  		e->wakeup_count = ws->wakeup_count;
>  
>  		if (bpf_probe_read_kernel_str(
>  				e->name, WAKEUP_NAME_LEN, ws->name) < 0)
>  			e->name[0] = '\0';
>  
>  		bpf_ringbuf_submit(e, 0);

bpf_ringbuf_reserve() does not zero-initialize the memory, and
bpf_probe_read_kernel_str() does not pad the remainder of the buffer with
zeroes if the string is shorter than WAKEUP_NAME_LEN.

Could this expose uninitialized kernel memory from previous ring buffer uses
to user-space through the remainder of e->name?

[ ... ]
> diff --git a/tools/testing/selftests/bpf/progs/wakeup_source.h b/tools/testing/selftests/bpf/progs/wakeup_source.h
> index 0000000000000..cd74de92c82f8 100644
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/wakeup_source.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright 2026 Google LLC */
> +
> +#ifndef __WAKEUP_SOURCE_H__
> +#define __WAKEUP_SOURCE_H__
> +
> +#define WAKEUP_NAME_LEN 128
> +
> +struct wakeup_event_t {
> +	unsigned long active_count;
> +	long long active_time_ns;

Because this structure is shared between BPF and user-space, could using
architecture-dependent types like unsigned long cause a size and layout
mismatch? 

BPF programs are compiled for a 64-bit target where unsigned long is 8 bytes,
but the user-space selftest may be compiled on a 32-bit host (like ARM32) where
it is 4 bytes.

Should explicitly sized types like __u64 be used for shared structures?

[ ... ]
> diff --git a/tools/testing/selftests/bpf/progs/wakeup_source_fail.c b/tools/testing/selftests/bpf/progs/wakeup_source_fail.c
> index 0000000000000..b8bbb61d4d4e0 100644
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/wakeup_source_fail.c
[ ... ]
> @@ -44,19 +44,19 @@ int wakeup_source_access_lock_fields(void *ctx)
>  	val = *(int *)lock;
>  
>  	bpf_wakeup_sources_read_unlock(lock);
>  	return val;
>  }
>  
>  SEC("syscall")
>  __failure __msg("type=scalar expected=fp")
>  int wakeup_source_unlock_no_lock(void *ctx)

Is this the correct expected failure message for a kfunc argument?

The check_kfunc_args() function typically emits messages like "arg#0 pointer
type mismatch" when an incorrect type is passed, whereas "expected=fp" is
usually emitted by check_func_arg() for standard helpers.

Will this test fail because the verifier output doesn't match the expectation?

>  {
>  	struct bpf_ws_lock *lock = (void *)0x1;
>  
>  	bpf_wakeup_sources_read_unlock(lock);
>  
>  	return 0;
>  }
[ ... ]
> @@ -60,17 +60,17 @@ int wakeup_source_unlock_null(void *ctx)
>  	bpf_wakeup_sources_read_unlock(NULL);
>  
>  	return 0;
>  }
>  
>  SEC("syscall")
>  __failure __msg("R0 invalid mem access 'scalar'")
>  int wakeup_source_unsafe_dereference(void *ctx)
>  {
>  	struct list_head *head = bpf_wakeup_sources_get_head();
>  
>  	if (head->next)

Since bpf_wakeup_sources_get_head() returns a BTF pointer, the verifier tracks
it as PTR_TO_BTF_ID.

Because the verifier allows direct dereferences of trusted pointers (resulting
in PROBE_MEM), will this program successfully load instead of failing with the
"invalid mem access 'scalar'" error?

>  		return 1;
>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511174559.659782-1-wusamuel@google.com?part=2