Re: [PATCH] drivers/perf: thunderx2_pmu: add error handling

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing unregistration on error paths during PMU addition and device probe leads to Use-After-Free of PMU and hotplug structures.
--

commit bfe9572d37443a1d40e13a34014a4451ab6d0e17
Author: Griffin Kroah-Hartman <griffin@kroah.com>

drivers/perf: thunderx2_pmu: add error handling

This commit adds missing error checks for devm_kasprintf() in
tx2_uncore_pmu_register() and tx2_uncore_pmu_init_dev(). It also centralizes
the error cleanup in tx2_uncore_pmu_init_dev() using a goto err pattern.

> diff --git a/drivers/perf/thunderx2_pmu.c b/drivers/perf/thunderx2_pmu.c
> index 6ed4707bd6bb5..1a6aa94a3efb9 100644
> --- a/drivers/perf/thunderx2_pmu.c
> +++ b/drivers/perf/thunderx2_pmu.c
> @@ -738,6 +738,8 @@ static int tx2_uncore_pmu_register(
>  
>  	tx2_pmu->pmu.name = devm_kasprintf(dev, GFP_KERNEL,
>  			"%s", name);
> +	if (!tx2_pmu->pmu.name)
> +		return -ENOMEM;
>  
>  	return perf_pmu_register(&tx2_pmu->pmu, tx2_pmu->pmu.name, -1);
>  }

[ ... ]

> @@ -866,17 +872,21 @@ static struct tx2_uncore_pmu *tx2_uncore_pmu_init_dev(struct device *dev,
>  		tx2_pmu->attr_groups = ccpi2_pmu_attr_groups;
>  		tx2_pmu->name = devm_kasprintf(dev, GFP_KERNEL,
>  				"uncore_ccpi2_%d", tx2_pmu->node);
> +		if (!tx2_pmu->name)
> +			goto err;
>  		tx2_pmu->init_cntr_base = init_cntr_base_ccpi2;
>  		tx2_pmu->start_event = uncore_start_event_ccpi2;
>  		tx2_pmu->stop_event = uncore_stop_event_ccpi2;
>  		tx2_pmu->hrtimer_callback = NULL;
>  		break;
>  	case PMU_TYPE_INVALID:
> -		devm_kfree(dev, tx2_pmu);
> -		return NULL;
> +		goto err;
>  	}
>  
>  	return tx2_pmu;
> +err:
> +	devm_kfree(dev, tx2_pmu);
> +	return NULL;
>  }

[Severity: High]
This is a pre-existing issue, but looking at how error paths are handled,
could these new error returns lead to a use-after-free in the perf core or
CPU hotplug core?

If cpuhp_state_add_instance() fails in tx2_uncore_pmu_add_dev(), it returns
an error without calling perf_pmu_unregister() for the PMU that was just
registered:

	ret = tx2_uncore_pmu_register(tx2_pmu);
	if (ret) {
...
	}

	ret = cpuhp_state_add_instance(
			CPUHP_AP_PERF_ARM_CAVIUM_TX2_UNCORE_ONLINE,
			&tx2_pmu->hpnode);
	if (ret) {
		dev_err(tx2_pmu->dev, "Error %d registering hotplug", ret);
		return ret;
	}

Additionally, if devm_kasprintf() fails and returns NULL here,
tx2_uncore_pmu_add() will return AE_ERROR. This causes acpi_walk_namespace()
in tx2_uncore_probe() to fail and abort the probe:

	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
				     tx2_uncore_pmu_add,
				     NULL, dev, NULL);
	if (ACPI_FAILURE(status)) {
		dev_err(dev, "failed to probe PMU devices\n");
		return_ACPI_STATUS(status);
	}

Since tx2_uncore_probe() aborts with an error, the driver core will free
all devm_ allocated memory. Are the PMUs that were successfully added in
previous iterations of acpi_walk_namespace() unregistered before the memory
is freed, or will this leave dangling pointers in the core kernel subsystems?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/2026070635-feel-joyride-6d5b@gregkh?part=1