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

[Philipp Zabel <p.zabel@pengutronix.de>]

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