Re: [RFC PATCH 1/5] perf/core: add AUX buffer ownership for kernel events

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Double `ring_buffer_put()` in `perf_event_release_aux()` leads to refcount underflow and use-after-free of the ring buffer.
- [High] Concurrent detach of `event->rb` causes a NULL pointer dereference inside `perf_pmu_output_stop()`.
--

--- Patch [1]: [RFC PATCH 1/5] perf/core: add AUX buffer ownership for kernel events ---
commit 34b60be57b99a06be2e537e647853e4c5c20c8c2
Author: Kunwu Chan <[email protected]>

perf/core: add AUX buffer ownership for kernel events

This commit adds an in-kernel AUX owner reference and setup/release helpers
for kernel-created perf events. It introduces perf_event_setup_aux() and
perf_event_release_aux() to handle AUX buffer lifecycle without requiring a
userspace mmap.

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c

[ ... ]

> +void perf_event_release_aux(struct perf_event *event)
> +{
> +	struct perf_buffer *rb;
> +
> +	if (!is_kernel_event(event) || event->parent)
> +		return;
> +
> +	rb = ring_buffer_get(event);
> +	if (!rb)
> +		return;
> +
> +	/* Do not detach a ring buffer that this API does not own. */
> +	if (!rb_has_aux(rb) || !refcount_read(&rb->aux_kernel_count))
> +		goto out_put;
> +
> +	if (refcount_dec_and_mutex_lock(&rb->aux_kernel_count, &rb->aux_mutex)) {
> +		/*
> +		 * Stop all AUX events writing to this buffer so the pages
> +		 * can be freed; after aux_kernel_count drops to zero they
> +		 * won't start any more (see perf_aux_output_begin()).
> +		 */
> +		perf_pmu_output_stop(event);

[Severity: High]
Is there a race condition here if multiple threads call this concurrently?

If Thread 1 enters this block and calls perf_pmu_output_stop(), it accesses
event->rb directly:

static void perf_pmu_output_stop(struct perf_event *event)
{
        ...
        list_for_each_entry_rcu(iter, &event->rb->event_list, rb_entry) {

Meanwhile, Thread 2 could fail the aux_kernel_count decrement, skip the
block, and immediately detach the buffer:

        if (event->rb == rb) {
                ring_buffer_attach(event, NULL);

Since ring_buffer_attach(event, NULL) sets event->rb to NULL, could Thread 1
dereference a NULL pointer inside perf_pmu_output_stop()?

> +
> +		rb_free_aux(rb);
> +		WARN_ON_ONCE(refcount_read(&rb->aux_refcount));
> +		mutex_unlock(&rb->aux_mutex);
> +	}
> +
> +	/*
> +	 * Detach the ring buffer from the event.  This runs even if the
> +	 * refcount_dec_and_mutex_lock above lost the race to another
> +	 * concurrent release caller; the mmap_mutex serialisation and the
> +	 * event->rb == rb check ensure only one caller performs the detach.
> +	 */
> +	mutex_lock(&event->mmap_mutex);
> +	if (event->rb == rb) {
> +		ring_buffer_attach(event, NULL);
> +		ring_buffer_put(rb); /* drop the event->rb reference */

[Severity: High]
Does this result in a double-free?

Looking at ring_buffer_attach(), it already drops the reference to the old
buffer when it is replaced:

static void ring_buffer_attach(struct perf_event *event,
                               struct perf_buffer *rb)
{
        ...
        rcu_assign_pointer(event->rb, rb);

        if (old_rb) {
                ring_buffer_put(old_rb);
        ...
}

If ring_buffer_attach(event, NULL) already calls ring_buffer_put() on the
detached buffer, will calling ring_buffer_put(rb) again immediately after
cause the refcount to underflow and prematurely free the ring buffer?

> +	}
> +	mutex_unlock(&event->mmap_mutex);
> +
> +out_put:
> +	ring_buffer_put(rb); /* the temporary reference from ring_buffer_get() */
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1