Re: [PATCH v2 3/3] perf/x86/amd/uncore: Add Hygon uncore PMU support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incorrect teardown order in the init error path causes NULL pointer dereferences and timer Use-After-Free.
--

commit 757cc017754a489c37240b97dc4192801ef594af
Author: Qi Liu <liuqi@hygon.cn>

perf/x86/amd/uncore: Add Hygon uncore PMU support

This patch adds uncore PMU support for Hygon processors based on the shared
AMD-family uncore framework. It reuses the common uncore infrastructure while
accommodating Hygon-specific differences like MSR base addresses and event
encodings.

> 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

[ ... ]

> +static int __init hygon_uncore_init(void)
> +{

[ ... ]

> +	ret = cpuhp_setup_state(CPUHP_AP_PERF_X86_HYGON_UNCORE_ONLINE,
> +				"perf/x86/hygon/uncore:online",
> +				hygon_uncore_cpu_online,
> +				hygon_uncore_cpu_down_prepare);
> +	if (ret)
> +		goto fail_start;
> +
> +	return 0;
> +
> +fail_start:
> +	cpuhp_remove_state(CPUHP_AP_PERF_X86_HYGON_UNCORE_STARTING);
> +	cpuhp_remove_state(CPUHP_PERF_X86_HYGON_UNCORE_PREP);
> +	hygon_uncore_pmus_unregister();

[Severity: High]
Is the teardown order inverted here?

If initialization successfully registers the PMU on the first CPU but fails on
a subsequent CPU, the error path jumps to fail_start.

If a user manages to open a perf event on this PMU during this small window,
removing the CPUHP_PERF_X86_HYGON_UNCORE_PREP state first will free the PMU
context and set it to NULL.

When hygon_uncore_pmus_unregister() is then called, it triggers
perf_pmu_unregister(), which attempts to detach the active event by calling
amd_uncore_common_stop().

Will amd_uncore_common_stop() then dereference the NULL context or cause a
timer use-after-free?

> +	goto fail;
> +fail_prep:
> +	cpuhp_remove_state(CPUHP_PERF_X86_HYGON_UNCORE_PREP);
> +fail:
> +	hygon_uncore_infos_free();
> +
> +	return ret;
> +}
> +
> +static void __exit hygon_uncore_exit(void)
> +{
> +	cpuhp_remove_state(CPUHP_AP_PERF_X86_HYGON_UNCORE_ONLINE);
> +	cpuhp_remove_state(CPUHP_AP_PERF_X86_HYGON_UNCORE_STARTING);
> +	cpuhp_remove_state(CPUHP_PERF_X86_HYGON_UNCORE_PREP);
> +
> +	hygon_uncore_pmus_unregister();

[Severity: High]
Does this exhibit the same inverted teardown order?

Although module refcounting usually protects the module exit path from executing
while events are active, shouldn't we unregister the PMUs before removing the
hotplug states to safely detach any resources and mirror the initialization
order?

> +	hygon_uncore_infos_free();
> +}

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