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

[PATCH 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().

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 | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

-- 
2.53.0

[PATCH 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>
---
 drivers/reset/core.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 38e189d04d09..1f6d7ef48a67 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -160,6 +160,8 @@ int reset_controller_register(struct reset_controller_dev *rcdev)
 }
 EXPORT_SYMBOL_GPL(reset_controller_register);
 
+static void reset_control_put_internal(struct reset_control *rstc);
+
 static void reset_controller_remove(struct reset_controller_dev *rcdev,
 				    struct reset_control *rstc)
 {
@@ -185,11 +187,17 @@ 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);
+			reset_control_put_internal(rstc);
 		}
 	}
 
@@ -913,12 +921,13 @@ 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) {
-- 
2.53.0

[PATCH 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 v2] 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/20261102155207.19252-1-tzungbi@kernel.org
---
 drivers/reset/core.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 1f6d7ef48a67..a36a4d6ff445 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -179,6 +179,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);
@@ -195,12 +196,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);
-			reset_control_put_internal(rstc);
+			list_add(&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) {
+		list_del(&rstc->list);
+		synchronize_srcu(&rstc->srcu);
+		reset_control_put_internal(rstc);
+	}
+
 	mutex_destroy(&rcdev->lock);
 }
 EXPORT_SYMBOL_GPL(reset_controller_unregister);
-- 
2.53.0

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

[Philipp Zabel]

On Mo, 2026-11-02 at 23:58 +0800, Tzung-Bi Shih wrote:
> 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 v2] 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/20261102155207.19252-1-tzungbi@kernel.org
> ---
>  drivers/reset/core.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 1f6d7ef48a67..a36a4d6ff445 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -179,6 +179,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);
> @@ -195,12 +196,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);

Now reset_controller_remove() can be called while a concurrent reset
operation that got an rcdev reference just before the
rcu_assign_pointer() is about to enter a reset_control_ops callback.

>  			reset_controller_remove(rcdev, rstc);

I think this should only do the list_del(&rstc->list); ...

> -			reset_control_put_internal(rstc);
> +			list_add(&rstc->list, &reset_control_head);

... which could be combined with this into list_move(). That would also
make clearer what happens here (move into a temporary list that doesn't
need to be rcdev->lock'ed).

The module_put() and put_device() calls should be deferred into the
loop below, after synchronize_srcu(). We don't make it possible to
unload the reset controller driver module until then.

>  		}
>  	}
>  
> +	/*
> +	 * 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) {
> +		list_del(&rstc->list);
> +		synchronize_srcu(&rstc->srcu);
> +		reset_control_put_internal(rstc);

This shouldn't call reset_control_put_internal(), which is supposed to
be the common internal implementation of the consumer
reset_control_put() API variants.

Rather, I'd prefer this to call ref = kref_put() directly and then put
the "if (ret) { ... }" part of reset_control_put_internal() into a
separate helper function that can be called from here as well.

In case the revocable series gets accepted, this might turn out to be a
non-issue though.

regards
Philipp