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

["Mi, Dapeng" <dapeng1.mi@linux.intel.com>]

On 7/17/2026 6:38 PM, sashiko-bot@kernel.org wrote:
> 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?

Yeah, that looks a similar race. Considering BTS would not be used broadly,
would fix it later in an independent patch.

Thanks.


>