Linux Watchdog driver development
 help / color / mirror / Atom feed
* [PATCH] watchdog: pretimeout: Add "dump" pretimeout governor
@ 2026-07-07  7:31 Tzung-Bi Shih
  2026-07-07  8:02 ` sashiko-bot
  0 siblings, 1 reply; 3+ messages in thread
From: Tzung-Bi Shih @ 2026-07-07  7:31 UTC (permalink / raw)
  To: Wim Van Sebroeck, Guenter Roeck
  Cc: tfiga, tzungbi, sboyd, dianders, linux-watchdog, linux-kernel

Add a new "dump" pretimeout governor that triggers a backtrace of all
CPUs (via trigger_all_cpu_backtrace()) to the kernel log buffer.  This
provides diagnostic information right before the hardware watchdog
fires.

In addition, update the WATCHDOG_PRETIMEOUT_GOV_SEL Kconfig logic to
fall back to the "panic" governor only when both "noop" and "dump"
governors are disabled.

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
Alternatives considered:
We can also introduce some options and put trigger_all_cpu_backtrace()
into the existing "noop" governor.
---
 drivers/watchdog/Kconfig               | 19 ++++++++++-
 drivers/watchdog/Makefile              |  1 +
 drivers/watchdog/pretimeout_dump.c     | 45 ++++++++++++++++++++++++++
 drivers/watchdog/watchdog_pretimeout.h |  4 ++-
 4 files changed, 67 insertions(+), 2 deletions(-)
 create mode 100644 drivers/watchdog/pretimeout_dump.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 08cb8612d41f..7b291ea8b0f0 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -93,10 +93,19 @@ config WATCHDOG_PRETIMEOUT_GOV_SEL
 	tristate
 	depends on WATCHDOG_PRETIMEOUT_GOV
 	default m
-	select WATCHDOG_PRETIMEOUT_GOV_PANIC if WATCHDOG_PRETIMEOUT_GOV_NOOP=n
+	select WATCHDOG_PRETIMEOUT_GOV_PANIC if \
+		WATCHDOG_PRETIMEOUT_GOV_NOOP=n && WATCHDOG_PRETIMEOUT_GOV_DUMP=n
 
 if WATCHDOG_PRETIMEOUT_GOV
 
+config WATCHDOG_PRETIMEOUT_GOV_DUMP
+	tristate "Dump watchdog pretimeout governor"
+	depends on WATCHDOG_CORE
+	default WATCHDOG_CORE
+	help
+	  Dump watchdog pretimeout governor, all cpu backtrace is
+	  added to kernel log buffer.
+
 config WATCHDOG_PRETIMEOUT_GOV_NOOP
 	tristate "Noop watchdog pretimeout governor"
 	depends on WATCHDOG_CORE
@@ -121,6 +130,14 @@ choice
 	  The governor takes its action, if a watchdog is capable
 	  to report a pretimeout event.
 
+config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_DUMP
+	bool "dump"
+	depends on WATCHDOG_PRETIMEOUT_GOV_DUMP
+	help
+	  Use dump watchdog pretimeout governor by default. If dump
+	  governor is selected by a user, dump all cpu backtrace to
+	  the kernel log buffer and don't do any system changes.
+
 config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_NOOP
 	bool "noop"
 	depends on WATCHDOG_PRETIMEOUT_GOV_NOOP
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index bc1d52220f22..598556f03bc3 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -11,6 +11,7 @@ watchdog-objs	+= watchdog_core.o watchdog_dev.o
 watchdog-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV)	+= watchdog_pretimeout.o
 watchdog-$(CONFIG_WATCHDOG_HRTIMER_PRETIMEOUT)	+= watchdog_hrtimer_pretimeout.o
 
+obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_DUMP)	+= pretimeout_dump.o
 obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_NOOP)	+= pretimeout_noop.o
 obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_PANIC)	+= pretimeout_panic.o
 
diff --git a/drivers/watchdog/pretimeout_dump.c b/drivers/watchdog/pretimeout_dump.c
new file mode 100644
index 000000000000..c5d3dac2606c
--- /dev/null
+++ b/drivers/watchdog/pretimeout_dump.c
@@ -0,0 +1,45 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2026 Google LLC
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/nmi.h>
+#include <linux/watchdog.h>
+
+#include "watchdog_pretimeout.h"
+
+/**
+ * pretimeout_dump - Dump on watchdog pretimeout event
+ * @wdd: watchdog_device
+ *
+ * Dump all cpu backtrace on pretimeout event.
+ */
+static void pretimeout_dump(struct watchdog_device *wdd)
+{
+	pr_alert("watchdog%d: pretimeout event\n", wdd->id);
+	if (!trigger_all_cpu_backtrace())
+		pr_alert("trigger_all_cpu_backtrace() isn't available\n");
+}
+
+static struct watchdog_governor watchdog_gov_dump = {
+	.name		= "dump",
+	.pretimeout	= pretimeout_dump,
+};
+
+static int __init watchdog_gov_dump_register(void)
+{
+	return watchdog_register_governor(&watchdog_gov_dump);
+}
+
+static void __exit watchdog_gov_dump_unregister(void)
+{
+	watchdog_unregister_governor(&watchdog_gov_dump);
+}
+module_init(watchdog_gov_dump_register);
+module_exit(watchdog_gov_dump_unregister);
+
+MODULE_AUTHOR("Tzung-Bi Shih <tzungbi@kernel.org>");
+MODULE_DESCRIPTION("Dump watchdog pretimeout governor");
+MODULE_LICENSE("GPL");
diff --git a/drivers/watchdog/watchdog_pretimeout.h b/drivers/watchdog/watchdog_pretimeout.h
index a3f1abc68839..57aa790e2669 100644
--- a/drivers/watchdog/watchdog_pretimeout.h
+++ b/drivers/watchdog/watchdog_pretimeout.h
@@ -24,7 +24,9 @@ int watchdog_pretimeout_governor_get(struct watchdog_device *wdd, char *buf);
 int watchdog_pretimeout_governor_set(struct watchdog_device *wdd,
 				     const char *buf);
 
-#if IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_DEFAULT_GOV_NOOP)
+#if IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_DEFAULT_GOV_DUMP)
+#define WATCHDOG_PRETIMEOUT_DEFAULT_GOV		"dump"
+#elif IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_DEFAULT_GOV_NOOP)
 #define WATCHDOG_PRETIMEOUT_DEFAULT_GOV		"noop"
 #elif IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_DEFAULT_GOV_PANIC)
 #define WATCHDOG_PRETIMEOUT_DEFAULT_GOV		"panic"
-- 
2.55.0.rc2.803.g1fd1e6609c-goog


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] watchdog: pretimeout: Add "dump" pretimeout governor
  2026-07-07  7:31 [PATCH] watchdog: pretimeout: Add "dump" pretimeout governor Tzung-Bi Shih
@ 2026-07-07  8:02 ` sashiko-bot
  2026-07-07 10:28   ` Tzung-Bi Shih
  0 siblings, 1 reply; 3+ messages in thread
From: sashiko-bot @ 2026-07-07  8:02 UTC (permalink / raw)
  To: Tzung-Bi Shih; +Cc: linux-watchdog

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] Compiling the new 'dump' pretimeout governor as a loadable module causes a modpost build failure because `trigger_all_cpu_backtrace()` relies on an unexported symbol.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but unregistering the default watchdog pretimeout governor leaves dangling pointers in `default_gov` and device structs, leading to a Use-After-Free (UAF) upon the next pretimeout event or sysfs read.
--

--- Patch [1]: [PATCH] watchdog: pretimeout: Add "dump" pretimeout governor ---
commit dc8900cef0c7ac20c37ff84ef6166ddde30f5da7
Author: Tzung-Bi Shih <tzungbi@kernel.org>

watchdog: pretimeout: Add "dump" pretimeout governor

This patch adds a new "dump" pretimeout governor that triggers a backtrace
of all CPUs to the kernel log buffer before the watchdog fires. It also
updates the Kconfig logic to fall back to the "panic" governor only when
both "noop" and "dump" governors are disabled.

> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 08cb8612d41fe..7b291ea8b0f01 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig

[ ... ]

> @@ -93,10 +93,19 @@ config WATCHDOG_PRETIMEOUT_GOV_SEL
>  
>  if WATCHDOG_PRETIMEOUT_GOV
>  
> +config WATCHDOG_PRETIMEOUT_GOV_DUMP
> +	tristate "Dump watchdog pretimeout governor"

[Severity: Low]
Will this fail to build if compiled as a module?

When CONFIG_WATCHDOG_PRETIMEOUT_GOV_DUMP=m, trigger_all_cpu_backtrace() can
rely on unexported architecture-specific functions (such as
arch_trigger_cpumask_backtrace() on x86, ARM, and ARM64), which causes a
modpost build failure.

> +	depends on WATCHDOG_CORE
> +	default WATCHDOG_CORE
> +	help

[ ... ]

> diff --git a/drivers/watchdog/pretimeout_dump.c b/drivers/watchdog/pretimeout_dump.c
> new file mode 100644
> index 0000000000000..c5d3dac2606c4
> --- /dev/null
> +++ b/drivers/watchdog/pretimeout_dump.c

[ ... ]

> +static void __exit watchdog_gov_dump_unregister(void)
> +{
> +	watchdog_unregister_governor(&watchdog_gov_dump);
> +}

[Severity: High]
This is a pre-existing issue, but does unregistering the default watchdog
pretimeout governor leave dangling pointers?

If this module is set as the default governor and then unloaded,
watchdog_unregister_governor() attempts to reset the active governors:

    list_for_each_entry(p, &pretimeout_list, entry)
        if (p->wdd->gov == gov)
            p->wdd->gov = default_gov;

If the unregistered governor is the default governor, default_gov equals
gov. This leaves p->wdd->gov pointing to the unloaded module's struct.

Could this lead to a use-after-free upon the next pretimeout event or sysfs
read?

> +module_init(watchdog_gov_dump_register);
> +module_exit(watchdog_gov_dump_unregister);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707073117.2962676-1-tzungbi@kernel.org?part=1

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] watchdog: pretimeout: Add "dump" pretimeout governor
  2026-07-07  8:02 ` sashiko-bot
