Re: [PATCH] watchdog: fix hrtimer start when pretimeout is zero

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] A race condition during watchdog device unregistration (`watchdog_cdev_unregister`) allows the pretimeout timer to be concurrently re-armed after it is stopped, leading to a Use-After-Free or NULL pointer dereference.
- [High] Missing lock acquisition during teardown causes data races with background operations, potentially leading to hardware corruption.
--

commit 04d7e6cb40777d8a3872c4dac3b1dddfdfe115f8
Author: David Arcari <darcari@redhat.com>

watchdog: fix hrtimer start when pretimeout is zero

Per the watchdog API, a pretimeout value of 0 disables the feature.
However, watchdog_hrtimer_pretimeout_start() fails to verify if the
pretimeout is non-zero before arming the timer.

This omission inadvertently starts the software pretimeout timer,
which could result in the pretimeout handler executing incorrectly
when the watchdog timeout is reached.

Fix this by adding a check for wdd->pretimeout before calling
hrtimer_start(), ensuring the disabled state is respected.

Fixes: 7b7d2fdc8c3e ("watchdog: Add hrtimer-based pretimeout feature")
Signed-off-by: David Arcari <darcari@redhat.com>

> diff --git a/drivers/watchdog/watchdog_hrtimer_pretimeout.c b/drivers/watchdog/watchdog_hrtimer_pretimeout.c
> index fbc7eecd8b203..49a05ea60c979 100644
> --- a/drivers/watchdog/watchdog_hrtimer_pretimeout.c
> +++ b/drivers/watchdog/watchdog_hrtimer_pretimeout.c
> @@ -30,6 +30,7 @@ void watchdog_hrtimer_pretimeout_init(struct watchdog_device *wdd)
>  void watchdog_hrtimer_pretimeout_start(struct watchdog_device *wdd)
>  {
>  	if (!(wdd->info->options & WDIOF_PRETIMEOUT) &&
> +	    wdd->pretimeout &&
>  	    !watchdog_pretimeout_invalid(wdd, wdd->pretimeout))
>  		hrtimer_start(&wdd->wd_data->pretimeout_timer,
>  			      ktime_set(wdd->timeout - wdd->pretimeout, 0),

[Severity: Critical]
This is a pre-existing issue, but since this function is used to arm the
hrtimer, can a concurrent device unregistration race with a ping to cause a
null pointer dereference?

Looking at drivers/watchdog/watchdog_dev.c:watchdog_cdev_unregister(), the
timer is canceled before acquiring wd_data->lock:

    watchdog_hrtimer_pretimeout_stop(wdd);

    mutex_lock(&wd_data->lock);
    wd_data->wdd = NULL;
    wdd->wd_data = NULL;
    mutex_unlock(&wd_data->lock);

If a background ping worker or a concurrent userspace write operation
acquires the lock before unregistration sets wd_data->wdd to NULL, couldn't
it observe a valid wdd and call __watchdog_ping()?

This would result in watchdog_hrtimer_pretimeout_start() being called to
re-arm the timer asynchronously. Since the unregister path never checks or
cancels the timer again, when the timer fires, its callback will execute
watchdog_notify_pretimeout(wd_data->wdd) with a null or freed wdd pointer.
Does this lead to a crash?

[Severity: High]
This is a pre-existing issue, but I also noticed a potential locking problem
just above the timer stop in drivers/watchdog/watchdog_dev.c
in the watchdog_cdev_unregister() function:

    if (watchdog_active(wdd) &&
        test_bit(WDOG_STOP_ON_UNREGISTER, &wdd->status)) {
        watchdog_stop(wdd);
    }

Does calling watchdog_stop() without holding wd_data->lock risk data races
and hardware corruption? If background ping workers or ioctl paths
concurrently issue pings and modify state flags while this runs unprotected,
could it lead to inconsistent hardware states?

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