Re: [PATCH] watchdog: pretimeout: Add "dump" pretimeout governor

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 <[email protected]>

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/[email protected]?part=1