Re: [PATCH v3 1/2] perf/x86/amd/uncore: Add Hygon uncore PMU support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Module initialization error path fails to unregister PMUs and leaks per-CPU arrays, leading to a Use-After-Free.
- [Medium] The CPU online callback aborts PMU initialization on the first failure, leaving remaining independent PMUs uninitialized.
- [Medium] The patch removes L3 cache uncore PMU support for Hygon processors without providing a replacement.
- [Medium] The `update_interval` module parameter lacks bounds checking, allowing a value of 0 to cause a CPU-locking interrupt storm.
--

--- Patch [1]: [PATCH v3 1/2] perf/x86/amd/uncore: Add Hygon uncore PMU support ---
commit 451a8d6c91c9ade8b2dbc942f864d5c36dc872c9
Author: Qi Liu <liuqi@hygon.cn>
Subject: perf/x86/amd/uncore: Add Hygon uncore PMU support

This commit adds a new uncore PMU driver specifically for Hygon Family 18h
processors, supporting Data Fabric and IO Die counters. It also removes the
Hygon-specific checks from the AMD uncore driver to keep the vendor
implementations separate.

> diff --git a/arch/x86/events/amd/hygon_uncore.c b/arch/x86/events/amd/hygon_uncore.c
> new file mode 100644
> --- /dev/null
> +++ b/arch/x86/events/amd/hygon_uncore.c
> @@ -0,0 +1,863 @@

[ ... ]

> +/* Interval for hrtimer, defaults to 60000 milliseconds */
> +static unsigned int update_interval = 60 * MSEC_PER_SEC;
> +module_param(update_interval, uint, 0444);

[Severity: Medium]
Could setting this to 0 cause an interrupt storm? If a privileged user loads
the module with update_interval=0, hrtimer_duration becomes 0. When this is
passed to hrtimer_forward_now(), it is clamped to the minimum resolution,
causing the high-resolution timer to fire continuously in a loop. Should
there be a minimum bounds check here to prevent a CPU lockup?

[ ... ]

> +static int hygon_uncore_cpu_online(unsigned int cpu)
> +{
> +	struct hygon_uncore *uncore;
> +	int i;
> +
> +	for (i = 0; i < HYGON_UNCORE_TYPE_MAX; i++) {
> +		uncore = &uncores[i];
> +		if (uncore->init(uncore, cpu))
> +			break;

[Severity: Medium]
Should this be a continue instead of a break? If the first PMU type (e.g.,
hygon_df) fails to initialize, this break aborts the loop, which prevents
any subsequent independent PMUs (like hygon_df_iod) from initializing on
this CPU. The same patch corrects this exact logic flaw in
amd_uncore_cpu_online(), but it seems to have been reintroduced here.

> +	}
> +
> +	return 0;
> +}

[ ... ]

> +fail_start:
> +	cpuhp_remove_state(CPUHP_AP_PERF_X86_HYGON_UNCORE_STARTING);
> +fail_prep:
> +	cpuhp_remove_state(CPUHP_PERF_X86_HYGON_UNCORE_PREP);
> +fail:
> +	hygon_uncore_info_cleanup();
> +	return ret;
> +}

[Severity: High]
Does this error path leak the registered PMUs? If cpuhp_setup_state()
fails for the ONLINE state, we remove the previous hotplug states but it
looks like hygon_uncore_pmu_cleanup() is never called.

Any PMUs successfully registered during the initial CPU online callbacks
would remain registered, and their per-CPU arrays would not be freed. When
the module unloads, could this lead to a use-after-free if the perf
subsystem tries to access the freed module memory?

[ ... ]

> diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
> index dbc00b6dd69ee..3f8e40abba736 100644
> --- a/arch/x86/events/amd/uncore.c
> +++ b/arch/x86/events/amd/uncore.c

[ ... ]

> @@ -1120,8 +1116,7 @@ static int __init amd_uncore_init(void)
>  	int ret = -ENODEV;
>  	int i;
>  
> -	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD &&
> -	    boot_cpu_data.x86_vendor != X86_VENDOR_HYGON)
> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
>  		return -ENODEV;

[Severity: Medium]
Does this intentionally drop L3 cache uncore support for Hygon? By
removing Hygon support here in amd_uncore.c, the previous L3 cache
counters are no longer available. Looking at the new hygon_uncore.c, it
only implements the Data Fabric and IO Die PMUs, meaning L3 PMU support
appears to be lost completely.

>  	if (!boot_cpu_has(X86_FEATURE_TOPOEXT))
>  		return -ENODEV;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708131315.13766-1-liuqi@hygon.cn?part=1