Re: [PATCH v3 4/7] perf/x86/uncore: Add L3 PMU support for Hygon family 18h model 6h

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Registered PMUs are not unregistered if `cpuhp_setup_state()` fails during module initialization, leading to a Use-After-Free.
--

commit 98d69c16b51701dce640f5ac92cee0daa35f2cfb
Author: Fu Hao <fuhao@open-hieco.net>

perf/x86/uncore: Add L3 PMU support for Hygon family 18h model 6h

This patch adds L3 PMU support for the Hygon family 18h model 6h processor
by adjusting the slicemask and threadmask in the uncore driver.

> diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
> index 9293ce50574da..21f00edd3caa6 100644
> --- a/arch/x86/events/amd/uncore.c
> +++ b/arch/x86/events/amd/uncore.c

[ ... ]

> @@ -877,7 +910,8 @@ int amd_uncore_l3_ctx_init(struct amd_uncore *uncore, unsigned int cpu)
>  	pmu->rdpmc_base = RDPMC_BASE_LLC;
>  	pmu->group = amd_uncore_ctx_gid(uncore, cpu);
>  
> -	if (boot_cpu_data.x86 >= 0x17) {
> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
> +	    boot_cpu_data.x86 >= 0x17) {
>  		*l3_attr++ = &format_attr_event8.attr;
>  		*l3_attr++ = &format_attr_umask8.attr;

[ ... ]

> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON &&
> +	    boot_cpu_data.x86 == 0x18) {
> +		*l3_attr++ = &format_attr_event8.attr;
> +		*l3_attr++ = &format_attr_umask8.attr;
> +		if (boot_cpu_data.x86_model >= 0x6 && boot_cpu_data.x86_model <= 0xf) {
> +			*l3_attr++ = &format_attr_threadmask32.attr;
> +			pmu->pmu.attr_update = hygon_uncore_l3_attr_update;
> +		} else {
> +			*l3_attr++ = &format_attr_threadmask8.attr;
> +		}
> +	}
> +
>  	if (perf_pmu_register(&pmu->pmu, pmu->pmu.name, -1)) {
>  		free_percpu(pmu->ctx);
>  		pmu->ctx = NULL;

[Severity: High]
This is a pre-existing issue, but registering the PMU here exposes a teardown
issue in the module initialization error path.

If cpuhp_setup_state() fails for the online state in amd_uncore_init(), the
error path jumps to fail_start and removes the hotplug states. This triggers
the amd_uncore_cpu_dead() teardown callback, which correctly frees the
per-CPU context by calling amd_uncore_ctx_free().

However, the error path does not unregister the PMU or free the uncore->pmus
array:

arch/x86/events/amd/uncore.c:amd_uncore_init() {
    ...
fail:
	for (i = 0; i < UNCORE_TYPE_MAX; i++) {
		uncore = &uncores[i];
		if (uncore->info) {
			free_percpu(uncore->info);
			uncore->info = NULL;
		}
	}

	return ret;
}

Since the initialization failed, the module will be unloaded, but the perf
subsystem retains references to the registered PMUs.

Could this lead to a use-after-free if the perf subsystem later accesses
these dangling PMUs (for instance, via perf stat) after the module memory
has been freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1781523812.git.fuhao@open-hieco.net?part=4