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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] TOCTOU race in intel_pmu_drain_bts_buffer leading to duplicate BTS records.
--

--- Patch [8]: [Patch v3 8/8] perf/x86/intel: Prevent drain_pebs() reentry ---
commit 77ebceadaa828dfb1bed75e560551cf29b1ae9f5
Author: Dapeng Mi <dapeng1.mi@linux.intel.com>

perf/x86/intel: Prevent drain_pebs() reentry

The PEBS buffer is shared by all events on a CPU, so drain_pebs() must
not run concurrently. This commit introduces __intel_pmu_quiesce() and
__intel_pmu_resume() helpers to disable the full PMU around the
drain_pebs() call, preventing reentry.

> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
> index e86e4ba91e1b3..7f8b98d1837dc 100644
> --- a/arch/x86/events/intel/ds.c
> +++ b/arch/x86/events/intel/ds.c
> @@ -1240,13 +1240,6 @@ int intel_pmu_drain_bts_buffer(void)
>  	return 1;
>  }

[Severity: High]
This is a pre-existing issue, but since you are fixing the drain reentry 
race for PEBS, does intel_pmu_drain_bts_buffer() need similar protection?

If a user configures a BTS event alongside another active counter and 
disables the BTS event, the stop IPI can call this sequence without 
globally disabling the PMU:

__perf_event_stop()
  x86_pmu_stop()
    intel_pmu_disable_event()
      intel_pmu_drain_bts_buffer()

If a PMI interrupts execution precisely between reading the top pointer and 
resetting the index:

arch/x86/events/intel/ds.c:intel_pmu_drain_bts_buffer() {
    ...
    base = (struct bts_record *)(unsigned long)ds->bts_buffer_base;
    top  = (struct bts_record *)(unsigned long)ds->bts_index;

    if (top <= base)
        return 0;

    memset(&regs, 0, sizeof(regs));

    <-- PMI FIRES HERE -->

    ds->bts_index = ds->bts_buffer_base;
    ...
}

Could the NMI handler unconditionally call intel_pmu_drain_bts_buffer(), 
draining the records and resetting the index, only for the interrupted 
context to resume and push the exact same records again using its stale 
top pointer?

Would it make sense to wrap the BTS draining logic with the newly 
introduced __intel_pmu_quiesce() and __intel_pmu_resume() helpers as well?

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