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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing PMU unregistration in the module initialization error path leads to a fatal Use-After-Free.
- [High] Integer truncation of the `num_pmcs` bitfield allows an out-of-bounds array write.
- [Medium] Uninitialized stack memory is copied to long-lived per-CPU data structures.
- [Low] Use of non-existent macro `kzalloc_obj` causes a build failure.
- [Low] Linking `uncore_common.o` into both `amd-uncore` and `hygon-uncore` causes multiple definition linker errors.
--

commit a3e675d32729bead13a45d2e64f57244f9040366
Author: Qi Liu <liuqi@hygon.cn>
Subject: perf/x86/amd/uncore: Add Hygon uncore PMU support

Add uncore PMU support for Hygon processors based on the shared AMD-family
uncore framework.

> diff --git a/arch/x86/events/amd/Makefile b/arch/x86/events/amd/Makefile
> index f951ae64ee361..32bf7aae23681 100644
> --- a/arch/x86/events/amd/Makefile
> +++ b/arch/x86/events/amd/Makefile
> @@ -5,6 +5,8 @@ obj-$(CONFIG_PERF_EVENTS_AMD_POWER)	+= power.o
>  obj-$(CONFIG_X86_LOCAL_APIC)		+= ibs.o
>  obj-$(CONFIG_PERF_EVENTS_AMD_UNCORE)	+= amd-uncore.o
>  amd-uncore-objs				:= uncore_common.o uncore.o
> +obj-$(CONFIG_PERF_EVENTS_HYGON_UNCORE)	+= hygon-uncore.o
> +hygon-uncore-objs			:= uncore_common.o hygon_uncore.o

If a kernel is configured with both CONFIG_PERF_EVENTS_AMD_UNCORE=y and
CONFIG_PERF_EVENTS_HYGON_UNCORE=y, won't Kbuild link uncore_common.o twice
into the built-in.a archive? Will this result in fatal 'multiple definition'
linker errors for all globally visible symbols defined in uncore_common.c
(e.g., uncore_common_event_init)?

> diff --git a/arch/x86/events/amd/hygon_uncore.c b/arch/x86/events/amd/hygon_uncore.c
> new file mode 100644
> index 0000000000000..133b6b1923de9
> --- /dev/null
> +++ b/arch/x86/events/amd/hygon_uncore.c
[ ... ]
> +static int hygon_uncore_df_ctx_init(struct uncore_common *uncore,
> +				    unsigned int cpu)
> +{
> +	struct attribute *df_attr;
> +	struct uncore_common_pmu *pmu;
> +	int num_counters;
> +
> +	if (uncore->init_done)
> +		return uncore_common_ctx_init(uncore, cpu);
> +
> +	num_counters = uncore_common_ctx_num_pmcs(uncore, cpu);
> +	if (!num_counters)
> +		goto done;
> +
> +	uncore->pmus = kzalloc_obj(*uncore->pmus);

Will this cause a build failure? The macro kzalloc_obj does not exist anywhere
in the kernel codebase. Should this be using kzalloc() instead?

(This issue also appears in hygon_uncore_df_iod_ctx_init()).

[ ... ]
> +static void hygon_uncore_df_ctx_scan(struct uncore_common *uncore,
> +				     unsigned int cpu)
> +{
> +	unsigned int eax, ebx, ecx, edx;
> +	union uncore_common_info info;

Does this code copy uninitialized stack memory to long-lived per-CPU data
structures?

Because info is not zero-initialized, and the code manually assigns several
bitfields but omits others (like private here and aux_data in the IOD scan
below), the omitted bitfields will retain uninitialized stack garbage.

> +
> +	if (!boot_cpu_has(X86_FEATURE_PERFCTR_NB))
> +		return;
> +
> +	info.split.gid = 0;
> +	info.split.aux_data = 0;
> +	info.split.num_pmcs = NUM_COUNTERS_DF;
> +
> +	cpuid(0x8000001e, &eax, &ebx, &ecx, &edx);
> +	info.split.cid = ecx & 0xff;
> +
> +	*per_cpu_ptr(uncore->info, cpu) = info;

Since the entire 64-bit union is stored into per-CPU memory here, should info
be explicitly initialized?

> +}
> +
> +static void hygon_uncore_df_iod_ctx_scan(struct uncore_common *uncore,
> +					 unsigned int cpu)
> +{
> +	int num_packages, iods_per_package;
> +	union uncore_common_info info;
> +
> +	if (!boot_cpu_has(X86_FEATURE_PERFCTR_NB))
> +		return;
> +
> +	if (boot_cpu_data.x86_model < 0x4 || boot_cpu_data.x86_model == 0x6)
> +		return;
> +
> +	num_packages = topology_max_packages();
> +	iods_per_package = amd_nb_num() / num_packages - topology_max_dies_per_package();
> +	if (iods_per_package <= 0)
> +		return;
> +
> +	info.split.cid = topology_physical_package_id(cpu);
> +	info.split.gid = 0;
> +	info.split.private = iods_per_package;
> +	info.split.num_pmcs = NUM_COUNTERS_DF * iods_per_package;

Could this truncation allow an out-of-bounds array write later during event
addition?

num_pmcs is an 8-bit bitfield. If the hardware topology presents a large
number of nodes (e.g., 65 IODs), NUM_COUNTERS_DF * iods_per_package yields
260, which silently truncates to 4.

Consequently, pmu->num_counters would be set to 4, and uncore_common_ctx_init()
would allocate a ctx->events array of only 4 elements.

During hygon_uncore_df_iod_add():
    iod_idx = event->attr.config1;
    for (i = iod_idx * NUM_COUNTERS_DF; ...
        if (try_cmpxchg(&ctx->events[i], &tmp, event)) {

If event->attr.config1 is 64 (which passes validation because
info.split.private is correctly 65), the array index here would be 256. Could
try_cmpxchg() write past the end of the 4-element ctx->events array?

[ ... ]
> +static int __init hygon_uncore_init(void)
> +{
> +	struct uncore_common *uncore;
> +	int ret = -ENODEV;
> +	int i;
> +
[ ... ]
> +	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);
> +fail_prep:
> +	cpuhp_remove_state(CPUHP_PERF_X86_HYGON_UNCORE_PREP);
> +fail:
> +	for (i = 0; i < HYGON_UNCORE_TYPE_MAX; i++) {
> +		uncore = &hygon_uncores[i];
> +
> +		if (uncore->info) {
> +			free_percpu(uncore->info);
> +			uncore->info = NULL;
> +		}
> +	}
> +
> +	return ret;
> +}

Does the module initialization error path have a regression where it fails to
unregister any PMUs that were successfully registered via perf_pmu_register()?

If cpuhp_setup_state(CPUHP_AP_PERF_X86_HYGON_UNCORE_ONLINE) fails (for example,
because uncore_common_ctx_init() returns -ENOMEM during DF_IOD setup), the
initialization aborts and jumps to fail_start. This error path removes CPU
hotplug states and frees uncore->info, but fails to unregister the PMU (if
the DF PMU succeeded earlier).

Since module initialization returns an error, the module is unloaded and its
memory is freed. The dangling PMU remains active in the core perf subsystem.
Would subsequent userspace operations on this PMU dereference function pointers
(like .event_init) pointing to the unloaded module's memory and cause a
use-after-free panic?

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