@ 2026-07-07 10:28   ` Tzung-Bi Shih
  0 siblings, 0 replies; 3+ messages in thread
From: Tzung-Bi Shih @ 2026-07-07 10:28 UTC (permalink / raw)
  To: sashiko-reviews; +Cc: linux-watchdog

On Tue, Jul 07, 2026 at 08:02:24AM +0000, sashiko-bot@kernel.org wrote:
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 08cb8612d41fe..7b291ea8b0f01 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> 
> [ ... ]
> 
> > @@ -93,10 +93,19 @@ config WATCHDOG_PRETIMEOUT_GOV_SEL
> >  
> >  if WATCHDOG_PRETIMEOUT_GOV
> >  
> > +config WATCHDOG_PRETIMEOUT_GOV_DUMP
> > +	tristate "Dump watchdog pretimeout governor"
> 
> [Severity: Low]
> Will this fail to build if compiled as a module?
> 
> When CONFIG_WATCHDOG_PRETIMEOUT_GOV_DUMP=m, trigger_all_cpu_backtrace() can
> rely on unexported architecture-specific functions (such as
> arch_trigger_cpumask_backtrace() on x86, ARM, and ARM64), which causes a
> modpost build failure.

Use a bool instead in v2 [1].

[1] https://lore.kernel.org/all/20260707102105.3600275-1-tzungbi@kernel.org

> > diff --git a/drivers/watchdog/pretimeout_dump.c b/drivers/watchdog/pretimeout_dump.c
> > new file mode 100644
> > index 0000000000000..c5d3dac2606c4
> > --- /dev/null
> > +++ b/drivers/watchdog/pretimeout_dump.c
> 
> [ ... ]
> 
> > +static void __exit watchdog_gov_dump_unregister(void)
> > +{
> > +	watchdog_unregister_governor(&watchdog_gov_dump);
> > +}
> 
> [Severity: High]
> This is a pre-existing issue, but does unregistering the default watchdog
> pretimeout governor leave dangling pointers?
> 
> If this module is set as the default governor and then unloaded,
> watchdog_unregister_governor() attempts to reset the active governors:
> 
>     list_for_each_entry(p, &pretimeout_list, entry)
>         if (p->wdd->gov == gov)
>             p->wdd->gov = default_gov;
> 
> If the unregistered governor is the default governor, default_gov equals
> gov. This leaves p->wdd->gov pointing to the unloaded module's struct.
> 
> Could this lead to a use-after-free upon the next pretimeout event or sysfs
> read?

Proposed a fix in [2].

[2] https://lore.kernel.org/all/20260707101803.3598173-1-tzungbi@kernel.org

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-07-07 10:28 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-07  7:31 [PATCH] watchdog: pretimeout: Add "dump" pretimeout governor Tzung-Bi Shih
2026-07-07  8:02 ` sashiko-bot
2026-07-07 10:28   ` Tzung-Bi Shih

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox