[PATCH] perf/core: Strengthen userpage update ordering

[PATCH] perf/core: Strengthen userpage update ordering

[Thaumy Cheng]

The perf event mmap userpage uses a sequence counter to let userspace
obtain a consistent snapshot of its data. The existing compiler
barriers reflect the original self-monitoring use case, where the
counter was updated and consumed on the same CPU.

Some consumers also use the userpage only for time conversion and may
read it from a CPU other than the one updating the event. For example,
perf_read_tsc_conversion() reads the time conversion fields
without constraining the caller to the event's CPU.

Make the publication ordering explicit for such readers by replacing the
compiler barriers around the userpage payload update with smp_wmb().
Document that cross-CPU readers of the time conversion fields must use
read memory barriers and reject odd or changed sequence values.

This does not change the UAPI layout or the values exposed to
userspace. It strengthens the ordering guarantee for cross-CPU readers
on weakly ordered architectures.

Signed-off-by: Thaumy Cheng <thaumy.love@gmail.com>
---
 include/uapi/linux/perf_event.h       | 6 ++++--
 kernel/events/core.c                  | 6 ++++--
 tools/include/uapi/linux/perf_event.h | 6 ++++--
 tools/perf/design.txt                 | 6 ++++--
 4 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index fd10aa8d697f..a7db00b9b455 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -629,8 +629,10 @@ struct perf_event_mmap_page {
 	 *     barrier();
 	 *   } while (pc->lock != seq);
 	 *
-	 * NOTE: for obvious reason this only works on self-monitoring
-	 *       processes.
+	 * NOTE: Reading the hardware counter as shown above only works for
+	 *       self-monitoring processes. A reader on another CPU may snapshot
+	 *       the time conversion fields, but must use rmb() around the field
+	 *       reads and retry if lock is odd or changes.
 	 */
 	__u32	lock;			/* seqlock for synchronization */
 	__u32	index;			/* hardware event identifier */
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 89b40e439717..72605776273f 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6852,7 +6852,8 @@ void perf_event_update_userpage(struct perf_event *event)
 	userpg = rb->user_page;
 
 	++userpg->lock;
-	barrier();
+	/* Publish the odd lock value before updating the payload. */
+	smp_wmb();
 	userpg->index = perf_event_index(event);
 	userpg->offset = perf_event_count(event, false);
 	if (userpg->index)
@@ -6866,7 +6867,8 @@ void perf_event_update_userpage(struct perf_event *event)
 
 	arch_perf_update_userpage(event, userpg, now);
 
-	barrier();
+	/* Publish the payload before the final lock update. */
+	smp_wmb();
 	++userpg->lock;
 	preempt_enable();
 unlock:
diff --git a/tools/include/uapi/linux/perf_event.h b/tools/include/uapi/linux/perf_event.h
index fd10aa8d697f..a7db00b9b455 100644
--- a/tools/include/uapi/linux/perf_event.h
+++ b/tools/include/uapi/linux/perf_event.h
@@ -629,8 +629,10 @@ struct perf_event_mmap_page {
 	 *     barrier();
 	 *   } while (pc->lock != seq);
 	 *
-	 * NOTE: for obvious reason this only works on self-monitoring
-	 *       processes.
+	 * NOTE: Reading the hardware counter as shown above only works for
+	 *       self-monitoring processes. A reader on another CPU may snapshot
+	 *       the time conversion fields, but must use rmb() around the field
+	 *       reads and retry if lock is odd or changes.
 	 */
 	__u32	lock;			/* seqlock for synchronization */
 	__u32	index;			/* hardware event identifier */
diff --git a/tools/perf/design.txt b/tools/perf/design.txt
index aa8cfeabb743..111afc90c442 100644
--- a/tools/perf/design.txt
+++ b/tools/perf/design.txt
@@ -316,8 +316,10 @@ struct perf_event_mmap_page {
          *     barrier();
          *   } while (pc->lock != seq);
          *
-         * NOTE: for obvious reason this only works on self-monitoring
-         *       processes.
+         * NOTE: Reading the hardware counter as shown above only works for
+         *       self-monitoring processes. A reader on another CPU may snapshot
+         *       the time conversion fields, but must use rmb() around the field
+         *       reads and retry if lock is odd or changes.
          */
         __u32   lock;                   /* seqlock for synchronization */
         __u32   index;                  /* hardware counter identifier */
-- 
2.55.0

Re: [PATCH] perf/core: Strengthen userpage update ordering

[sashiko-bot]

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

Pre-existing issues:
- [High] Preexisting seqlock writer re-entrancy bug in perf_event_update_userpage leading to corrupted data for userspace readers.
--

commit 019372a4e61c18f0c5a22ddb487c7acef14ffc7e
Author: Thaumy Cheng <thaumy.love@gmail.com>

perf/core: Strengthen userpage update ordering

This commit replaces compiler barriers with smp_wmb() around the userpage
payload update in perf_event_update_userpage(). It aims to strengthen the
publication ordering for cross-CPU readers of time conversion fields on
weakly ordered architectures.

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index a6c8e38a31104..9c6dfb79fd77d 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -6858,7 +6858,8 @@ void perf_event_update_userpage(struct perf_event *event)
>  	userpg = rb->user_page;
>  
>  	++userpg->lock;
> -	barrier();
> +	/* Publish the odd lock value before updating the payload. */
> +	smp_wmb();

[Severity: High]
This is a pre-existing issue, but does perf_event_update_userpage() have a
seqlock writer re-entrancy bug here that defeats the synchronization?

Since this function disables preemption (via preempt_disable() earlier in
the function) but leaves hardware interrupts enabled when called from task
context (such as from perf_mmap_rb()), what happens if a PMU hardware
interrupt like an NMI triggers during this window? 

If the NMI handler also calls perf_event_update_userpage() for the same
event, it would increment userpg->lock again, advancing it to an even value.
A concurrent userspace reader on another CPU would then see an even lock
value and could read actively-torn payload data (like time_mult, time_shift,
or time_offset), leading to bogus timestamps. 

Should hardware interrupts be disabled before incrementing the seqlock to
prevent this writer re-entrancy?

>  	userpg->index = perf_event_index(event);
>  	userpg->offset = perf_event_count(event, false);

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