diff mbox series

[Concept,01/35] watchdog: Unregister cyclic on device removal

Message ID 20251210000737.180797-2-sjg@u-boot.org
State New
Headers
Series malloc: Add heap debugging commands and mcheck caller tracking |

Commit Message

Simon Glass Dec. 10, 2025, 12:06 a.m. UTC 
  From: Simon Glass <simon.glass@canonical.com>

When a watchdog device is destroyed, the cyclic_info embedded in the
device's private data is freed but remains in the global cyclic list.
The subsequent cyclic_unregister_all() call then accesses freed memory,
causing a crash.

Add a pre_remove hook to the watchdog uclass to unregister the cyclic
function before the device is destroyed.

Co-developed-by: Claude <noreply@anthropic.com>
Signed-off-by: Simon Glass <simon.glass@canonical.com>
---

 drivers/watchdog/wdt-uclass.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Heinrich Schuchardt Dec. 10, 2025, 6:53 a.m. UTC | #1 
Am 10. Dezember 2025 01:06:52 MEZ schrieb Simon Glass <sjg@u-boot.org>:
>From: Simon Glass <simon.glass@canonical.com>
>
>When a watchdog device is destroyed, the cyclic_info embedded in the
>device's private data is freed but remains in the global cyclic list.
>The subsequent cyclic_unregister_all() call then accesses freed memory,
>causing a crash.
>
>Add a pre_remove hook to the watchdog uclass to unregister the cyclic
>function before the device is destroyed.

Is this a sandbox only problem? Or can this happen on real hardware when booting?


>
>Co-developed-by: Claude <noreply@anthropic.com>
>Signed-off-by: Simon Glass <simon.glass@canonical.com>
>---
>
> drivers/watchdog/wdt-uclass.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
>diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
>index 10be334e9ed..c2d19530b3d 100644
>--- a/drivers/watchdog/wdt-uclass.c
>+++ b/drivers/watchdog/wdt-uclass.c
>@@ -256,10 +256,21 @@ static int wdt_pre_probe(struct udevice *dev)
> 	return 0;
> }
> 
>+static int wdt_pre_remove(struct udevice *dev)
>+{
>+	struct wdt_priv *priv = dev_get_uclass_priv(dev);
>+
>+	if (IS_ENABLED(CONFIG_WATCHDOG) && priv->running)
>+		cyclic_unregister(&priv->cyclic);
>+
>+	return 0;
>+}
>+
> UCLASS_DRIVER(wdt) = {
> 	.id			= UCLASS_WDT,
> 	.name			= "watchdog",
> 	.flags			= DM_UC_FLAG_SEQ_ALIAS,
> 	.pre_probe		= wdt_pre_probe,
>+	.pre_remove		= wdt_pre_remove,
> 	.per_device_auto	= sizeof(struct wdt_priv),
> };
Simon Glass Dec. 10, 2025, 12:32 p.m. UTC | #2 
Hi Heinrich,

On Tue, 9 Dec 2025 at 23:53, Heinrich Schuchardt via Concept
<concept@u-boot.org> wrote:
>
> Am 10. Dezember 2025 01:06:52 MEZ schrieb Simon Glass <sjg@u-boot.org>:
> >From: Simon Glass <simon.glass@canonical.com>
> >
> >When a watchdog device is destroyed, the cyclic_info embedded in the
> >device's private data is freed but remains in the global cyclic list.
> >The subsequent cyclic_unregister_all() call then accesses freed memory,
> >causing a crash.
> >
> >Add a pre_remove hook to the watchdog uclass to unregister the cyclic
> >function before the device is destroyed.
>
> Is this a sandbox only problem? Or can this happen on real hardware when booting?

Normally when booting we don't have idle time, so cyclic is not called.

It looks like only octeontx_wdt.c is marked with the
DM_FLAG_OS_PREPARE flag. So perhaps on that platform, if the device is
removed and then schedule() is called, it would cause the problem.

Regards,
Simon


>
>
> >
> >Co-developed-by: Claude <noreply@anthropic.com>
> >Signed-off-by: Simon Glass <simon.glass@canonical.com>
> >---
> >
> > drivers/watchdog/wdt-uclass.c | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> >diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> >index 10be334e9ed..c2d19530b3d 100644
> >--- a/drivers/watchdog/wdt-uclass.c
> >+++ b/drivers/watchdog/wdt-uclass.c
> >@@ -256,10 +256,21 @@ static int wdt_pre_probe(struct udevice *dev)
> >       return 0;
> > }
> >
> >+static int wdt_pre_remove(struct udevice *dev)
> >+{
> >+      struct wdt_priv *priv = dev_get_uclass_priv(dev);
> >+
> >+      if (IS_ENABLED(CONFIG_WATCHDOG) && priv->running)
> >+              cyclic_unregister(&priv->cyclic);
> >+
> >+      return 0;
> >+}
> >+
> > UCLASS_DRIVER(wdt) = {
> >       .id                     = UCLASS_WDT,
> >       .name                   = "watchdog",
> >       .flags                  = DM_UC_FLAG_SEQ_ALIAS,
> >       .pre_probe              = wdt_pre_probe,
> >+      .pre_remove             = wdt_pre_remove,
> >       .per_device_auto        = sizeof(struct wdt_priv),
> > };
>
> _______________________________________________
> Concept mailing list -- concept@u-boot.org
> To unsubscribe send an email to concept-leave@u-boot.org
diff mbox series

Patch

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index 10be334e9ed..c2d19530b3d 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -256,10 +256,21 @@  static int wdt_pre_probe(struct udevice *dev)
 	return 0;
 }
 
+static int wdt_pre_remove(struct udevice *dev)
+{
+	struct wdt_priv *priv = dev_get_uclass_priv(dev);
+
+	if (IS_ENABLED(CONFIG_WATCHDOG) && priv->running)
+		cyclic_unregister(&priv->cyclic);
+
+	return 0;
+}
+
 UCLASS_DRIVER(wdt) = {
 	.id			= UCLASS_WDT,
 	.name			= "watchdog",
 	.flags			= DM_UC_FLAG_SEQ_ALIAS,
 	.pre_probe		= wdt_pre_probe,
+	.pre_remove		= wdt_pre_remove,
 	.per_device_auto	= sizeof(struct wdt_priv),
 };