Re: [Patch v7 22/24] perf/x86/intel: Enable arch-PEBS based SIMD/eGPRs/SSP sampling

On 4/25/2026 11:08 AM, [email protected] wrote:
> Sashiko AI review found 2 potential issue(s):
> - [Critical] Leaking stale per-CPU FPU/extended register state to user space when a PEBS event triggers in kernel mode and requests PERF_SAMPLE_REGS_USER.
> - [High] Incorrect bitwise AND with a raw u64 bitmask in `intel_pebs_support_regs` permanently disables large PEBS for XMM registers on arch-PEBS platforms.
> --
>
> commit 7ad07fe08e2e0b394952e6f0ccf5f040f99870e3
> Author: Dapeng Mi <[email protected]>
>
> perf/x86/intel: Enable arch-PEBS based SIMD/eGPRs/SSP sampling
>
> This patch enables arch-PEBS based SIMD/eGPRs/SSP registers sampling
> using the XSAVE-Enabled Registers (XER) group.
>
>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>> index 3d64f28b7e9ac..b280a39f68bca 100644
>> --- a/arch/x86/events/intel/core.c
>> +++ b/arch/x86/events/intel/core.c
> [ ... ]
>
>> @@ -4416,6 +4431,34 @@ static void intel_pebs_aliases_skl(struct perf_event *event)
>>  	return intel_pebs_aliases_precdist(event);
>>  }
>>  
>> +static inline bool intel_pebs_support_regs(struct perf_event *event, u64 regs)
>> +{
>> +	struct arch_pebs_cap cap = hybrid(event->pmu, arch_pebs_cap);
>> +	int pebs_format = x86_pmu.intel_cap.pebs_format;
>> +	bool supported = true;
>> +
>> +	/* SSP */
>> +	if (regs & PEBS_DATACFG_GP)
>> +		supported &= x86_pmu.arch_pebs && (ARCH_PEBS_GPR & cap.caps);
>> +	if (regs & PEBS_DATACFG_XMMS) {
>> +		supported &= x86_pmu.arch_pebs ?
>> +			     ARCH_PEBS_VECR_XMM & cap.caps :
>> +			     pebs_format > 3 && x86_pmu.intel_cap.pebs_baseline;
>> +	}
> Will this permanently disable large PEBS for XMM registers?
>
> Because supported is a boolean (initially 1), ARCH_PEBS_VECR_XMM & cap.caps
> evaluates to BIT_ULL(49). The bitwise AND operation 1 &= 0x2000000000000ULL
> strictly yields 0.
>
> Should this bitmask be normalized to a boolean value, perhaps using
> !!(ARCH_PEBS_VECR_XMM & cap.caps) inside the ternary operator?

Yes, it's a bug. would fix it in next version.


>
> [ ... ]
>
>> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
>> index 560f178d7ac92..61d72c58f9a25 100644
>> --- a/arch/x86/events/intel/ds.c
>> +++ b/arch/x86/events/intel/ds.c
> [ ... ]
>
>> @@ -2708,15 +2719,69 @@ static void setup_arch_pebs_sample_data(struct perf_event *event,
>>  					   meminfo->tsx_tuning, ax);
>>  	}
>>  
>> -	if (header->xmm) {
>> +	if (header->xmm || header->ymmh || header->egpr ||
>> +	    header->opmask || header->zmmh || header->h16zmm) {
>> +		struct arch_pebs_xer_header *xer_header = next_record;
>>  		struct pebs_xmm *xmm;
>> +		struct ymmh_struct *ymmh;
>> +		struct avx_512_zmm_uppers_state *zmmh;
>> +		struct avx_512_hi16_state *h16zmm;
>> +		struct avx_512_opmask_state *opmask;
>> +		struct apx_state *egpr;
>>  
>>  		next_record += sizeof(struct arch_pebs_xer_header);
>>  
>> -		ignore_mask |= XFEATURE_MASK_SSE;
>> -		xmm = next_record;
>> -		perf_regs->xmm_regs = xmm->xmm;
>> -		next_record = xmm + 1;
>> +		if (header->xmm) {
>> +			ignore_mask |= XFEATURE_MASK_SSE;
>> +			xmm = next_record;
>> +			/*
>> +			 * Only output XMM regs to user space when arch-PEBS
>> +			 * really writes data into xstate area.
>> +			 */
>> +			if (xer_header->xstate & XFEATURE_MASK_SSE)
>> +				perf_regs->xmm_regs = xmm->xmm;
>> +			next_record = xmm + 1;
>> +		}
> Is it possible for this code to leak stale per-CPU FPU/extended register
> state to user space?
>
> When a PEBS event triggers in kernel mode and requests PERF_SAMPLE_REGS_USER,
> this code processes the PEBS data and sets the corresponding bits
> (e.g., XFEATURE_MASK_SSE) in ignore_mask. It then calls
> x86_pmu_setup_regs_data(), which assigns data->regs_user.regs to the
> static per-CPU x86_user_regs.
>
> If x86_user_regs is never fully cleared, its extended register pointers
> (like xmm_regs) still point to the per-CPU ext_regs_buf from prior samples.
> Because XFEATURE_MASK_SSE is excluded via ignore_mask, xsaves_nmi() and
> x86_pmu_update_xregs() skip updating x86_user_regs.xmm_regs.
>
> Consequently, the xmm_regs pointer remains pointing to the stale data in
> ext_regs_buf. When the perf core outputs the user registers, could it
> read the stale FPU state from ext_regs_buf and leak it across processes?

Yes, it's a known issue. Would be fixed in next version.

Thanks.


>