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

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

[David Lee]

From: Kyle Zeng <kylebot@openai.com>

perf_mmap_close() drops the ring-local mmap_count before serializing
with perf_mmap() through event->mmap_mutex.  When the last mapping is
being closed, a concurrent mmap can therefore observe a nonzero
event->mmap_count and a zero rb->mmap_count.

In that case perf_mmap_rb() detaches the old ring, installs a replacement,
and resets event->mmap_count to one.  The old close then consumes that
replacement count and detaches the new ring.  Its pages can consequently
be freed while the replacement VMA still maps their PFNs.

Take event->mmap_mutex before updating either count.  This makes the
ring-local and event-global count transitions atomic with respect to
perf_mmap(), so a replacement cannot be installed until the old close has
detached its ring.

Fixes: 59741451b49c ("perf: Identify the 0->1 transition for event::mmap_count")
Assisted-by: Codex:gpt-5.6-sol Codex:gpt-5.5-cyber
Signed-off-by: Kyle Zeng <kylebot@openai.com>
Co-developed-by: David Lee <david.lee@trailofbits.com>
Signed-off-by: David Lee <david.lee@trailofbits.com>
---
Changes in v2:
- Restore Kyle Zeng as the patch author and correct the sign-off chain.
- Move the research credit below the commit-message separator.

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

Bug found and triaged by OpenAI Security Research and
validated by Trail of Bits.

Trail of Bits has a reproducer for this bug that triggers a
kernel panic and can share if needed.

 kernel/events/core.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index ba5bd6a78..f93327c76 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;
+	}
 
 	ring_buffer_attach(event, NULL);
 	mutex_unlock(&event->mmap_mutex);
-- 
2.53.0

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

[sashiko-bot]

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