[PATCH] reset: fix racy acquisition of exclusive reset controls

[Alvin Šipraga]

A given reset line may only be acquired by up to one exclusive reset
control at a time. If two tasks with their own exclusive controls race
to acquire a currently released line, only one may succeed.

Since the blamed commit, which introduced more granular locking, this
guarantee has been lost. The mutex rstc->lock was introduced to
serialize the internals of reset_control_acquire(), but since this lock
is local to a given reset control, it achieves no synchronization
between distinct reset controls, even though they may refer to the same
underlying reset line.

As it happens, there is already sufficient synchronization in this
function through the rcdev->lock mutex, which protects the controller's
reset control list. Under this lock, the acquisition logic correctly
validates that the line is not acquired by another control. But the
control is marked as acquired only after the lock is freed, wherein
another task may race to the same point and mark itself acquired too.

This is easy to fix: just mark acquisition while the rcdev->lock mutex
is still held. And since the rstc->lock is only used here, and achieves
nothing, it can be disposed of.

Tested under QEMU (-M ast2600-evb -smp 2) with some driver hacks to
provoke the race. With this fix, the race is no longer reproducible.

Fixes: 8c91302a29bc ("reset: protect struct reset_control with its own mutex")
Link: https://lore.kernel.org/lkml/atnZWUA9bc1btCix@xxxxxxxxxx/
Signed-off-by: Alvin Šipraga <alvin.sipraga@xxxxxxxxxx>
---
Philipp, this will probably conflict with Shengjiu's fix for the shared
reset race. If you want, I'll gladly resend after that fix has been
applied. Just putting out for you to check now.
---
 drivers/reset/core.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 38e189d04d09..70115233e951 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -51,7 +51,6 @@ static DEFINE_IDA(reset_gpio_ida);
  * @triggered_count: Number of times this reset line has been reset. Currently
  *                   only used for shared resets, which means that the value
  *                   will be either 0 or 1.
- * @lock: serializes the internals of reset_control_acquire()
  */
 struct reset_control {
 	struct reset_controller_dev __rcu *rcdev;
@@ -64,7 +63,6 @@ struct reset_control {
 	bool array;
 	atomic_t deassert_count;
 	atomic_t triggered_count;
-	struct mutex lock;
 };
 
 /**
@@ -726,8 +724,6 @@ int reset_control_acquire(struct reset_control *rstc)
 	if (reset_control_is_array(rstc))
 		return reset_control_array_acquire(rstc_to_array(rstc));
 
-	guard(mutex)(&rstc->lock);
-
 	if (rstc->acquired)
 		return 0;
 
@@ -744,9 +740,9 @@ int reset_control_acquire(struct reset_control *rstc)
 					return -EBUSY;
 			}
 		}
-	}
 
-	rstc->acquired = true;
+		rstc->acquired = true;
+	}
 
 	return 0;
 }
@@ -878,7 +874,6 @@ __reset_control_get_internal(struct reset_controller_dev *rcdev,
 	list_add(&rstc->list, &rcdev->reset_control_head);
 	rstc->id = index;
 	kref_init(&rstc->refcnt);
-	mutex_init(&rstc->lock);
 	rstc->acquired = acquired;
 	rstc->shared = shared;
 	get_device(rcdev->dev);
@@ -899,8 +894,6 @@ static void __reset_control_release(struct kref *kref)
 		lockdep_assert_held(&rcdev->lock);
 		reset_controller_remove(rcdev, rstc);
 	}
-
-	mutex_destroy(&rstc->lock);
 }
 
 static void reset_control_put_internal(struct reset_control *rstc)

---
base-commit: 71827776667f4e4677a4fa806bcfb24d4b8dd9d7
change-id: 20261022-reset-acquire-race-7de7e2b94912

Best regards,
--  
Alvin Šipraga <alvin.sipraga@xxxxxxxxxx>