Re: [Patch v9 11/24] perf/x86: Enable XMM register sampling for non-PEBS events

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

On 7/6/2026 10:34 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] reserve_ext_regs_buffers() fails to propagate memory allocation errors and improperly uses WARN_ON_ONCE, leading to a silent feature failure and possible kernel panics under memory pressure.
> --
>
> commit 8262cdbae2d650c85cf4924888ecf8b4f85b98de
> Author: Dapeng Mi <dapeng1.mi@linux.intel.com>
>
> perf/x86: Enable XMM register sampling for non-PEBS events
>
> This commit extends XMM register sampling to non-PEBS events using the xsaves
> instruction. It allocates a per-CPU ext_regs_buf to store SIMD and other
> registers. For PEBS events, data is retrieved directly from PEBS records.
>
>> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
>> index d83ea02e24577..3e52610510cdc 100644
>> --- a/arch/x86/events/core.c
>> +++ b/arch/x86/events/core.c
> [ ... ]
>> @@ -410,6 +410,56 @@ set_ext_hw_attr(struct hw_perf_event *hwc, struct perf_event *event)
>> +static void reserve_ext_regs_buffers(void)
>> +{
>> +	bool compacted = cpu_feature_enabled(X86_FEATURE_XCOMPACTED);
>> +	unsigned int size;
>> +	int cpu;
>> +
>> +	if (!x86_pmu.ext_regs_mask)
>> +		return;
>> +
>> +	/* +64 bytes for the 64 bytes alignment request of xsave area. */
>> +	size = xstate_calculate_size(x86_pmu.ext_regs_mask, compacted) + 64;
>> +
>> +	for_each_possible_cpu(cpu) {
>> +		per_cpu(ext_regs_buf, cpu) = kzalloc_node(size, GFP_KERNEL,
>> +							  cpu_to_node(cpu));
>> +		if (WARN_ON_ONCE(!per_cpu(ext_regs_buf, cpu)))
>> +			goto err;
>> +	}
>> +
>> +	return;
>> +
>> +err:
>> +	release_ext_regs_buffers();
>> +}
> [ ... ]
>>  int x86_reserve_hardware(void)
>>  {
>>  	int err = 0;
>>  
>>  	if (!atomic_inc_not_zero(&pmc_refcount)) {
>>  		mutex_lock(&pmc_reserve_mutex);
>>  		if (atomic_read(&pmc_refcount) == 0) {
>>  			if (!reserve_pmc_hardware()) {
>>  				err = -EBUSY;
>>  			} else {
>>  				reserve_ds_buffers();
>>  				reserve_lbr_buffers();
>> +				reserve_ext_regs_buffers();
>>  			}
>>  		}
>>  		if (!err)
> [Severity: High]
> Does this code swallow the memory allocation failure?
>
> Since reserve_ext_regs_buffers() returns void, x86_reserve_hardware() 
> appears unaware if kzalloc_node() fails under memory pressure. The hardware 
> reservation will succeed anyway, and the perf event creation is permitted
> even though the backing buffers do not exist.
>
> [ ... ]
>> +static void x86_pmu_sample_xregs(struct perf_event *event,
>> +				 struct perf_sample_data *data,
>> +				 u64 ignore_mask)
>> +{
>> +	struct xregs_state *xsave = get_ext_regs_buf(smp_processor_id());
>> +	u64 sample_type = event->attr.sample_type;
>> +	struct x86_perf_regs *perf_regs;
>> +	u64 intr_mask = 0;
>> +	u64 mask = 0;
>> +
>> +	if (WARN_ON_ONCE(!xsave) || !in_nmi())
>> +		return;
> [Severity: High]
> Can this path be reached with a NULL xsave buffer during a PMI if the earlier
> allocation failed?
>
> If an unprivileged user creates an event requesting extended registers during
> memory pressure, and the allocation in reserve_ext_regs_buffers() fails,
> the event is successfully created. When the PMI hits, this WARN_ON_ONCE in
> NMI context will trigger.
>
> This could panic the system if panic_on_warn is set, or otherwise silently
> drop the requested register data. Should reserve_ext_regs_buffers() return
> an error code so x86_reserve_hardware() can fail the event creation instead?

IMO, it's good enough to use WARN_ON_ONCE() to warn the allocation error.
Propagating the allocation error would need to add much complicated
fallback code since there are lots of allocations, it seems unworthy to
make the code be so complicated.

Thanks.


>