Re: [PATCH] perf: Fix mmap_count accounting on the perf_mmap_close() race path

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Incrementing event->mmap_count on the raced path causes the concurrent perf_mmap_close() to skip cleanup and unaccounting of the old ring buffer, permanently leaking memory charges and the old ring buffer itself, leading to a Use-After-Free.

Pre-existing issues:
- [High] perf_mmap_aux() lacks protection against mmaping redirected events, causing a refcount saturation WARN on event->mmap_count.
- [High] Mmapping the same perf event multiple times permanently leaks locked_vm and pinned_vm accounting charges.
--

commit 74767b5c0f42ab023d9f220cb4e3a861a837fda1
Author: Aohan Mei <henrymei@tencent.com>

perf: Fix mmap_count accounting on the perf_mmap_close() race path

This patch aims to fix an issue on the perf_mmap_rb() raced path where
a concurrent perf_mmap_close() has dropped the ring buffer's mmap_count to
zero. It restores the previous behavior of incrementing event->mmap_count
instead of setting it to 1, to prevent the concurrent closer from observing
a 1->0 transition and prematurely freeing the new buffer.

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index a6c8e38a31104..7b55156bbc023 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -7281,6 +7281,7 @@ static int perf_mmap_rb(struct vm_area_struct *vma, struct perf_event *event,
>  	long extra = 0, user_extra = nr_pages;
>  	struct perf_buffer *rb;
>  	int rb_flags = 0;
> +	bool raced = false;
>  
>  	nr_pages -= 1;

[Severity: High]
This is a pre-existing issue, but could mmapping the same perf event multiple
times permanently leak locked_vm and pinned_vm accounting charges?

When perf_mmap_rb() handles overlapping alias mappings (just before the raced
path check), it calls perf_mmap_account() for each mapping.

However, during teardown in perf_mmap_close(), perf_mmap_unaccount() is only
called once when the final mapping is unmapped and detach_rest is true. For
all prior munmap() calls, the unaccounting seems to be completely skipped
because detach_rest is false.

Could a user exhaust their RLIMIT_MEMLOCK quota by repeatedly mmapping and
munmapping the same perf event?

[ ... ]

> @@ -7346,7 +7348,21 @@ static int perf_mmap_rb(struct vm_area_struct *vma, struct perf_event *event,
>  	perf_event_update_userpage(event);
>  
>  	perf_mmap_account(vma, user_extra, extra);
> -	refcount_set(&event->mmap_count, 1);
> +
> +	/*
> +	 * On the raced path above, a concurrent perf_mmap_close() can
> +	 * still have a pending decrement of event->mmap_count: it sits
> +	 * blocked inside refcount_dec_and_mutex_lock() on event->mmap_mutex
> +	 * (which we hold) with the count still at 1. Using
> +	 * refcount_set(..., 1) here would make that closer observe a 1->0
> +	 * transition once we drop the mutex, causing it to detach and free
> +	 * the buffer we just installed, while this mmap() still maps it.
> +	 * The count is guaranteed non-zero on the raced path, so increment.
> +	 */
> +	if (raced)
> +		refcount_inc(&event->mmap_count);

[Severity: Critical]
Might incrementing event->mmap_count on the raced path inadvertently break the
teardown logic in the racing closer?

By inflating the count from 1 to 2, the concurrent perf_mmap_close() for the
old ring buffer might see refcount_dec_and_mutex_lock() return false. This
would cause it to jump directly to the out_put label, completely skipping the
detachment of other events from old_rb->event_list and skipping
perf_mmap_unaccount().

Would this cause the old_rb to be freed while other events are still attached,
leading to a use-after-free if those events attempt to write to the freed
buffer? Does this also permanently leak memory charges since the unaccounting
is skipped?

[Severity: High]
This isn't a bug introduced by this patch, but it appears perf_mmap_aux()
lacks protection against mmapping redirected events. Could this lead to a
refcount saturation WARN?

In perf_mmap_aux(), the code validates that an rb exists but seems to fail to
verify if the event actually owns the mmap count via a check like
!refcount_read(&event->mmap_count). It unconditionally calls
refcount_inc(&event->mmap_count).

If a user redirects event B's output to event A via
ioctl(PERF_EVENT_IOC_SET_OUTPUT), setting B->mmap_count to 0, and then calls
mmap() on event B with a non-zero offset, will this cause a refcount
saturation WARN?

> +	else
> +		refcount_set(&event->mmap_count, 1);
>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830145512.2583689-1-ljp1205831794@gmail.com?part=1