platform: fix reference leak on failed platform_device_register()

When platform_device_register() fails in digsy_mtc_eeprom_devices_init(), the embedded struct device has already been initialized by device_initialize() inside platform_device_register(). The failure path cleans up the software node but returns the error without dropping the device reference:

digsy_mtc_eeprom_devices_init()

    -> platform_device_register(&digsy_mtc_eeprom)
       -> device_initialize(&digsy_mtc_eeprom.dev)   /* kref = 1 */
       -> platform_device_add(&digsy_mtc_eeprom)     /* fails */
    <- returns error, kref still 1, reference leaked

Per platform_device_register() kernel-doc:

NOTE: _Never_ directly free @pdev after calling this function, even if it returned an error! Always use platform_device_put() to give up the reference initialised in this function instead.

Fix this by calling platform_device_put() in the error branch before removing the software node.

---
 drivers/misc/eeprom/digsy_mtc_eeprom.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index ee58f7ce5bfa..4ca3e567c49d 100644
@@ -89,8 +89,10 @@ static int __init digsy_mtc_eeprom_devices_init(void)
 		return ret;
 
 	ret = platform_device_register(&digsy_mtc_eeprom);
-	if (ret)
+	if (ret) {
+		platform_device_put(&digsy_mtc_eeprom);
 		device_remove_software_node(&digsy_mtc_eeprom.dev);
+	}
 
 	return ret;
 }

2.51.0

When platform_device_register() fails in cros_ec_lpc_init(), the embedded struct device has already been initialized by device_initialize() inside platform_device_register(). The error path unregisters the driver but returns without dropping the device reference:

cros_ec_lpc_init()

    -> platform_device_register(&cros_ec_lpc_device)
       -> device_initialize(&cros_ec_lpc_device.dev)   /* kref = 1 */
       -> platform_device_add(&cros_ec_lpc_device)     /* fails */
    <- platform_driver_unregister() called, but kref still 1

Per platform_device_register() kernel-doc:

NOTE: _Never_ directly free @pdev after calling this function, even if it returned an error! Always use platform_device_put() to give up the reference initialised in this function instead.

Fix this by calling platform_device_put() before unregistering the driver.

---
 drivers/platform/chrome/cros_ec_lpc.c | 1 +
 1 file changed, 1 insertion(+)

index 78cfff80cdea..cb3ff76d29e9 100644
@@ -892,6 +892,7 @@ static int __init cros_ec_lpc_init(void)
 		ret = platform_device_register(&cros_ec_lpc_device);
 		if (ret) {
 			pr_err(DRV_NAME ": can't register device: %d\n", ret);
+			platform_device_put(&cros_ec_lpc_device);
 			platform_driver_unregister(&cros_ec_lpc_driver);
 		}
 	}

2.51.0

When platform_device_register() fails in physmap_init(), the embedded struct device has already been initialized by device_initialize() inside platform_device_register(). The error path unregisters the driver but returns without dropping the device reference:

physmap_init()

    -> platform_device_register(&physmap_flash)
       -> device_initialize(&physmap_flash.dev)   /* kref = 1 */
       -> platform_device_add(&physmap_flash)     /* fails */
    <- platform_driver_unregister() called, but kref still 1

Per platform_device_register() kernel-doc:

NOTE: _Never_ directly free @pdev after calling this function, even if it returned an error! Always use platform_device_put() to give up the reference initialised in this function instead.

Fix this by calling platform_device_put() before unregistering the driver.

---
 drivers/mtd/maps/physmap-core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index dcda7685fc99..45d79ca622c1 100644
@@ -654,8 +654,10 @@ static int __init physmap_init(void)
 #ifdef CONFIG_MTD_PHYSMAP_COMPAT
 	if (err == 0) {
 		err = platform_device_register(&physmap_flash);
-		if (err)
+		if (err) {
+			platform_device_put(&physmap_flash);
 			platform_driver_unregister(&physmap_flash_driver);
+		}
 	}
 #endif
 

2.51.0

When platform_device_register() fails in arm_acpi_register_pmu_device(), the embedded struct device has already been initialized by device_initialize() inside platform_device_register(). The error path unregisters the GSI interrupt but returns without dropping the device reference:

arm_acpi_register_pmu_device()

    -> platform_device_register(pdev)
       -> device_initialize(&pdev->dev)   /* kref = 1 */
       -> platform_device_add(pdev)       /* fails */
    <- acpi_unregister_gsi() called, but kref still 1

Per platform_device_register() kernel-doc:

NOTE: _Never_ directly free @pdev after calling this function, even if it returned an error! Always use platform_device_put() to give up the reference initialised in this function instead.

Fix this by calling platform_device_put() in the error branch before unregistering the GSI.

---
 drivers/perf/arm_pmu_acpi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index e80f76d95e68..c2defbc32ad9 100644
