Re: [RFC PATCH 3/5] perf/core: add KUnit tests for AUX kernel-consumer API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] KUnit assertions (`KUNIT_ASSERT_*`) and `kunit_skip()` abort tests immediately, leaking resources and causing stack Use-After-Free.
- [High] Race between `atomic_dec_and_test()` and lockless `wait_event()` causes a stack Use-After-Free in `test_concurrent_release()`.
- [High] Test cases use non-fatal `KUNIT_EXPECT_*` macros for critical setup steps, resulting in uninitialized stack usage and NULL pointer dereferences.
- [High] `test_concurrent_release` triggers a `refcount_t` underflow kernel WARNING via concurrent `perf_event_release_aux()` calls.
- [High] Error path in `test_concurrent_release` calls `kthread_stop()` on natively exiting threads, leading to Use-After-Free.
- [Medium] Dead code in `test_buffer_full_stops_producer` masks actual API behavior and fails to enforce expected outcomes.
--

--- Patch [3]: [RFC PATCH 3/5] perf/core: add KUnit tests for AUX kernel-consumer API ---
commit 7ac72ec86c4a7abdecf89cc4cddc373879a7f3b2
Author: Lian Wang <lianux.mm@gmail.com>

perf/core: add KUnit tests for AUX kernel-consumer API

Adds a KUnit test suite (perf_aux_kernel) for the perf_event_setup_aux() and
related functions. Registers a self-contained dummy AUX PMU to run tests
deterministically without hardware. Includes 39 test cases covering lifecycle,
writers, ring buffer accessors, and concurrency.

