[PATCH] powercap: Hold control_type->lock when removing a zone ID

[PATCH] powercap: Hold control_type->lock when removing a zone ID

[Changyul Lee]

powercap_register_zone() allocates the zone ID from the parent IDR
under control_type->lock, but powercap_release() removes it without
that lock. powercap_unregister_zone() calls device_unregister() after
dropping the lock, so removing one zone can change the IDR at the same
time as registering another zone of the same control type. The IDR can
then hand out an ID that is still in use, so device_register() fails
with -EEXIST, or keep an ID that was removed. intel_rapl_tpmi can do
this, as all packages share one control type and each package device
is bound and unbound on its own.

Take control_type->lock in powercap_release(). The device_register()
error path of powercap_register_zone() drops the last reference with
the lock held, so drop the lock before put_device() there.

Fixes: 75d2364ea0ca ("PowerCap: Add class driver")
Signed-off-by: Changyul Lee <lcy8047@gmail.com>
---
Found with a static analysis tool we are developing.

This is on top of commit 3d1c2693d809 ("powercap: Do not release the
zone when device_register() fails") in linux-pm.

Reproduced in QEMU (x86_64, KASAN, lockdep) with a test module that
runs four kthreads for 20 seconds. Each kthread registers and
unregisters a top-level zone of one control type in a loop. Without
this patch, powercap_register_zone() failed with -EEXIST 7 to 12 times
per run in three runs, the highest zone ID was 4 to 8 although at most
four zones existed at a time, and up to four IDs were left in the IDR
at the end. With it, there was no failure, the highest ID was 3 and the
IDR was empty at the end, in all three runs. The module also makes one
device_register() fail on purpose, and with this patch that error path
ran without a lockdep report.
Not tested on Intel RAPL hardware.

 drivers/powercap/powercap_sys.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/powercap/powercap_sys.c b/drivers/powercap/powercap_sys.c
index eacbc3c96db1..e9aa1feb5e3c 100644
--- a/drivers/powercap/powercap_sys.c
+++ b/drivers/powercap/powercap_sys.c
@@ -388,11 +388,15 @@ static void powercap_release(struct device *dev)
 
 	if (dev->parent) {
 		struct powercap_zone *power_zone = to_powercap_zone(dev);
+		struct powercap_control_type *control_type =
+						power_zone->control_type_inst;
 
 		/* Store flag as the release() may free memory */
 		allocated = power_zone->allocated;
 		/* Remove id from parent idr struct */
+		mutex_lock(&control_type->lock);
 		idr_remove(power_zone->parent_idr, power_zone->id);
+		mutex_unlock(&control_type->lock);
 		/* Destroy idrs allocated for this zone */
 		idr_destroy(&power_zone->idr);
 		kfree(power_zone->name);
@@ -561,8 +565,8 @@ struct powercap_zone *powercap_register_zone(
 		 * ->release(), which may free the zone.
 		 */
 		power_zone->ops = NULL;
-		put_device(&power_zone->dev);
 		mutex_unlock(&control_type->lock);
+		put_device(&power_zone->dev);
 
 		return ERR_PTR(result);
 	}

base-commit: 3d1c2693d8091a6529cf7ed8079c4968527d58b4
-- 
2.43.0