@@ -119,8 +119,10 @@ arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len,
 
 	pdev->resource[0].start = irq;
 	ret = platform_device_register(pdev);
-	if (ret)
+	if (ret) {
+		platform_device_put(pdev);
 		acpi_unregister_gsi(gsi);
+	}
 
 	return ret;
 }

2.51.0

When platform_device_register() fails in sm501_register_device(), the platform device allocated by sm501_create_subdev() has its struct device initialized by device_initialize() inside platform_device_register(). The error path logs the error but returns without dropping the device reference, leaking the memory allocated by sm501_create_subdev():

sm501_register_device()

    -> platform_device_register(pdev)
       -> device_initialize(&pdev->dev)   /* kref = 1 */
       -> platform_device_add(pdev)       /* fails */
    <- dev_err() called, kref still 1, sm501_device_release never called

The device's release callback (sm501_device_release) calls kfree() on the containing sm501_device structure. Without platform_device_put(), this memory is never freed.

Per platform_device_register() kernel-doc:

NOTE: _Never_ directly free @pdev after calling this function, even if it returned an error! Always use platform_device_put() to give up the reference initialised in this function instead.

Fix this by calling platform_device_put() in the error branch, which triggers sm501_device_release() and frees the allocated memory.

---
 drivers/mfd/sm501.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 0ee6d8940e69..8276456b142f 100644
@@ -704,9 +704,11 @@ static int sm501_register_device(struct sm501_devdata *sm,
 	if (ret >= 0) {
 		dev_dbg(sm->dev, "registered %s\n", pdev->name);
 		list_add_tail(&smdev->list, &sm->devices);
-	} else
+	} else {
 		dev_err(sm->dev, "error registering %s (%d)\n",
 			pdev->name, ret);
+		platform_device_put(pdev);
+	}
 
 	return ret;
 }

2.51.0

platform_device_register() calls device_initialize() before platform_device_add(). If platform_device_add() fails, device_initialize() has already set the struct device kref to 1, and the kref must be released via platform_device_put(). The kernel-doc for platform_device_register() states this explicitly:

NOTE: _Never_ directly free @pdev after calling this function, even if it returned an error! Always use platform_device_put() to give up the reference initialised in this function instead.

This series fixes five drivers that call platform_device_register() and return the error code without calling platform_device_put() on the failure path. The pattern was identified by Coccinelle static analysis, confirmed by manual inspection, and is analogous to the recently merged fix in commit 4ff036f95238 ("ALSA: pcmtest: fix reference leak on failed device registration").

Sites fixed:
drivers/misc/eeprom/digsy_mtc_eeprom.c
drivers/platform/chrome/cros_ec_lpc.c
drivers/mtd/maps/physmap-core.c
drivers/perf/arm_pmu_acpi.c
drivers/mfd/sm501.c

Build tested on x86 defconfig:
- digsy_mtc_eeprom.c: compiled cleanly

  - cros_ec_lpc.c:       compiled cleanly
  - sm501.c:             compiled cleanly
  - physmap-core.c:      pre-existing Werror in map.h header (unrelated)
  - arm_pmu_acpi.c:      ARM-only, no asm/cputype.h on x86

Vastargazing (5):
misc: eeprom: digsy_mtc: fix reference leak on failed device
registration
platform/chrome: cros_ec_lpc: fix reference leak on failed device
registration
mtd: maps: physmap: fix reference leak on failed device registration
perf: arm: pmu: fix reference leak on failed device registration
mfd: sm501: fix reference leak on failed device registration

 drivers/mfd/sm501.c                    | 4 +++-
 drivers/misc/eeprom/digsy_mtc_eeprom.c | 4 +++-
 drivers/mtd/maps/physmap-core.c        | 4 +++-
 drivers/perf/arm_pmu_acpi.c            | 4 +++-
 drivers/platform/chrome/cros_ec_lpc.c  | 1 +
 5 files changed, 13 insertions(+), 4 deletions(-)

2.51.0

When platform_device_register() fails in sm501_register_device(), the platform device allocated by sm501_create_subdev() has its struct device initialized by device_initialize() inside platform_device_register(). The error path logs the error but returns without dropping the device reference, leaking the memory allocated by sm501_create_subdev():

sm501_register_device()

    -> platform_device_register(pdev)
       -> device_initialize(&pdev->dev)   /* kref = 1 */
       -> platform_device_add(pdev)       /* fails */
    <- dev_err() called, kref still 1, sm501_device_release never called

The device's release callback (sm501_device_release) calls kfree() on the containing sm501_device structure. Without platform_device_put(), this memory is never freed.

Per platform_device_register() kernel-doc:

NOTE: _Never_ directly free @pdev after calling this function, even if it returned an error! Always use platform_device_put() to give up the reference initialised in this function instead.

