Re: [PATCH v2] perf: Fix refcount warning on event->mmap_count increment

[Peter Zijlstra <peterz@infradead.org>]

On Mon, Jan 05, 2026 at 09:51:49AM -0700, Will Rosenberg wrote:
> When calling refcount_inc(&event->mmap_count) inside perf_mmap_rb(), the
> following warning is triggered:
> 
> 	refcount_t: addition on 0; use-after-free.
> 	WARNING: lib/refcount.c:25
> 
> PoC:
> 
>     struct perf_event_attr attr = {0};
>     int fd = syscall(__NR_perf_event_open, &attr, 0, -1, -1, 0);
>     mmap(NULL, 0x3000, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
>     int victim = syscall(__NR_perf_event_open, &attr, 0, -1, fd,
>                          PERF_FLAG_FD_OUTPUT);
>     mmap(NULL, 0x3000, PROT_READ | PROT_WRITE, MAP_SHARED, victim, 0);
> 
> This occurs when creating a group member event with the flag
> PERF_FLAG_FD_OUTPUT. The group leader should be mmap-ed and then mmap-ing
> the event triggers the warning.
> 
> Since the event has copied the output_event in perf_event_set_output(),
> event->rb is set. As a result, perf_mmap_rb() calls
> refcount_inc(&event->mmap_count) when event->mmap_count = 0.
> 
> Account for the case when event->mmap_count = 0. This patch goes against
> the design philosophy of the refcount library by re-enabling an empty
> refcount, but the patch remains inline with the current treatment of
> mmap_count.
> 
> Fixes: 448f97fba901 ("perf: Convert mmap() refcounts to refcount_t")
> Signed-off-by: Will Rosenberg <whrosenb@asu.edu>
> ---
> 
> Notes:
>     v1 -> v2: Add Fixes tag
>     
>     I also have a related concern about code that handles the mmap_count.
>     In perf_mmap_close(), if refcount_dec_and_mutex_lock() decrements
>     event->mmap_count to zero, then event->rb is set to NULL. This
>     effectively undos our ring buffer copy. However, is this desired
>     behavior? Should event->rb remain unchanged since it may still be
>     mmap-ed by other events and can still be used?
> 
>  kernel/events/core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 376fb07d869b..49709b627b1f 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -7279,7 +7279,8 @@ static int perf_mmap_rb(struct vm_area_struct *vma, struct perf_event *event,
>  			 * multiple times.
>  			 */
>  			perf_mmap_account(vma, user_extra, extra);
> -			refcount_inc(&event->mmap_count);
> +			if (!refcount_inc_not_zero(&event->mmap_count))
> +				refcount_set(&event->mmap_count, 1);

So this pattern was an instant red flag; this cannot be right.

Yes, this makes the error go away, but I think the result is bad. 

The sequence as provided will create a mapping for event fd, and create
victim such that its events are redirected to this buffer.

So far so good.

However, the mmap() of victim will create an alias of the earlier buffer
(which is pointless but isn't a problem per-se), but by setting
event->mmap_count, you're saying this second event should also update
the user_page (struct perf_event_mmap_page at offset +0).

This means that if you make this succeed you end up with both events
(fd, victim) writing to the same page (which is mapped twice). And that
is broken.

I'm thinking we should dis-allow this mmap()... something like so, hmm?

---
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 3c2a491200c6..ccf3aecbfff5 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -7273,6 +7273,15 @@ static int perf_mmap_rb(struct vm_area_struct *vma, struct perf_event *event,
 		if (data_page_nr(event->rb) != nr_pages)
 			return -EINVAL;
 
+		/*
+		 * If this event doesn't have mmap_count, we're attempting to
+		 * create an alias of another event's mmap(); this would mean
+		 * both events will end up scribbling the same user_page;
+		 * which makes no sense.
+		 */
+		if (refcount_read(&event->mmap_count))
+			return -EBUSY;
+
 		if (refcount_inc_not_zero(&event->rb->mmap_count)) {
 			/*
 			 * Success -- managed to mmap() the same buffer