Re: [PATCH] watchdog: Differentiate scenarios when watchdog is closed

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] If `wdd->ops->stop()` fails, `WDOG_HW_RUNNING` is never restored, bypassing module unload protections and breaking fallback keepalive pings, leading to a Use-After-Free.
--

commit aaf821e8a7620610688b1a164fc083439874422c
Author: Charles Haithcock <chaithco@redhat.com>

watchdog: Differentiate scenarios when watchdog is closed

This patch adds an error message when the watchdog device fails to stop in
the device-specific code paths. This helps differentiate between scenarios
where the watchdog was intentionally left running (such as when nowayout is
set) versus when it simply failed to close.

> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 9a5e544b886bf..6e3e437b7851f 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -305,6 +305,10 @@ static int watchdog_stop(struct watchdog_device *wdd)
>  	if (wdd->ops->stop) {
>  		clear_bit(WDOG_HW_RUNNING, &wdd->status);
>  		err = wdd->ops->stop(wdd);
> +		if (err < 0) {

[Severity: High]
This is a pre-existing issue, but if wdd->ops->stop() returns an error, is
WDOG_HW_RUNNING left incorrectly cleared?

It appears clear_bit(WDOG_HW_RUNNING, &wdd->status) is called before
wdd->ops->stop(). If the stop callback fails (e.g., an I2C failure in
drivers like ziirave), WDOG_HW_RUNNING remains cleared while WDOG_ACTIVE
remains set.

Could this bypass module unload protections and lead to a Use-After-Free?

If userspace closes the device and watchdog_stop() fails,
watchdog_release() will execute:

drivers/watchdog/watchdog_dev.c:watchdog_release() {
        ...
        if (err < 0) {
                pr_crit("watchdog%d: watchdog did not stop!\n", wdd->id);
                watchdog_ping(wdd);
        }
        ...
}

Because WDOG_HW_RUNNING is cleared, watchdog_hw_running() evaluates to
false and the keepalive watchdog_ping() becomes a no-op.

Later in watchdog_release(), it evaluates:

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);
        }
        ...
}

Since running is false, it drops module references. This permits the
module to be unloaded and watchdog_core_data to be freed, but the
background worker watchdog_update_worker() remains active because
WDOG_ACTIVE is still set. Will this cause a kernel panic when the hrtimer
expires and accesses the freed struct or unloaded module?

> +			pr_err("watchdog%d: Failed to stop watchdog: %pe\n", wdd->id,
> +				ERR_PTR(err));
> +		}
>  		trace_watchdog_stop(wdd, err);
>  	} else {
>  		set_bit(WDOG_HW_RUNNING, &wdd->status);

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