Re: [PATCH] Differentiate scenarios when watchdog is closed

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch inadvertently omits the keepalive ping when a watchdog device is closed without the magic close character, altering expected timeout behavior.

Pre-existing issues:
- [High] If the hardware stop operation fails, the `WDOG_HW_RUNNING` bit is incorrectly left cleared, leading to a premature module reference release and a subsequent Use-After-Free kernel panic.
- [High] Unbalanced module_put() in the error path of watchdog_open() if the hardware is already running, which can lead to a Use-After-Free.
- [High] watchdog_open() calls watchdog_start() without holding wd_data->lock, creating a data race with the background ping worker.
--

--- Patch [1]: [PATCH] Differentiate scenarios when watchdog is closed ---
commit a2b7eb1d1568deccc7f35e939965c301a7ed7c1c
Author: Charles Haithcock <chaithco@redhat.com>

Differentiate scenarios when watchdog is closed

This commit updates the watchdog closing logic to print a specific message
when the watchdog is closed unexpectedly while still running. It separates
this scenario from the failure to stop when nowayout is set or when the
device fails to stop in the hardware driver.

> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 9a5e544b886bf..72f687aee067a 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -955,14 +955,17 @@ static int watchdog_release(struct inode *inode, struct file *file)
>  	if (!watchdog_active(wdd))
>  		err = 0;
>  	else if (test_and_clear_bit(_WDOG_ALLOW_RELEASE, &wd_data->status) ||
> -		 !(wdd->info->options & WDIOF_MAGICCLOSE))
> +		 !(wdd->info->options & WDIOF_MAGICCLOSE)) {
>  		err = watchdog_stop(wdd);
>  
> -	/* If the watchdog was not stopped, send a keepalive ping */
> -	if (err < 0) {
> -		pr_crit("watchdog%d: watchdog did not stop!\n", wdd->id);
> -		watchdog_ping(wdd);
> +		/* If the watchdog was not stopped, send a keepalive ping */
> +		if (err < 0) {
> +			pr_crit("watchdog%d: watchdog did not stop!\n", wdd->id);
> +			watchdog_ping(wdd);
> +		}
>  	}
> +	else
> +		pr_info("watchdog%d: closing while running!\n", wdd->id);

[Severity: High]
By nesting the keepalive ping logic inside the else-if block, does this skip
sending the keepalive ping when the watchdog is closed unexpectedly (without
the magic character)?

This seems to remove the watchdog's ability to reset its hardware timer on an
unexpected close, which could cause the system to reboot based on the last
explicit ping time rather than providing a full timeout window for recovery.

>  
>  	watchdog_update_worker(wdd);
>  

[Severity: High]
This is a pre-existing issue, but if the hardware stop operation fails, is
the WDOG_HW_RUNNING bit incorrectly left cleared?

Looking at watchdog_stop():

drivers/watchdog/watchdog_dev.c:watchdog_stop() {
	if (wdd->ops->stop) {
		clear_bit(WDOG_HW_RUNNING, &wdd->status);
		err = wdd->ops->stop(wdd);
		trace_watchdog_stop(wdd, err);
	} else {
...
}

If wdd->ops->stop() fails, err is returned but WDOG_HW_RUNNING is never
restored.

Later in watchdog_release():

drivers/watchdog/watchdog_dev.c:watchdog_release() {
...
	running = wdd && watchdog_hw_running(wdd);
	mutex_unlock(&wd_data->lock);

	if (!running) {
		module_put(wd_data->cdev.owner);
		put_device(&wd_data->dev);
	}
...
}

Would this cause the module reference to be dropped erroneously, allowing the
module to be unloaded while the hardware is still running, and potentially
leading to a use-after-free when the background worker fires?

[Severity: High]
This is a pre-existing issue, but is there an unbalanced module_put() in the
error path of watchdog_open() if the hardware is already running?

drivers/watchdog/watchdog_dev.c:watchdog_open() {
	hw_running = watchdog_hw_running(wdd);
	if (!hw_running && !try_module_get(wdd->ops->owner)) {
		err = -EBUSY;
		goto out_clear;
	}

	err = watchdog_start(wdd);
	if (err < 0)
		goto out_mod;
...
out_mod:
	module_put(wd_data->wdd->ops->owner);
}

If hw_running is true, try_module_get() is skipped. If watchdog_start() then
fails, execution jumps to out_mod and calls module_put(). Could this drop a
module reference that was never acquired, leading to a premature unload and a
use-after-free?

[Severity: High]
This is a pre-existing issue, but does watchdog_open() call watchdog_start()
without holding wd_data->lock?

drivers/watchdog/watchdog_dev.c:watchdog_open() {
...
	err = watchdog_start(wdd);
	if (err < 0)
		goto out_mod;
...
}

If the watchdog hardware is running before being opened, the background ping
worker may execute concurrently. Does this lack of synchronization allow data
races with the worker that can corrupt internal states?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824205051.4117285-2-chaithco@redhat.com?part=1