* Re: [PATCH v1] watchdog: unregister PM notifier on watchdog unregister
2026-06-01 19:20 [PATCH v1] watchdog: unregister PM notifier on watchdog unregister Yuho Choi
@ 2026-06-01 19:32 ` sashiko-bot
2026-06-08 18:57 ` Guenter Roeck
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-06-01 19:32 UTC (permalink / raw)
To: Yuho Choi; +Cc: linux-watchdog
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Ignored errors during deferred watchdog registration and unhandled failure paths lead to Use-After-Free and NULL pointer dereferences upon unregistration.
- [High] Lockless access to `wd_data->wdd` in `watchdog_open()` leads to a Use-After-Free and violates the `watchdog_start()` locking contract.
--
commit 7739691e12f24218c1001ecfc408a279156feca5
Author: Yuho Choi <dbgh9129@gmail.com>
watchdog: unregister PM notifier on watchdog unregister
This commit ensures the PM notifier is unregistered before tearing down the
watchdog device. Previously, if WDOG_NO_PING_ON_SUSPEND was set, the embedded
notifier block would be left on the PM notifier chain after unregistration,
potentially leading to calls with a stale watchdog_device pointer.
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index 8300520688d07..6c087b4ef5dd5 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -391,6 +391,9 @@ static void __watchdog_unregister_device(struct watchdog_device *wdd)
> if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status))
> unregister_reboot_notifier(&wdd->reboot_nb);
>
> + if (test_bit(WDOG_NO_PING_ON_SUSPEND, &wdd->status))
> + unregister_pm_notifier(&wdd->pm_nb);
> +
> watchdog_dev_unregister(wdd);
[Severity: High]
This is a pre-existing issue, but I noticed that ignored errors during
deferred watchdog registration and unhandled failure paths can lead to a
Use-After-Free and NULL pointer dereferences upon unregistration.
When a watchdog driver registers early, watchdog_register_device() adds it to
wtd_deferred_reg_list and returns 0. Later, watchdog_deferred_registration()
processes the list:
drivers/watchdog/watchdog_core.c:watchdog_deferred_registration() {
...
list_del(&wdd->deferred);
__watchdog_register_device(wdd);
}
mutex_unlock(&wtd_deferred_reg_mutex);
}
The return value of __watchdog_register_device() is ignored here, concealing
fatal failures from drivers. If watchdog_cdev_register() fails, the error
paths free wd_data via put_device() but fail to clear wdd->wd_data:
drivers/watchdog/watchdog_dev.c:watchdog_cdev_register() {
...
if (err) {
pr_err("watchdog%d unable to add device %d:%d\n",
wdd->id, MAJOR(watchdog_devt), wdd->id);
...
put_device(&wd_data->dev);
return err;
}
...
}
When the driver is later unbound, it calls watchdog_unregister_device(), which
invokes watchdog_cdev_unregister(). This function unconditionally dereferences
wdd->wd_data:
drivers/watchdog/watchdog_dev.c:watchdog_cdev_unregister() {
...
struct watchdog_core_data *wd_data = wdd->wd_data;
cdev_device_del(&wd_data->cdev, &wd_data->dev);
...
}
Could this lead to a NULL pointer dereference or a Use-After-Free if deferred
registration fails, since wdd->wd_data was not cleared?
[Severity: High]
This is a pre-existing issue, but lockless access to wd_data->wdd in
watchdog_open() appears to lead to a Use-After-Free and violates the
watchdog_start() locking contract.
watchdog_open() fetches wdd = wd_data->wdd locklessly, failing to acquire
wd_data->lock to synchronize with concurrent teardown operations:
drivers/watchdog/watchdog_dev.c:watchdog_open() {
...
wdd = wd_data->wdd;
/*
* If the /dev/watchdog device is open, we don't want the module
* to be unloaded.
*/
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 a device unbind or hot-unplug occurs concurrently,
watchdog_cdev_unregister() acquires wd_data->lock, sets wd_data->wdd = NULL,
and unlocks:
drivers/watchdog/watchdog_dev.c:watchdog_cdev_unregister() {
...
mutex_lock(&wd_data->lock);
wd_data->wdd = NULL;
wdd->wd_data = NULL;
mutex_unlock(&wd_data->lock);
...
}
Does this allow watchdog_open() to dereference a freed wdd pointer if the
hardware driver finishes its remove() callback and frees wdd? Additionally,
is the locking contract for watchdog_start() violated since wd_data->lock
is not held?
> ida_free(&watchdog_ida, wdd->id);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260601192005.1970805-1-dbgh9129@gmail.com?part=1
^ permalink raw reply [flat|nested] 3+ messages in thread