perf/arm_cspmu: Miscellaneous improvements

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

New issues:
- [High] Platform device ID collision causing APMT parsing abort and subsequent UAF.

Pre-existing issues:
- [High] Missing rollback on failure leaves previously registered platform devices holding dangling pointers to an unmapped ACPI table.
- [High] Missing bounds check on apmt_node->length allows out-of-bounds memory read.
- [Medium] GSI IRQ mapping is leaked on platform device registration error paths.
--

--- Patch [1]: [PATCH v2 1/4] ACPI/APMT: Use stable device ID --- commit 895c0eb864a4218fbb99620f4fa9efb2dc4e091c
Author: Robin Murphy <[email protected]>

ACPI/APMT: Use stable device ID

The APMT node format includes a unique identifier, so we can use this as the platform device ID to give userspace stable and identifiable device names.

Quoted 12 lines

drivers/acpi/arm64/apmt.c +1 -1

index bb010f6164e52..91fcdd289e637 100644
@@ -76,10 +76,12 @@ static int __init apmt_add_platform_device(struct acpi_apmt_node *node,
  					   struct fwnode_handle *fwnode)
  {
  	struct platform_device *pdev;
-	int ret, count;
+	int ret, count, uid = node->id & INT_MAX;
  	struct resource res[DEV_MAX_RESOURCE_COUNT];
 

drivers/acpi/arm64/apmt.c static int __init apmt_add_platform_device(struct acpi_apmt_node *node, · line 84

Quoted 4 lines

drivers/acpi/arm64/apmt.c +3 -1

-	pdev = platform_device_alloc(DEV_NAME, PLATFORM_DEVID_AUTO);
+	if (uid != node->id)
+		pr_warn("Unexpectedly large UID 0x%x, truncated to 0x%x\n", node->id, uid);
+	pdev = platform_device_alloc(DEV_NAME, uid);

[Severity: High]
Does this newly introduced truncation create a risk of ID collisions if two unique 32-bit APMT node IDs differ only in the sign bit?

The APMT node format includes a unique identifier, so we can use this as the platform device ID to give userspace stable and identifiable device names, rather than auto numbering dependent on how the table is parsed.

Cc Lorenzo Pieralisi <[email protected]>

Cc Hanjun Guo <[email protected]>

Cc Sudeep Holla <[email protected]>

Reviewed-by Ilkka Koskinen <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
v2: Handle and warn about unexpected signed overflow

---
  drivers/acpi/arm64/apmt.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

drivers/acpi/arm64/apmt.c +0 -0

index bb010f6164e5..91fcdd289e63 100644
@@ -76,10 +76,12 @@ static int __init apmt_add_platform_device(struct acpi_apmt_node *node,
  					   struct fwnode_handle *fwnode)
  {
  	struct platform_device *pdev;

On ACPI systems, it has not actually been possible for userspace to reliably tell which PMU corresponds to which APMT entry for types other than "ACPI device" - the evidence trail only leads from the arbitrarily-numbered PMU device to its arbitrarily-numbered parent platform device that has no distinguishing features either.

While we've now improved the platform device creation to associate the actual APMT unique ID, we may as well also tweak the PMU devices to substitute the arbitrary number with a different arbitrary number that might be more directly meaningful based on the APMT definitions.

We don't have an equivalent for Devicetree, but in that case the platform devices are at least identifiable via their sysfs-visible of_node.

Signed-off-by Robin Murphy <[email protected]>

---
v2: Actually use the instance-based ID where different from node->id

---
drivers/perf/arm_cspmu/arm_cspmu.c | 35 +++++++++++++++++-------------

1 file changed, 20 insertions(+), 15 deletions(-)

drivers/perf/arm_cspmu/arm_cspmu.c +20 -15

index 80fb314d5135..8c2dda17d73e 100644
@@ -250,38 +250,43 @@ static const char *arm_cspmu_get_name(const struct arm_cspmu *cspmu)
 	struct device *dev;
 	struct acpi_apmt_node *apmt_node;
 	u8 pmu_type;
-	char *name;
 	char acpi_hid_string[ACPI_ID_LEN] = { 0 };
-	static atomic_t pmu_idx[ACPI_APMT_NODE_TYPE_COUNT] = { 0 };
+	static atomic_t pmu_idx;
+	u32 id;
 
 	dev = cspmu->dev;
 	apmt_node = arm_cspmu_apmt_node(dev);
 	if (!apmt_node)
 		return devm_kasprintf(dev, GFP_KERNEL, PMUNAME "_%u",
-				      atomic_fetch_inc(&pmu_idx[0]));
+				      atomic_fetch_inc(&pmu_idx));
 
 	pmu_type = apmt_node->type;
-
-	if (pmu_type >= ACPI_APMT_NODE_TYPE_COUNT) {
+	switch (pmu_type) {
+	default:
 		dev_err(dev, "unsupported PMU type-%u\n", pmu_type);
 		return NULL;
-	}
-
-	if (pmu_type == ACPI_APMT_NODE_TYPE_ACPI) {
+	case ACPI_APMT_NODE_TYPE_ACPI:
 		memcpy(acpi_hid_string,
 			&apmt_node->inst_primary,
 			sizeof(apmt_node->inst_primary));
-		name = devm_kasprintf(dev, GFP_KERNEL, "%s_%s_%s_%u", PMUNAME,
+		return devm_kasprintf(dev, GFP_KERNEL, "%s_%s_%s_%u", PMUNAME,
 				      arm_cspmu_type_str[pmu_type],
 				      acpi_hid_string,
 				      apmt_node->inst_secondary);
-	} else {
-		name = devm_kasprintf(dev, GFP_KERNEL, "%s_%s_%d", PMUNAME,
-				      arm_cspmu_type_str[pmu_type],
-				      atomic_fetch_inc(&pmu_idx[pmu_type]));
-	}
+	case ACPI_APMT_NODE_TYPE_MC:
+		id = apmt_node->id;
+		break;
+	case ACPI_APMT_NODE_TYPE_SMMU:
+	case ACPI_APMT_NODE_TYPE_PCIE_ROOT:
+		id = apmt_node->inst_primary;
+		break;
+	case ACPI_APMT_NODE_TYPE_CACHE:
+		id = apmt_node->inst_secondary;
+		break;
+	};
 
-	return name;
+	return devm_kasprintf(dev, GFP_KERNEL, "%s_%s_%u", PMUNAME,
+			      arm_cspmu_type_str[pmu_type], id);
 }
 
 static ssize_t arm_cspmu_cpumask_show(struct device *dev,

Signature · 1 line

2.54.0.dirty

When waiting for a sub-module to register, we return a bare -EPROBE_DEFER that ends up showing the end user:

platform arm-cs-arch-pmu.1: deferred probe pending (no reason)

wherein it's not necessarily clear that they might need to take some action to ensure the appropriate module is available to load. Let's use dev_err_probe() here so we can show exactly what we're waiting for.

Similarly, in the case where something's gone horribly wrong with an already-registered module, we can use dev_WARN() to standardise the device/driver attribution rather than just open-coding "arm_cspmu".

Reviewed-by Ilkka Koskinen <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
v2: No change

---
 drivers/perf/arm_cspmu/arm_cspmu.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/perf/arm_cspmu/arm_cspmu.c +4 -2

index 8c2dda17d73e..caad9a36651d 100644
@@ -437,13 +437,15 @@ static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
 				if (ret)
 					module_put(match->module);
 			} else {
-				WARN(1, "arm_cspmu failed to get module: %s\n",
+				dev_WARN(cspmu->dev, "Failed to get module: %s\n",
 					match->module_name);
 				ret = -EINVAL;
 			}
 		} else {
 			request_module_nowait(match->module_name);
-			ret = -EPROBE_DEFER;
+			ret = dev_err_probe(cspmu->dev, -EPROBE_DEFER,
+					    "Waiting for module %s to load\n",
+					    match->module_name);
 		}
 
 		mutex_unlock(&arm_cspmu_lock);

Quoted 9 lines · On Thu, 16 Jul 2026 15:56:36 +0100 Robin Murphy <[email protected]> wrote:

When waiting for a sub-module to register, we return a bare -EPROBE_DEFER that ends up showing the end user:

platform arm-cs-arch-pmu.1: deferred probe pending (no reason)

wherein it's not necessarily clear that they might need to take some action to ensure the appropriate module is available to load. Let's use dev_err_probe() here so we can show exactly what we're waiting for.

Quoted 6 lines

Similarly, in the case where something's gone horribly wrong with an already-registered module, we can use dev_WARN() to standardise the device/driver attribution rather than just open-coding "arm_cspmu".

Reviewed-by Ilkka Koskinen <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

Hi Robin,

Maybe sprinkle some cleanup.h magic and use a guard(mutex)() and direct returns. Simplifies this function that you are touching anyway.

Looking more broadly it'd either take a scoped_guard() or some refactoring if you want to go that way cross the file.

Jonathan

Quoted 29 lines

---
v2: No change

---
  drivers/perf/arm_cspmu/arm_cspmu.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

drivers/perf/arm_cspmu/arm_cspmu.c +4 -2

index 8c2dda17d73e..caad9a36651d 100644
@@ -437,13 +437,15 @@ static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
  				if (ret)
  					module_put(match->module);
  			} else {
-				WARN(1, "arm_cspmu failed to get module: %s\n",
+				dev_WARN(cspmu->dev, "Failed to get module: %s\n",
  					match->module_name);
  				ret = -EINVAL;
  			}
  		} else {
  			request_module_nowait(match->module_name);
-			ret = -EPROBE_DEFER;
+			ret = dev_err_probe(cspmu->dev, -EPROBE_DEFER,
+					    "Waiting for module %s to load\n",
+					    match->module_name);
  		}
 
  		mutex_unlock(&arm_cspmu_lock);

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

New issues:
- [High] Unconditional swallowing of arm_cspmu_request_irq() error codes (such as -EPROBE_DEFER and -ENOMEM) for PMUs with >= 64-bit counters.

Pre-existing issues:
- [High] Incorrect ordering between CPU hotplug state management and PMU registration causes Use-After-Free during teardown.
- [High] Missing cleanup of CPU hotplug state if platform_driver_register() fails in module init.
- [High] Failed driver_attach() in arm_cspmu_impl_register() leaves dangling pointers in the impl_match array.
- [High] Stack-allocated bitmap cast to u32 * leaves memory uninitialized on Big Endian systems, causing reads of uninitialized stack memory. --

--- Patch [4]: [PATCH v2 4/4] perf/arm_cspmu: Make IRQ more optional --- commit e7afdbca83ffd562889abdf4a15a47f1af9fc395
Author: Robin Murphy <[email protected]>

perf/arm_cspmu: Make IRQ more optional

This commit allows the driver to continue probing even if it fails to request an IRQ, provided the PMU has 64-bit counters. It assumes that 64-bit counters will not overflow in a realistic timeframe.

Quoted 7 lines

drivers/perf/arm_cspmu/arm_cspmu.c +0 -0

index 385d3e34694f2..007038aa7e9a6 100644
@@ -1256,8 +1256,11 @@ static int arm_cspmu_device_probe(struct platform_device *pdev)
  		return ret;
 

drivers/perf/arm_cspmu/arm_cspmu.c static int arm_cspmu_device_probe(struct platform_device *pdev) · line 1261

Quoted 6 lines

drivers/perf/arm_cspmu/arm_cspmu.c +3 -2

  	ret = arm_cspmu_request_irq(cspmu);
-	if (ret)
-		return ret;
+	if (ret) {
+		if (counter_size(cspmu) < 64)
+			return ret;

[Severity: High]
Does this unintentionally swallow error codes like -EPROBE_DEFER or -ENOMEM for 64-bit PMUs?

These are a few quality-of-life improvements for CoreSight PMU that I've accumulated based on things which have come up in practical usage. There's no real dependency between any of the individual patches, it just felt right to send them as a series.

v1: https://lore.kernel.org/linux-perf-users/[email protected]/
v2: Fix valid nits from Sashiko review, add Ilkka's R-b.

[...]