[Patch v2] perf/x86/intel: Prevent drain_pebs() reentry

[Patch v2] perf/x86/intel: Prevent drain_pebs() reentry

[Dapeng Mi]

The PEBS buffer is shared by all events on a CPU, so drain_pebs() must
not be reentered. If so, one instance may observe stale buffer state and
potentially access out-of-bound memory.

Most invocations happen in NMI context, which naturally prevents reentry.
However, drain_pebs() is also reachable from process context via
intel_pmu_drain_pebs_buffer().

In those paths, the PMU is often already disabled, but not guaranteed.
For example, __intel_pmu_pebs_disable() only disables the target counter,
so other active counters can still raise a PMI and interrupt an in-flight
drain_pebs(). Here is an example,

__perf_addr_filters_adjust()
  perf_event_stop()
    __perf_event_stop()
      x86_pmu_stop() (event->pmu->stop)
        intel_pmu_disable_event()
          intel_pmu_pebs_disable()
            __intel_pmu_pebs_disable()
              intel_pmu_drain_large_pebs()
                intel_pmu_drain_pebs_buffer()

Introduce __intel_pmu_quiesce() and __intel_pmu_resume() helpers and
use them in intel_pmu_drain_large_pebs() to disable the full PMU
around the intel_pmu_drain_pebs_buffer() call, preventing reentry.

Also add a warning in intel_pmu_drain_pebs_buffer() when the full PMU is
not disabled.

Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---

v2: Move __intel_pmu_quiesce()/__intel_pmu_resume() pair into
intel_pmu_drain_large_pebs() instead of guarding all drain_pebs()
callbacks. (Peter)

v1: https://lore.kernel.org/all/20260717080342.1879573-9-dapeng1.mi@linux.intel.com/

 arch/x86/events/intel/core.c | 33 ++++++++++++++++++++++++---------
 arch/x86/events/intel/ds.c   |  8 +++++++-
 arch/x86/events/perf_event.h |  3 +++
 3 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index f6ee1819864e..1cdb52c0aad5 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -3125,6 +3125,27 @@ static void intel_pmu_del_event(struct perf_event *event)
 		this_cpu_ptr(&cpu_hw_events)->n_late_setup--;
 }
 
