Re: [PATCH 6/8] perf/x86/intel: Validate return value of intel_pmu_init_hybrid()

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

On 6/5/2026 9:36 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] Unnecessary error handling and dead cleanup code for early boot memory allocation failure.
> --
>
> commit e6fc94eb7f63fa466e8e1aeaed9a0d18231a88dc
> Author: Dapeng Mi <dapeng1.mi@linux.intel.com>
>
> perf/x86/intel: Validate return value of intel_pmu_init_hybrid()
>
> This commit validates the return value of intel_pmu_init_hybrid() to prevent
> attempts to access the uninitialized x86_pmu.hybrid_pmu[] array due to
> allocation failure. It adds error paths that destroy the task_ctx_cache when
> an error occurs.
>
>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>> index ea3ab3050a3b1..efd9caa3502c5 100644
>> --- a/arch/x86/events/intel/core.c
>> +++ b/arch/x86/events/intel/core.c
>> @@ -8539,7 +8540,9 @@ __init int intel_pmu_init(void)
>>  		 *
>>  		 * Initialize the common PerfMon capabilities here.
>>  		 */
>> -		intel_pmu_init_hybrid(hybrid_big_small);
>> +		ret = intel_pmu_init_hybrid(hybrid_big_small);
>> +		if (ret < 0)
>> +			goto err;
> [Severity: Low]
> Is this error handling necessary for an early boot function?
>
> Since intel_pmu_init() in arch/x86/events/intel/core.c is marked with __init,
> it executes only during early boot. At this stage, if a physical memory
> allocation fails, the system typically cannot boot anyway. Handling memory
> allocation failures and returning -ENOMEM here is likely dead code.

Even it's in early boot, it's better not to trigger an invalid memory
access error which could cause misleading.


>
> [ ... ]
>
>> @@ -8885,6 +8900,10 @@ __init int intel_pmu_init(void)
>>  	intel_aux_output_init();
>>  
>>  	return 0;
>> +
>> +err:
>> +	kmem_cache_destroy(x86_get_pmu(smp_processor_id())->task_ctx_cache);
> [Severity: Low]
> Will this cleanup code in intel_pmu_init() actually free anything?
>
> If the allocation fails during intel_pmu_init_hybrid() and we jump to the err
> label, task_ctx_cache has not been allocated yet. It is only allocated later
> in the function via intel_pmu_lbr_init() or intel_pmu_arch_lbr_init().
>
> This guarantees task_ctx_cache is NULL when this error path is taken, meaning
> this call to kmem_cache_destroy() acts as a no-op.

No, intel_pmu_arch_lbr_init() which is called before
intel_pmu_init_hybrid() could create the LBR needed slab cache
"task_ctx_cache". If intel_pmu_init_hybrid() returns error, the created
slab cache needs to be destroyed.

Thanks.

>
>> +	return ret;
>>  }
>>