Re: [Patch v3 3/7] perf/x86: Check if cpuc->events[*] pointer exists before accessing it

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

On 8/22/2025 1:26 PM, Mi, Dapeng wrote:
> On 8/21/2025 9:35 PM, Peter Zijlstra wrote:
>> On Wed, Aug 20, 2025 at 10:30:28AM +0800, Dapeng Mi wrote:
>>> When intel_pmu_drain_pebs_icl() is called to drain PEBS records, the
>>> perf_event_overflow() could be called to process the last PEBS record.
>>>
>>> While perf_event_overflow() could trigger the interrupt throttle and
>>> stop all events of the group, like what the below call-chain shows.
>>>
>>> perf_event_overflow()
>>>   -> __perf_event_overflow()
>>>     ->__perf_event_account_interrupt()
>>>       -> perf_event_throttle_group()
>>>         -> perf_event_throttle()
>>>           -> event->pmu->stop()
>>>             -> x86_pmu_stop()
>>>
>>> The side effect of stopping the events is that all corresponding event
>>> pointers in cpuc->events[] array are cleared to NULL.
>>>
>>> Assume there are two PEBS events (event a and event b) in a group. When
>>> intel_pmu_drain_pebs_icl() calls perf_event_overflow() to process the
>>> last PEBS record of PEBS event a, interrupt throttle is triggered and
>>> all pointers of event a and event b are cleared to NULL. Then
>>> intel_pmu_drain_pebs_icl() tries to process the last PEBS record of
>>> event b and encounters NULL pointer access.
>>>
>>> Since the left PEBS records have been processed when stopping the event,
>>> check and skip to process the last PEBS record if cpuc->events[*] is
>>> NULL.
>>>
>>> Reported-by: kernel test robot <oliver.sang@intel.com>
>>> Closes: https://lore.kernel.org/oe-lkp/202507042103.a15d2923-lkp@intel.com
>>> Fixes: 9734e25fbf5a ("perf: Fix the throttle logic for a group")
>>> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
>>> Tested-by: kernel test robot <oliver.sang@intel.com>
>>> ---
>>>  arch/x86/events/intel/ds.c | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
>>> index c0b7ac1c7594..dcf29c099ad2 100644
>>> --- a/arch/x86/events/intel/ds.c
>>> +++ b/arch/x86/events/intel/ds.c
>>> @@ -2663,6 +2663,16 @@ static void intel_pmu_drain_pebs_icl(struct pt_regs *iregs, struct perf_sample_d
>>>  			continue;
>>>  
>>>  		event = cpuc->events[bit];
>>> +		/*
>>> +		 * perf_event_overflow() called by below __intel_pmu_pebs_last_event()
>>> +		 * could trigger interrupt throttle and clear all event pointers of the
>>> +		 * group in cpuc->events[] to NULL. So need to re-check if cpuc->events[*]
>>> +		 * is NULL, if so it indicates the event has been throttled (stopped) and
>>> +		 * the corresponding last PEBS records have been processed in stopping
>>> +		 * event, don't need to process it again.
>>> +		 */
>>> +		if (!event)
>>> +			continue;
>>>  
>>>  		__intel_pmu_pebs_last_event(event, iregs, regs, data, last[bit],
>>>  					    counts[bit], setup_pebs_adaptive_sample_data);
>> So if this is due to __intel_pmu_pebs_last_event() calling into
>> perf_event_overflow(); then isn't intel_pmu_drain_pebs_nhm() similarly
>> affected?
>>
>> And worse, the _nhm() version would loose all events for that counter,
>> not just the last.
> hmm, Yes. After double check, I suppose I made a mistake for the answer to
> Andi. It indeed has data loss since the "ds->pebs_index" is reset at the
> head of _nhm()/_icl() these drain_pebs helper instead of the end of the
> drain_pebs helper.  :(
>
>> I'm really thinking this isn't the right thing to do.
>>
>>
>> How about we audit the entirety of arch/x86/events/ for cpuc->events[]
>> usage and see if we can get away with changing x86_pmu_stop() to simply
>> not clearing that field.
> Checking current code, I suppose it's fine that we don't clear
> cpuc->events[] in x86_pmu_stop() since we already have another variable
> "cpuc->active_mask" which is used to indicate if the corresponding
> cpuc->events[*] is active. But in current code, the cpuc->active_mask is
> not always checked.
>
> So if we select not to clear cpuc->events[] in x86_pmu_stop(), then it's a
> must to check cpuc->active_mask before really accessing cpuc->events[]
> represented event. Maybe we can add an inline function got check this.
>
> bool inline x86_pmu_cntr_event_active(int idx)
> {
>     struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>
>     return cpuc->events[idx] && test_bit(idx, cpuc->active_mask);
> }

Just think twice about this method, it breaks the original logic about
cpuc->events[] (setting at x86_pmu_start() and clearing at x86_pmu_stop())
and this leaves the cpuc->events[] is never cleared, this is ambiguous.
Besides checking cpuc->events[idx] and cpuc->active_mask is not atomic,
this may bring potential risks especially considering cpuc->events[] are
broadly used in x86/perf code. 

Talked with Kan offline, he suggests to get a events[] snapshot from
cpuc->events[] before calling perf_event_overflow() and then use the
events[] snapshot to process all left PEBS records. That seems a better and
safer method to fix this issue for me. 

Peter, if you don't object this method, I would follow Kan's suggestion.
Thanks.


>
>> Or perhaps move the setting and clearing into x86_pmu_{add,del}() rather
>> than x86_pmu_{start,stop}(). After all, the latter don't affect the
>> counter placement, they just stop/start the event.
> IIUC, we could not move the setting into x86_pmu_add() from x86_pmu_stop()
> since the counter index is not finalized at x86_pmu_add() is called. The
> counter index could change for each adding a new event.
>
>
>>