Re: [PATCH v2] perf: Fix mmap replacement ring lifetime race

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Cross-mm pinned_vm and locked_vm accounting mismatch when main buffer and AUX buffer are mapped by different processes.
- [High] Error path in perf_mmap leaks ring buffer mmap counts and unaccounts wrong memory size for AUX mmaps on redirected events.
--

--- Patch [1]: [PATCH v2] perf: Fix mmap replacement ring lifetime race ---
commit 454633461f89e2fe1c245d071d92eff5dc95e16e
Author: Kyle Zeng <kylebot@openai.com>
Subject: perf: Fix mmap replacement ring lifetime race

perf_mmap_close() drops the ring-local mmap_count before serializing
with perf_mmap() through event->mmap_mutex. This patch takes
event->mmap_mutex before updating either count, ensuring the transitions
are atomic with respect to perf_mmap().

Link: https://lore.kernel.org/all/20260731120401.558858-1-david.lee@trailofbits.com/

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 51c1200ea3fdd..cce24be2d1f28 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -7053,11 +7053,19 @@ static void perf_mmap_close(struct vm_area_struct *vma)
>  		mutex_unlock(&rb->aux_mutex);
>  	}
>  
> +	/*
> +	 * Serialize both count updates with perf_mmap() so they cannot
> +	 * refer to different ring buffer generations.
> +	 */
> +	mutex_lock(&event->mmap_mutex);
> +
>  	if (refcount_dec_and_test(&rb->mmap_count))
>  		detach_rest = true;
>  
> -	if (!refcount_dec_and_mutex_lock(&event->mmap_count, &event->mmap_mutex))
> +	if (!refcount_dec_and_test(&event->mmap_count)) {
> +		mutex_unlock(&event->mmap_mutex);
>  		goto out_put;

[Severity: High]
This is a pre-existing issue, but does this mmap_count logic lead to a
cross-mm pinned_vm and locked_vm accounting mismatch when the main buffer
and AUX buffer are mapped by different processes?

If Process A maps the main ring buffer of an event, charging its VMA's
pinned_vm, and the event fd is shared with Process B which maps the AUX
buffer, rb->mmap_count tracks both mappings globally.

If Process A unmaps the main buffer first, the following evaluates to false:

	if (refcount_dec_and_test(&rb->mmap_count))
		detach_rest = true;

The mmap_count decrements but does not reach zero, skipping the later call
to perf_mmap_unaccount().

When Process B later unmaps the AUX buffer, rb->mmap_count reaches zero
and perf_mmap_unaccount() is called with Process B's VMA near the end of
perf_mmap_close():

	/*
	 * Aside from that, this buffer is 'fully' detached and unmapped,
	 * undo the VM accounting.
	 */
	perf_mmap_unaccount(vma, rb);

Does this unaccount the main buffer's size from Process B's limits,
underflowing Process B's pinned_vm while permanently leaking the limits for
Process A?

> +	}
>  
>  	ring_buffer_attach(event, NULL);
>  	mutex_unlock(&event->mmap_mutex);

[Severity: High]
This is also a pre-existing issue, but looking at the related error path in
perf_mmap(), could it leak ring buffer mmap counts and unaccount the wrong
memory size for AUX mmaps on redirected events?

When mmap() is called for the AUX buffer of a redirected event,
perf_mmap_aux() increments rb->mmap_count, rb->aux_mmap_count, and sets
event->mmap_count to 1.

If map_range() fails due to an error, the cleanup path in perf_mmap()
evaluates this:

		if (refcount_read(&event->mmap_count) == 1) {
			/*
			 * Minimal perf_mmap_close(); there can't be AUX or
			 * other events on account of this being the first.
			 */
			mapped = get_mapped(event, event_unmapped);
			if (mapped)
				mapped(event, vma->vm_mm);
			perf_mmap_unaccount(vma, event->rb);
			ring_buffer_attach(event, NULL);
			refcount_set(&event->mmap_count, 0);
			return ret;
		}

Because event->mmap_count is 1, it assumes this is an exclusive main buffer
mmap. It then calls perf_mmap_unaccount(), which unaccounts the main
buffer's size instead of the AUX buffer's size, potentially underflowing the
caller's pinned_vm.

Additionally, it skips decrementing rb->mmap_count and rb->aux_mmap_count
while detaching the event from the ring buffer. Could this cause a
persistent memory leak of ring buffer pages and break event redirection?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804060931.711308-1-david.lee@trailofbits.com?part=1