* [PATCH v2 1/2] reset: Fix reset_control leak when put after controller unregister
2026-11-04 16:20 [PATCH v2 0/2] reset: Fix issues reported by Sashiko Tzung-Bi Shih
@ 2026-11-04 16:20 ` Tzung-Bi Shih
2026-11-04 17:38 ` Markus Elfring
2026-11-04 16:20 ` [PATCH v2 2/2] reset: Fix circular locking dependency in reset_controller_unregister() Tzung-Bi Shih
2026-11-05 9:16 ` [PATCH v2 0/2] reset: Fix issues reported by Sashiko Bartosz Golaszewski
2 siblings, 1 reply; 5+ messages in thread
From: Tzung-Bi Shih @ 2026-11-04 16:20 UTC (permalink / raw)
To: Philipp Zabel; +Cc: Bartosz Golaszewski, linux-kernel, tzungbi
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
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH v2 2/2] reset: Fix circular locking dependency in reset_controller_unregister()
2026-11-04 16:20 [PATCH v2 0/2] reset: Fix issues reported by Sashiko Tzung-Bi Shih
2026-11-04 16:20 ` [PATCH v2 1/2] reset: Fix reset_control leak when put after controller unregister Tzung-Bi Shih
@ 2026-11-04 16:20 ` Tzung-Bi Shih
2026-11-05 9:16 ` [PATCH v2 0/2] reset: Fix issues reported by Sashiko Bartosz Golaszewski
2 siblings, 0 replies; 5+ messages in thread
From: Tzung-Bi Shih @ 2026-11-04 16:20 UTC (permalink / raw)
To: Philipp Zabel; +Cc: Bartosz Golaszewski, linux-kernel, tzungbi
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
^ permalink raw reply [flat|nested] 5+ messages in thread