Re: [PATCH] perf/x86/intel: Fix segfault with PEBS-via-PT with sample_freq

["Liang, Kan" <kan.liang@linux.intel.com>]

On 2025-05-08 9:44 a.m., Adrian Hunter wrote:
> Currently, using PEBS-via-PT with a sample frequency instead of a sample
> period, causes a segfault.  For example:
> 
>  [  103.607823] BUG: kernel NULL pointer dereference, address: 0000000000000195
>  [  103.607876]  <NMI>
>  [  103.607879]  ? __die_body.cold+0x19/0x27
>  [  103.607885]  ? page_fault_oops+0xca/0x290
>  [  103.607891]  ? exc_page_fault+0x7e/0x1b0
>  [  103.607897]  ? asm_exc_page_fault+0x26/0x30
>  [  103.607901]  ? intel_pmu_pebs_event_update_no_drain+0x40/0x60
>  [  103.607903]  ? intel_pmu_pebs_event_update_no_drain+0x32/0x60
>  [  103.607905]  intel_pmu_drain_pebs_icl+0x333/0x350
>  [  103.607910]  handle_pmi_common+0x272/0x3c0
>  [  103.607919]  intel_pmu_handle_irq+0x10a/0x2e0
>  [  103.607922]  perf_event_nmi_handler+0x2a/0x50
> 
> That happens because intel_pmu_pebs_event_update_no_drain() assumes all the
> pebs_enabled bits represent counter indexes, which is not always the case.
> In this particular case, bits 60 and 61 are set for PEBS-via-PT purposes.
> 
> The behaviour of PEBS-via-PT with sample frequency is questionable because
> although a PMI is generated (PEBS_PMI_AFTER_EACH_RECORD), the period is not
> adjusted anyway.
> 
> Putting that aside, fix intel_pmu_pebs_event_update_no_drain() by passing
> the mask of counter bits instead of 'size'.  Note, prior to the Fixes
> commit, 'size' would be limited to the maximum counter index, so the issue
> was not hit.
> 
> Fixes: 722e42e45c2f1 ("perf/x86: Support counter mask")
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>

Thanks for the fix.

Reviewed-by: Kan Liang <kan.liang@linux.intel.com>

Thanks,
Kan

> ---
>  arch/x86/events/intel/ds.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
> index adb2e44761b2..8da1105a419f 100644
> --- a/arch/x86/events/intel/ds.c
> +++ b/arch/x86/events/intel/ds.c
> @@ -2469,8 +2469,9 @@ static void intel_pmu_drain_pebs_core(struct pt_regs *iregs, struct perf_sample_
>  				setup_pebs_fixed_sample_data);
>  }
>  
> -static void intel_pmu_pebs_event_update_no_drain(struct cpu_hw_events *cpuc, int size)
> +static void intel_pmu_pebs_event_update_no_drain(struct cpu_hw_events *cpuc, u64 mask)
>  {
> +	u64 pebs_enabled = cpuc->pebs_enabled & mask;
>  	struct perf_event *event;
>  	int bit;
>  
> @@ -2481,7 +2482,7 @@ static void intel_pmu_pebs_event_update_no_drain(struct cpu_hw_events *cpuc, int
>  	 * It needs to call intel_pmu_save_and_restart_reload() to
>  	 * update the event->count for this case.
>  	 */
> -	for_each_set_bit(bit, (unsigned long *)&cpuc->pebs_enabled, size) {
> +	for_each_set_bit(bit, (unsigned long *)&pebs_enabled, X86_PMC_IDX_MAX) {
>  		event = cpuc->events[bit];
>  		if (event->hw.flags & PERF_X86_EVENT_AUTO_RELOAD)
>  			intel_pmu_save_and_restart_reload(event, 0);
> @@ -2516,7 +2517,7 @@ static void intel_pmu_drain_pebs_nhm(struct pt_regs *iregs, struct perf_sample_d
>  	}
>  
>  	if (unlikely(base >= top)) {
> -		intel_pmu_pebs_event_update_no_drain(cpuc, size);
> +		intel_pmu_pebs_event_update_no_drain(cpuc, mask);
>  		return;
>  	}
>  
> @@ -2630,7 +2631,7 @@ static void intel_pmu_drain_pebs_icl(struct pt_regs *iregs, struct perf_sample_d
>  	       (hybrid(cpuc->pmu, fixed_cntr_mask64) << INTEL_PMC_IDX_FIXED);
>  
>  	if (unlikely(base >= top)) {
> -		intel_pmu_pebs_event_update_no_drain(cpuc, X86_PMC_IDX_MAX);
> +		intel_pmu_pebs_event_update_no_drain(cpuc, mask);
>  		return;
>  	}
>