> diff --git a/kernel/events/aux_kernel_test.c b/kernel/events/aux_kernel_test.c
> new file mode 100644
> index 0000000000000..0b5f787c59799
> --- /dev/null
> +++ b/kernel/events/aux_kernel_test.c
[ ... ]
> +static void test_copy_full_window(struct kunit *test)
> +{
> +	struct perf_event *event = aux_test_create_event();
[ ... ]
> +	ret = perf_event_setup_aux(event, 1, 0);
> +	KUNIT_EXPECT_EQ(test, 0, ret);
> +
> +	head = aux_test_produce(event, buf_size / 2);
> +	KUNIT_EXPECT_GT(test, head, 0);
> +
> +	buf = kunit_kmalloc(test, head, GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, buf);

[Severity: High]
Does this assertion leak the perf_event structure?

KUnit assertions like KUNIT_ASSERT_NOT_ERR_OR_NULL abort the test immediately
by throwing a KUnit exception. If this allocation fails after
aux_test_create_event() succeeds, the test will abort without calling
perf_event_release_kernel(), causing a permanent resource leak.

[ ... ]
> +static void test_buffer_full_stops_producer(struct kunit *test)
> +{
[ ... ]
> +	head = aux_test_produce(event, buf_size);
> +	KUNIT_EXPECT_GT(test, head, 0);
> +
> +	addr = perf_aux_output_begin(&handle, event);
> +	if (addr) {
> +		KUNIT_EXPECT_EQ(test, 0, handle.size);
> +		perf_aux_output_end(&handle, 0);
> +	}

[Severity: Medium]
Does this block fail to verify the expected behavior?

When the buffer is full, perf_aux_output_begin() correctly returns NULL.
Because addr is NULL, this conditional block is skipped entirely, turning the
core validation into dead code. This allows the test to silently pass even if
perf_aux_output_begin() incorrectly returns a non-NULL pointer with an
arbitrary size.

Would it be better to explicitly assert KUNIT_EXPECT_NULL(test, addr)?

[ ... ]
> +static void test_user_kernel_coexistence(struct kunit *test)
> +{
[ ... ]
> +	/* Event B: start with kernel setup, then simulate userspace */
> +	ret = perf_event_setup_aux(ev_b, 1, 0);
> +	KUNIT_EXPECT_EQ(test, 0, ret);
> +
> +	/* Simulate userspace ownership: swap refcounts on B's rb.
> +	 * perf_mmap() would set aux_mmap_count=1 for a userspace event.
> +	 */
> +	refcount_set(&ev_b->rb->aux_mmap_count, 1);

[Severity: High]
Can this result in a NULL pointer dereference?

The KUNIT_EXPECT_EQ macro logs failures but allows execution to continue.
If perf_event_setup_aux() fails, ev_b->rb will be NULL. The test then proceeds
to evaluate refcount_set(&ev_b->rb->aux_mmap_count, 1), which dereferences it.

Should KUNIT_ASSERT_EQ be used for critical setup steps?

[ ... ]
> +	/* Produce on A — B's head stays 0 */
> +	addr = perf_aux_output_begin(&handle_a, ev_a);
> +	KUNIT_EXPECT_NOT_NULL(test, addr);
> +	perf_aux_output_end(&handle_a, 512);

[Severity: High]
Can this sequence cause a kernel panic?

Similar to the previous setup step, if perf_aux_output_begin() fails and
returns NULL, handle_a remains uninitialized since KUNIT_EXPECT_NOT_NULL does
not halt the test.

Continuing past the failure passes this uninitialized handle to
perf_aux_output_end(), which will dereference garbage data from handle_a.rb.

[ ... ]
> +struct concurrent_release_ctx {
> +	struct perf_event *event;
> +	atomic_t count;
> +	wait_queue_head_t wq;
> +};
> +
> +static int concurrent_release_thread(void *data)
> +{
> +	struct concurrent_release_ctx *ctx = data;
> +
> +	perf_event_release_aux(ctx->event);
> +
> +	if (atomic_dec_and_test(&ctx->count))
> +		wake_up(&ctx->wq);
> +	return 0;
> +}

[Severity: High]
Can decoupling the atomic decrement and the wake_up() call create a stack
use-after-free window?

The main thread waits for worker threads using wait_event(), which evaluates
its condition locklessly. If the last worker thread executes
atomic_dec_and_test(&ctx->count), setting it to 0, the main thread can observe
the 0, break out of wait_event(), and return from the function before the
worker executes wake_up(&ctx->wq).

Returning from test_concurrent_release() destroys the ctx struct on the stack,
meaning the worker thread will then call wake_up() on a destroyed stack frame.

Could this be avoided by using the struct completion API, which is explicitly
designed to prevent this race for on-stack waiters?

[ ... ]
> +static void test_concurrent_release(struct kunit *test)
> +{
> +	struct perf_event *event = aux_test_create_event();
> +	struct concurrent_release_ctx ctx;
> +	struct task_struct *threads[2];
> +	int ret, i;
[ ... ]
> +	for (i = 0; i < ARRAY_SIZE(threads); i++) {
> +		threads[i] = kthread_run(concurrent_release_thread, &ctx,
> +					 "aux_crel_%d", i);

[Severity: High]
Does intentionally launching concurrent releases here trigger a refcount_t
underflow warning?

In perf_event_release_aux() in core.c, the check
!refcount_read(&rb->aux_kernel_count) is performed locklessly. Both worker
threads can read aux_kernel_count as 1 and proceed to call
refcount_dec_and_mutex_lock().

The first thread successfully decrements the count from 1 to 0. The second
thread subsequently calls refcount_dec_and_mutex_lock() on a count of 0,
which triggers a REFCOUNT_WARN("underflow; use-after-free.") inside
__refcount_dec_and_test().

Is it intended for a KUnit test to trigger kernel warnings on a successful
path?

> +		if (IS_ERR_OR_NULL(threads[i])) {
> +			kunit_skip(test, "failed to create kthread");

[Severity: High]
Can kunit_skip() cause a stack use-after-free if a thread is already running?

kunit_skip() terminates the test immediately by throwing a KUnit exception,
bypassing any subsequent cleanup code. This means the test function returns
instantly, destroying the ctx struct allocated on the main thread's stack.

If an earlier thread was successfully spawned and is still running, it will
access ctx->event and ctx->count on the destroyed stack frame.

> +			while (--i >= 0)
> +				kthread_stop(threads[i]);

[Severity: High]
Does kthread_stop() cause a use-after-free on natively exiting threads?

The concurrent_release_thread function does not check kthread_should_stop();
it simply executes and naturally returns 0. When a thread naturally exits, its
task_struct is reaped.

Calling kthread_stop() here on a potentially reaped task_struct will access
freed memory inside get_task_struct(). While currently masked as dead code due
to kunit_skip() aborting execution above, this cleanup logic appears unsafe.

> +			perf_event_release_aux(event);
> +			perf_event_release_kernel(event);
> +			return;
> +		}
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814144927.489172-1-kunwu.chan@linux.dev?part=3