Fix this by calling platform_device_put() in the error branch, which triggers sm501_device_release() and frees the allocated memory.

---
 drivers/mfd/sm501.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 0ee6d8940e69..8276456b142f 100644
@@ -704,9 +704,11 @@ static int sm501_register_device(struct sm501_devdata *sm,
 	if (ret >= 0) {
 		dev_dbg(sm->dev, "registered %s\n", pdev->name);
 		list_add_tail(&smdev->list, &sm->devices);
-	} else
+	} else {
 		dev_err(sm->dev, "error registering %s (%d)\n",
 			pdev->name, ret);
+		platform_device_put(pdev);
+	}
 
 	return ret;
 }

2.51.0

Applied to mtd/next, thanks!

[1/1] mfd: sm501: fix reference leak on failed device registration

      commit: faa9bba3fe2f37e7dcb26d4501d890fbfd7df160

Patche(s) should be available on mtd/linux.git and will be part of the next PR (provided that no robot complains by then).

Kind regards,
Miquèl

Somehow b4 applied another patch from that series, I'm not sure why. I just dropped it. Please resend this patch alone.

Also, your SoB line seems to be incorrect, we need a proper name, please have a loot at the DCO.

Thanks,
Miquèl

Yeah, you're right, my bad. The arm_pmu_acpi.c patch is definitely broken.

Since spe_dev and trbe_dev are statically allocated, they don't have a .dev.release callback. If we hit platform_device_put() here, the refcount drops to zero and triggers device_release(), which is going to scream about the missing release function. At best, we get a messy WARN; at worst, it'll panic the kernel if someone's running with panic_on_warn.

The kernel-doc note about platform_device_put() is really meant for dynamic allocations where the release path actually frees memory. For static setups like this, the original code is actually the right way to go.

Please drop patches 1/5 through 4/5 from the v1 series—they all suffer from the same logic error. Patch 5/5 (mfd: sm501) is the only clean one, so I've re-sent that as a standalone v2.

Sorry for the noise.

Valery Borovsky

I think you misread the subject line.

Please remove this from your tree. It should be handled via M[F]D.

Thanks.

Lee Jones

Hi Lee,

Yes, it took a bit of time for me to receive my own answer so I replied to the original patch immediately stating that I dropped it. For some reason b4 applied this patch, whereas I was applying another m*t*d patch from apparently the same series (?). Both the contribution and b4 behaviour was strange.

Sorry for the noise.

Thanks,
Miquèl

When platform_device_register() fails in sm501_register_device(), the platform device allocated by sm501_create_subdev() has its struct device initialized by device_initialize() inside platform_device_register(). The error path logs the error but returns without dropping the device reference, leaking the memory allocated by sm501_create_subdev():

sm501_register_device()

    -> platform_device_register(pdev)
       -> device_initialize(&pdev->dev)   /* kref = 1 */
       -> platform_device_add(pdev)       /* fails */
    <- dev_err() called, kref still 1, sm501_device_release never called

The device's release callback (sm501_device_release) calls kfree() on the containing sm501_device structure. Without platform_device_put(), this memory is never freed.

Per platform_device_register() kernel-doc:

NOTE: _Never_ directly free @pdev after calling this function, even if it returned an error! Always use platform_device_put() to give up the reference initialised in this function instead.

Fix this by calling platform_device_put() in the error branch, which triggers sm501_device_release() and frees the allocated memory.

---
 drivers/mfd/sm501.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 0ee6d8940e69..8276456b142f 100644
@@ -704,9 +704,11 @@ static int sm501_register_device(struct sm501_devdata *sm,
 	if (ret >= 0) {
 		dev_dbg(sm->dev, "registered %s\n", pdev->name);
 		list_add_tail(&smdev->list, &sm->devices);
-	} else
+	} else {
 		dev_err(sm->dev, "error registering %s (%d)\n",
 			pdev->name, ret);
+		platform_device_put(pdev);
+	}
 
 	return ret;
 }

2.51.0

Hi Miquel,

Thanks for the review. I need to withdraw this patch as well, for the same reason as the rest of the series.

physmap_flash is a static platform_device without a .dev.release callback. Using platform_device_put() triggers a WARN in device_release() when the kref hits zero, which is a dealbreaker for systems with panic_on_warn=1.

The kernel-doc NOTE refers to dynamically allocated devices; for static ones, the original code is actually correct.

I've already sent a withdrawal notice for the whole series (1/5-4/5) to the cover letter thread. Sorry for the noise.

Best,
Valery Borovsky

I don't see a message from me acknowledging this, but it is applied to my tree.

Lee Jones

Hi Lee,

Great, thanks for picking it up — confirmed on my end.

For context: an earlier version of this patch ended up briefly in mtd/next by accident (Miquel applied it to the wrong tree, then dropped it), so I appreciate you taking it directly into MFD.

Best,
Valery