[PATCH v2 0/2] reset: Fix issues reported by Sashiko

[PATCH v2 0/2] reset: Fix issues reported by Sashiko

[Tzung-Bi Shih]

This series fixes two issues in the reset controller unregistration path
reported by Sashiko:

- Patch 1 fixes a reset_control leak when reset_control_put() is called
  after reset_controller_unregister().
- Patch 2 fixes a circular locking dependency between `rcdev->lock` and
  `rstc->srcu` in reset_controller_unregister().
---
v2:
- Address review comments on reset_controller_unregister() cleanup and
  teardown ordering.

v1: https://lore.kernel.org/all/20261102155840.20376-1-tzungbi@kernel.org

Tzung-Bi Shih (2):
  reset: Fix reset_control leak when put after controller unregister
  reset: Fix circular locking dependency in
    reset_controller_unregister()

 drivers/reset/core.c | 69 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 52 insertions(+), 17 deletions(-)

-- 
2.53.0

[PATCH v2 1/2] reset: Fix reset_control leak when put after controller unregister

[Tzung-Bi Shih]

After commit 8c91302a29bc ("reset: protect struct reset_control with its
own mutex"), reset_control_put_internal() returns early if `rcdev` is
NULL.

When a reset controller is unregistered while consumers still hold
reset_control handles, reset_controller_unregister() sets `rstc->rcdev`
to NULL.  When consumers later release their handles via
reset_control_put(), the early return skips kref_put(), resulting in
leaking resources.

Call kref_put() in reset_control_put_internal() even if `rcdev` is NULL.

Fixes: 8c91302a29bc ("reset: protect struct reset_control with its own mutex")
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
v2:
- Factor out __reset_control_free().
- Call kref_put() directly in reset_controller_unregister() instead of
  calling reset_control_put_internal().
- Drop redundant lockdep_assert_held() and add comment for
  __reset_control_release().

v1: https://lore.kernel.org/all/20261102155840.20376-2-tzungbi@kernel.org
---
 drivers/reset/core.c | 47 ++++++++++++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 13 deletions(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 38e189d04d09..f0ab09795ae1 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -160,6 +160,9 @@ int reset_controller_register(struct reset_controller_dev *rcdev)
 }
 EXPORT_SYMBOL_GPL(reset_controller_register);
 
+static void __reset_control_release(struct kref *kref);
+static void __reset_control_free(struct reset_control *rstc);
+
 static void reset_controller_remove(struct reset_controller_dev *rcdev,
 				    struct reset_control *rstc)
 {
@@ -185,11 +188,18 @@ void reset_controller_unregister(struct reset_controller_dev *rcdev)
 		/*
 		 * Numb but don't free the remaining reset control handles that are
 		 * still held by consumers.
+		 *
+		 * Hold a temporary reference on rstc before clearing rstc->rcdev:
+		 * once rstc->rcdev is NULL, a concurrent reset_control_put_internal()
+		 * will not acquire rcdev->lock and could otherwise free rstc.
 		 */
 		list_for_each_entry_safe(rstc, pos, &rcdev->reset_control_head, list) {
+			kref_get(&rstc->refcnt);
 			rcu_assign_pointer(rstc->rcdev, NULL);
 			synchronize_srcu(&rstc->srcu);
 			reset_controller_remove(rcdev, rstc);
+			if (kref_put(&rstc->refcnt, __reset_control_release))
+				__reset_control_free(rstc);
 		}
 	}
 
@@ -886,14 +896,16 @@ __reset_control_get_internal(struct reset_controller_dev *rcdev,
 	return rstc;
 }
 
+/*
+ * Detaches rstc from rcdev (if still attached) when refcnt reaches 0.
+ * If rstc is still attached, the caller must hold rcdev->lock.
+ */
 static void __reset_control_release(struct kref *kref)
 {
 	struct reset_control *rstc = container_of(kref, struct reset_control,
 						  refcnt);
 	struct reset_controller_dev *rcdev;
 
-	lockdep_assert_held(&rstc->srcu);
-
 	rcdev = rcu_replace_pointer(rstc->rcdev, NULL, true);
 	if (rcdev) {
 		lockdep_assert_held(&rcdev->lock);
@@ -903,6 +915,17 @@ static void __reset_control_release(struct kref *kref)
 	mutex_destroy(&rstc->lock);
 }
 
+/*
+ * Must be called outside the rstc->srcu read-side critical section after
+ * kref_put(..., __reset_control_release) returns true.
+ */
+static void __reset_control_free(struct reset_control *rstc)
+{
+	synchronize_srcu(&rstc->srcu);
+	cleanup_srcu_struct(&rstc->srcu);
+	kfree(rstc);
+}
+
 static void reset_control_put_internal(struct reset_control *rstc)
 {
 	struct reset_controller_dev *rcdev;
@@ -913,19 +936,17 @@ static void reset_control_put_internal(struct reset_control *rstc)
 
 	scoped_guard(srcu, &rstc->srcu) {
 		rcdev = srcu_dereference(rstc->rcdev, &rstc->srcu);
-		if (!rcdev)
-			/* Already released. */
-			return;
-
-		guard(mutex)(&rcdev->lock);
-		ret = kref_put(&rstc->refcnt, __reset_control_release);
+		if (rcdev) {
+			guard(mutex)(&rcdev->lock);
+			ret = kref_put(&rstc->refcnt, __reset_control_release);
+		} else {
+			/* Already detached from rcdev. */
+			ret = kref_put(&rstc->refcnt, __reset_control_release);
+		}
 	}
 
-	if (ret) {
-		synchronize_srcu(&rstc->srcu);
-		cleanup_srcu_struct(&rstc->srcu);
-		kfree(rstc);
-	}
+	if (ret)
+		__reset_control_free(rstc);
 }
 
 static void reset_gpio_aux_device_release(struct device *dev)
-- 
2.53.0

[PATCH v2 2/2] reset: Fix circular locking dependency in reset_controller_unregister()

[Tzung-Bi Shih]

reset_control_acquire() and reset_control_put_internal() acquire
`rcdev->lock` inside the `rstc->srcu` read-side critical section
(`rstc->srcu` -> `rcdev->lock`).

However, reset_controller_unregister() holds `rcdev->lock` while calling
synchronize_srcu(&rstc->srcu) (`rcdev->lock` -> `rstc->srcu`), causing a
lockdep circular locking dependency warning and a potential ABBA
deadlock.

Run synchronize_srcu() outside `rcdev->lock`.

Fixes: 44a0acb2caca ("reset: protect struct reset_controller_dev with its own mutex")
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
Test:
With "[PATCH v3] reset: Add KUnit test cases"[1] applied:
$ ./tools/testing/kunit/kunit.py run \
        --kconfig_add CONFIG_RESET_CONTROLLER=y \
        --kconfig_add CONFIG_RESET_KUNIT_TEST=y \
        --kconfig_add CONFIG_PROVE_LOCKING=y \
        --kconfig_add CONFIG_DEBUG_KERNEL=y \
        --kconfig_add CONFIG_KASAN=y \
        --arch=x86_64 \
        --raw_output=all \
        reset_test

[1] https://lore.kernel.org/all/20261104160123.6784-1-tzungbi@kernel.org
---
v2:
- Use list_move() to make the intent clear.
- Defer reset_controller_remove() after synchronize_srcu() to avoid race.
- Drop lockdep_assert_held() and add comment for reset_controller_remove().

v1: https://lore.kernel.org/all/20261102155840.20376-3-tzungbi@kernel.org
---
 drivers/reset/core.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index f0ab09795ae1..4a582721782f 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -163,11 +163,15 @@ EXPORT_SYMBOL_GPL(reset_controller_register);
 static void __reset_control_release(struct kref *kref);
 static void __reset_control_free(struct reset_control *rstc);
 
+/*
+ * Removes rstc from its list and drops the references on rcdev's owner module
+ * and device.  The caller must ensure rstc->list access is serialized (e.g.,
+ * by holding rcdev->lock or moving rstc to a private list) and that no
+ * operations on rcdev via rstc are in flight.
+ */
 static void reset_controller_remove(struct reset_controller_dev *rcdev,
 				    struct reset_control *rstc)
 {
-	lockdep_assert_held(&rcdev->lock);
-
 	list_del(&rstc->list);
 	module_put(rcdev->owner);
 	put_device(rcdev->dev);
@@ -180,6 +184,7 @@ static void reset_controller_remove(struct reset_controller_dev *rcdev,
 void reset_controller_unregister(struct reset_controller_dev *rcdev)
 {
 	struct reset_control *rstc, *pos;
+	LIST_HEAD(reset_control_head);
 
 	scoped_guard(mutex, &reset_list_mutex)
 		list_del(&rcdev->list);
@@ -196,13 +201,22 @@ void reset_controller_unregister(struct reset_controller_dev *rcdev)
 		list_for_each_entry_safe(rstc, pos, &rcdev->reset_control_head, list) {
 			kref_get(&rstc->refcnt);
 			rcu_assign_pointer(rstc->rcdev, NULL);
-			synchronize_srcu(&rstc->srcu);
-			reset_controller_remove(rcdev, rstc);
-			if (kref_put(&rstc->refcnt, __reset_control_release))
-				__reset_control_free(rstc);
+			list_move(&rstc->list, &reset_control_head);
 		}
 	}
 
+	/*
+	 * Run synchronize_srcu() outside rcdev->lock to avoid a circular
+	 * locking dependency with paths that acquire rcdev->lock inside
+	 * the rstc->srcu read-side critical section.
+	 */
+	list_for_each_entry_safe(rstc, pos, &reset_control_head, list) {
+		synchronize_srcu(&rstc->srcu);
+		reset_controller_remove(rcdev, rstc);
+		if (kref_put(&rstc->refcnt, __reset_control_release))
+			__reset_control_free(rstc);
+	}
+
 	mutex_destroy(&rcdev->lock);
 }
 EXPORT_SYMBOL_GPL(reset_controller_unregister);
-- 
2.53.0

Re: [PATCH v2 1/2] reset: Fix reset_control leak when put after controller unregister

[Markus Elfring]

…
> +++ b/drivers/reset/core.c
> @@ -160,6 +160,9 @@ int reset_controller_register(struct reset_controller_dev *rcdev)
>  }
>  EXPORT_SYMBOL_GPL(reset_controller_register);
>  
> +static void __reset_control_release(struct kref *kref);
> +static void __reset_control_free(struct reset_control *rstc);
…

How do you think about to reconsider the usage of double underscores
in such identifiers any more?
https://cmu-sei.github.io/secure-coding-standards/sei-cert-c-coding-standard/rules/declarations-and-initialization-dcl/dcl37-c/

Regards,
Markus