+int __intel_pmu_quiesce(void)
+{
+	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
+	int pmu_enabled = cpuc->enabled;
+
+	cpuc->enabled = 0;
+	if (pmu_enabled)
+		intel_pmu_disable_all();
+
+	return pmu_enabled;
+}
+
+void __intel_pmu_resume(int pmu_enabled)
+{
+	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
+
+	cpuc->enabled = pmu_enabled;
+	if (pmu_enabled)
+		intel_pmu_enable_all(0);
+}
+
 static int icl_set_topdown_event_period(struct perf_event *event)
 {
 	struct hw_perf_event *hwc = &event->hw;
@@ -3316,16 +3337,13 @@ static void intel_pmu_read_event(struct perf_event *event)
 	if (event->hw.flags & (PERF_X86_EVENT_AUTO_RELOAD | PERF_X86_EVENT_TOPDOWN) ||
 	    is_pebs_counter_event_group(event)) {
 		struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
-		bool pmu_enabled = cpuc->enabled;
+		int pmu_enabled;
 
 		/* Only need to call update_topdown_event() once for group read. */
 		if (is_metric_event(event) && (cpuc->txn_flags & PERF_PMU_TXN_READ))
 			return;
 
-		cpuc->enabled = 0;
-		if (pmu_enabled)
-			intel_pmu_disable_all();
-
+		pmu_enabled = __intel_pmu_quiesce();
 		/*
 		 * If the PEBS counters snapshotting is enabled,
 		 * the topdown event is available in PEBS records.
@@ -3334,10 +3352,7 @@ static void intel_pmu_read_event(struct perf_event *event)
 			static_call(intel_pmu_update_topdown_event)(event, NULL);
 		else
 			intel_pmu_drain_pebs_buffer();
-
-		cpuc->enabled = pmu_enabled;
-		if (pmu_enabled)
-			intel_pmu_enable_all(0);
+		__intel_pmu_resume(pmu_enabled);
 
 		return;
 	}
diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
index e86e4ba91e1b..54890dda0589 100644
--- a/arch/x86/events/intel/ds.c
+++ b/arch/x86/events/intel/ds.c
@@ -1242,8 +1242,11 @@ int intel_pmu_drain_bts_buffer(void)
 
 void intel_pmu_drain_pebs_buffer(void)
 {
+	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
 	struct perf_sample_data data;
 
+	WARN_ON_ONCE(cpuc->enabled);
+
 	static_call(x86_pmu_drain_pebs)(NULL, &data);
 }
 
@@ -1864,8 +1867,11 @@ static void intel_pmu_pebs_via_pt_enable(struct perf_event *event)
 static inline void intel_pmu_drain_large_pebs(struct cpu_hw_events *cpuc)
 {
 	if (cpuc->n_pebs == cpuc->n_large_pebs &&
-	    cpuc->n_pebs != cpuc->n_pebs_via_pt)
+	    cpuc->n_pebs != cpuc->n_pebs_via_pt) {
+		int enabled = __intel_pmu_quiesce();
 		intel_pmu_drain_pebs_buffer();
+		__intel_pmu_resume(enabled);
+	}
 }
 
 static void __intel_pmu_pebs_enable(struct perf_event *event)
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index fa381110f7a7..84495279079a 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -1638,6 +1638,9 @@ static __always_inline void __intel_pmu_lbr_disable(void)
 	wrmsrq(MSR_IA32_DEBUGCTLMSR, debugctl);
 }
 
+extern int __intel_pmu_quiesce(void);
+extern void __intel_pmu_resume(int pmu_enabled);
+
 int intel_pmu_save_and_restart(struct perf_event *event);
 
 struct event_constraint *

base-commit: 917d558b151cad5b05991e5eaee22efab33525ca
-- 
2.34.1

Re: [Patch v2] perf/x86/intel: Prevent drain_pebs() reentry

[sashiko-bot]

> The PEBS buffer is shared by all events on a CPU, so drain_pebs() must
> not be reentered. If so, one instance may observe stale buffer state and
> potentially access out-of-bound memory.
> 
> Most invocations happen in NMI context, which naturally prevents reentry.
> However, drain_pebs() is also reachable from process context via
> intel_pmu_drain_pebs_buffer().
> 
> In those paths, the PMU is often already disabled, but not guaranteed.
> For example, __intel_pmu_pebs_disable() only disables the target counter,
> so other active counters can still raise a PMI and interrupt an in-flight
> drain_pebs(). Here is an example,
> 
> __perf_addr_filters_adjust()
>   perf_event_stop()
> [ ... ]
> 
> Also add a warning in intel_pmu_drain_pebs_buffer() when the full PMU is
> not disabled.
> 
> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813064346.335458-1-dapeng1.mi@linux.intel.com?part=1

Re: [Patch v2] perf/x86/intel: Prevent drain_pebs() reentry

[Ian Rogers]

On Wed, Aug 12, 2026 at 11:50 PM Dapeng Mi <dapeng1.mi@linux.intel.com> wrote:
>
> The PEBS buffer is shared by all events on a CPU, so drain_pebs() must
> not be reentered. If so, one instance may observe stale buffer state and
> potentially access out-of-bound memory.
>
> Most invocations happen in NMI context, which naturally prevents reentry.
> However, drain_pebs() is also reachable from process context via
> intel_pmu_drain_pebs_buffer().
>
> In those paths, the PMU is often already disabled, but not guaranteed.
> For example, __intel_pmu_pebs_disable() only disables the target counter,
> so other active counters can still raise a PMI and interrupt an in-flight
> drain_pebs(). Here is an example,
>
> __perf_addr_filters_adjust()
>   perf_event_stop()
>     __perf_event_stop()
>       x86_pmu_stop() (event->pmu->stop)
>         intel_pmu_disable_event()
>           intel_pmu_pebs_disable()
>             __intel_pmu_pebs_disable()
>               intel_pmu_drain_large_pebs()
>                 intel_pmu_drain_pebs_buffer()
>
> Introduce __intel_pmu_quiesce() and __intel_pmu_resume() helpers and
> use them in intel_pmu_drain_large_pebs() to disable the full PMU
> around the intel_pmu_drain_pebs_buffer() call, preventing reentry.
>
> Also add a warning in intel_pmu_drain_pebs_buffer() when the full PMU is
> not disabled.
>
> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
> ---
>
> v2: Move __intel_pmu_quiesce()/__intel_pmu_resume() pair into
> intel_pmu_drain_large_pebs() instead of guarding all drain_pebs()
> callbacks. (Peter)
>
> v1: https://lore.kernel.org/all/20260717080342.1879573-9-dapeng1.mi@linux.intel.com/
>
>  arch/x86/events/intel/core.c | 33 ++++++++++++++++++++++++---------
>  arch/x86/events/intel/ds.c   |  8 +++++++-
>  arch/x86/events/perf_event.h |  3 +++
>  3 files changed, 34 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index f6ee1819864e..1cdb52c0aad5 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -3125,6 +3125,27 @@ static void intel_pmu_del_event(struct perf_event *event)
>                 this_cpu_ptr(&cpu_hw_events)->n_late_setup--;
>  }
>
> +int __intel_pmu_quiesce(void)

Thanks Dapeng. Rather than quiesce would
__intel_pmu_disable_if_enabled be a more intention revealing name?

> +{
> +       struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
> +       int pmu_enabled = cpuc->enabled;
> +
> +       cpuc->enabled = 0;

Presumably the line above could also be inside the if.

> +       if (pmu_enabled)
> +               intel_pmu_disable_all();
> +
> +       return pmu_enabled;
> +}
> +
> +void __intel_pmu_resume(int pmu_enabled)
> +{
> +       struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
> +
> +       cpuc->enabled = pmu_enabled;
> +       if (pmu_enabled)
> +               intel_pmu_enable_all(0);
> +}
> +
>  static int icl_set_topdown_event_period(struct perf_event *event)
>  {
>         struct hw_perf_event *hwc = &event->hw;
> @@ -3316,16 +3337,13 @@ static void intel_pmu_read_event(struct perf_event *event)
>         if (event->hw.flags & (PERF_X86_EVENT_AUTO_RELOAD | PERF_X86_EVENT_TOPDOWN) ||
>             is_pebs_counter_event_group(event)) {
>                 struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
> -               bool pmu_enabled = cpuc->enabled;
> +               int pmu_enabled;

nit: Maybe pmu_enabled would be better as pmu_was_enabled or pmu_reenable?

>
>                 /* Only need to call update_topdown_event() once for group read. */
>                 if (is_metric_event(event) && (cpuc->txn_flags & PERF_PMU_TXN_READ))
>                         return;
>
> -               cpuc->enabled = 0;
> -               if (pmu_enabled)
> -                       intel_pmu_disable_all();
> -
> +               pmu_enabled = __intel_pmu_quiesce();
>                 /*
>                  * If the PEBS counters snapshotting is enabled,
>                  * the topdown event is available in PEBS records.
> @@ -3334,10 +3352,7 @@ static void intel_pmu_read_event(struct perf_event *event)
>                         static_call(intel_pmu_update_topdown_event)(event, NULL);
>                 else
>                         intel_pmu_drain_pebs_buffer();
> -
> -               cpuc->enabled = pmu_enabled;
> -               if (pmu_enabled)
> -                       intel_pmu_enable_all(0);
> +               __intel_pmu_resume(pmu_enabled);
>
>                 return;
>         }
> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
> index e86e4ba91e1b..54890dda0589 100644
> --- a/arch/x86/events/intel/ds.c
> +++ b/arch/x86/events/intel/ds.c
> @@ -1242,8 +1242,11 @@ int intel_pmu_drain_bts_buffer(void)
>
>  void intel_pmu_drain_pebs_buffer(void)
>  {
> +       struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>         struct perf_sample_data data;
>
> +       WARN_ON_ONCE(cpuc->enabled);
> +
>         static_call(x86_pmu_drain_pebs)(NULL, &data);
>  }
>
> @@ -1864,8 +1867,11 @@ static void intel_pmu_pebs_via_pt_enable(struct perf_event *event)
>  static inline void intel_pmu_drain_large_pebs(struct cpu_hw_events *cpuc)
>  {
>         if (cpuc->n_pebs == cpuc->n_large_pebs &&
> -           cpuc->n_pebs != cpuc->n_pebs_via_pt)
> +           cpuc->n_pebs != cpuc->n_pebs_via_pt) {
> +               int enabled = __intel_pmu_quiesce();

Similarly, may be was_enabled rather than enabled.

Thanks,
Ian

>                 intel_pmu_drain_pebs_buffer();
> +               __intel_pmu_resume(enabled);
> +       }
>  }
>
>  static void __intel_pmu_pebs_enable(struct perf_event *event)
> diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
> index fa381110f7a7..84495279079a 100644
> --- a/arch/x86/events/perf_event.h
> +++ b/arch/x86/events/perf_event.h
> @@ -1638,6 +1638,9 @@ static __always_inline void __intel_pmu_lbr_disable(void)
>         wrmsrq(MSR_IA32_DEBUGCTLMSR, debugctl);
>  }
>
> +extern int __intel_pmu_quiesce(void);
> +extern void __intel_pmu_resume(int pmu_enabled);
> +
>  int intel_pmu_save_and_restart(struct perf_event *event);
>
>  struct event_constraint *
>
> base-commit: 917d558b151cad5b05991e5eaee22efab33525ca
> --
> 2.34.1
>

Re: [Patch v2] perf/x86/intel: Prevent drain_pebs() reentry

[Mi, Dapeng]

On 8/18/2026 3:28 AM, Ian Rogers wrote:
> On Wed, Aug 12, 2026 at 11:50 PM Dapeng Mi <dapeng1.mi@linux.intel.com> wrote:
>> The PEBS buffer is shared by all events on a CPU, so drain_pebs() must
>> not be reentered. If so, one instance may observe stale buffer state and
>> potentially access out-of-bound memory.
>>
>> Most invocations happen in NMI context, which naturally prevents reentry.
>> However, drain_pebs() is also reachable from process context via
>> intel_pmu_drain_pebs_buffer().
>>
>> In those paths, the PMU is often already disabled, but not guaranteed.
>> For example, __intel_pmu_pebs_disable() only disables the target counter,
>> so other active counters can still raise a PMI and interrupt an in-flight
>> drain_pebs(). Here is an example,
>>
>> __perf_addr_filters_adjust()
>>   perf_event_stop()
>>     __perf_event_stop()
>>       x86_pmu_stop() (event->pmu->stop)
>>         intel_pmu_disable_event()
>>           intel_pmu_pebs_disable()
>>             __intel_pmu_pebs_disable()
>>               intel_pmu_drain_large_pebs()
>>                 intel_pmu_drain_pebs_buffer()
>>
>> Introduce __intel_pmu_quiesce() and __intel_pmu_resume() helpers and
>> use them in intel_pmu_drain_large_pebs() to disable the full PMU
>> around the intel_pmu_drain_pebs_buffer() call, preventing reentry.
>>
>> Also add a warning in intel_pmu_drain_pebs_buffer() when the full PMU is
>> not disabled.
>>
>> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
>> ---
>>
>> v2: Move __intel_pmu_quiesce()/__intel_pmu_resume() pair into
>> intel_pmu_drain_large_pebs() instead of guarding all drain_pebs()
>> callbacks. (Peter)
>>
>> v1: https://lore.kernel.org/all/20260717080342.1879573-9-dapeng1.mi@linux.intel.com/
>>
>>  arch/x86/events/intel/core.c | 33 ++++++++++++++++++++++++---------
>>  arch/x86/events/intel/ds.c   |  8 +++++++-
>>  arch/x86/events/perf_event.h |  3 +++
>>  3 files changed, 34 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>> index f6ee1819864e..1cdb52c0aad5 100644
>> --- a/arch/x86/events/intel/core.c
>> +++ b/arch/x86/events/intel/core.c
>> @@ -3125,6 +3125,27 @@ static void intel_pmu_del_event(struct perf_event *event)
>>                 this_cpu_ptr(&cpu_hw_events)->n_late_setup--;
>>  }
>>
>> +int __intel_pmu_quiesce(void)
> Thanks Dapeng. Rather than quiesce would
> __intel_pmu_disable_if_enabled be a more intention revealing name?

Thanks Ian for reviewing this patch. 

Yeah, I was hesitated about the name for a while. Currently I still prefer
the name __intel_pmu_quiesce() which is simplified enough than other names. 


>
>> +{
>> +       struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>> +       int pmu_enabled = cpuc->enabled;
>> +
>> +       cpuc->enabled = 0;
> Presumably the line above could also be inside the if.

Yes. I'm not sure if it's worthy to post a new version for this minor
change. But If there is a new version, I would change it.


>
>> +       if (pmu_enabled)
>> +               intel_pmu_disable_all();
>> +
>> +       return pmu_enabled;
>> +}
>> +
>> +void __intel_pmu_resume(int pmu_enabled)
>> +{
>> +       struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>> +
>> +       cpuc->enabled = pmu_enabled;
>> +       if (pmu_enabled)
>> +               intel_pmu_enable_all(0);
>> +}
>> +
>>  static int icl_set_topdown_event_period(struct perf_event *event)
>>  {
>>         struct hw_perf_event *hwc = &event->hw;
>> @@ -3316,16 +3337,13 @@ static void intel_pmu_read_event(struct perf_event *event)
>>         if (event->hw.flags & (PERF_X86_EVENT_AUTO_RELOAD | PERF_X86_EVENT_TOPDOWN) ||
>>             is_pebs_counter_event_group(event)) {
>>                 struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>> -               bool pmu_enabled = cpuc->enabled;
>> +               int pmu_enabled;
> nit: Maybe pmu_enabled would be better as pmu_was_enabled or pmu_reenable?

pmu_enabled seems better for me. pmu_enabled is simplified enough comparing
with other names. 

Thanks.


>
>>                 /* Only need to call update_topdown_event() once for group read. */
>>                 if (is_metric_event(event) && (cpuc->txn_flags & PERF_PMU_TXN_READ))
>>                         return;
>>
>> -               cpuc->enabled = 0;
>> -               if (pmu_enabled)
>> -                       intel_pmu_disable_all();
>> -
>> +               pmu_enabled = __intel_pmu_quiesce();
>>                 /*
>>                  * If the PEBS counters snapshotting is enabled,
>>                  * the topdown event is available in PEBS records.
>> @@ -3334,10 +3352,7 @@ static void intel_pmu_read_event(struct perf_event *event)
>>                         static_call(intel_pmu_update_topdown_event)(event, NULL);
>>                 else
>>                         intel_pmu_drain_pebs_buffer();
>> -
>> -               cpuc->enabled = pmu_enabled;
>> -               if (pmu_enabled)
>> -                       intel_pmu_enable_all(0);
>> +               __intel_pmu_resume(pmu_enabled);
>>
>>                 return;
>>         }
>> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
>> index e86e4ba91e1b..54890dda0589 100644
>> --- a/arch/x86/events/intel/ds.c
>> +++ b/arch/x86/events/intel/ds.c
>> @@ -1242,8 +1242,11 @@ int intel_pmu_drain_bts_buffer(void)
>>
>>  void intel_pmu_drain_pebs_buffer(void)
>>  {
>> +       struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>>         struct perf_sample_data data;
>>
>> +       WARN_ON_ONCE(cpuc->enabled);
>> +
>>         static_call(x86_pmu_drain_pebs)(NULL, &data);
>>  }
>>
>> @@ -1864,8 +1867,11 @@ static void intel_pmu_pebs_via_pt_enable(struct perf_event *event)
>>  static inline void intel_pmu_drain_large_pebs(struct cpu_hw_events *cpuc)
>>  {
>>         if (cpuc->n_pebs == cpuc->n_large_pebs &&
>> -           cpuc->n_pebs != cpuc->n_pebs_via_pt)
>> +           cpuc->n_pebs != cpuc->n_pebs_via_pt) {
>> +               int enabled = __intel_pmu_quiesce();
> Similarly, may be was_enabled rather than enabled.
>
> Thanks,
> Ian
>
>>                 intel_pmu_drain_pebs_buffer();
>> +               __intel_pmu_resume(enabled);
>> +       }
>>  }
>>
>>  static void __intel_pmu_pebs_enable(struct perf_event *event)
>> diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
>> index fa381110f7a7..84495279079a 100644
>> --- a/arch/x86/events/perf_event.h
>> +++ b/arch/x86/events/perf_event.h
>> @@ -1638,6 +1638,9 @@ static __always_inline void __intel_pmu_lbr_disable(void)
>>         wrmsrq(MSR_IA32_DEBUGCTLMSR, debugctl);
>>  }
>>
>> +extern int __intel_pmu_quiesce(void);
>> +extern void __intel_pmu_resume(int pmu_enabled);
>> +
>>  int intel_pmu_save_and_restart(struct perf_event *event);
>>
>>  struct event_constraint *
>>
>> base-commit: 917d558b151cad5b05991e5eaee22efab33525ca
>> --
>> 2.34.1
>>