* Re: [PATCH] watchdog: starfive: fix runtime PM usage counter leak in pm_start
2026-08-23 11:22 [PATCH] watchdog: starfive: fix runtime PM usage counter leak in pm_start manushprajwal
@ 2026-08-23 11:33 ` sashiko-bot
2026-08-23 14:45 ` Guenter Roeck
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-08-23 11:33 UTC (permalink / raw)
To: manushprajwal; +Cc: linux-watchdog
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Unprotected hardware access in get_timeleft sysfs callback allows unprivileged local DoS.
- [High] The patch fixes a runtime PM usage counter leak when pm_runtime_get_sync() fails, but leaves an identical leak when the subsequent starfive_wdt_start() fails.
- [High] Unclocked hardware register accesses in suspend, shutdown, and remove paths cause bus faults.
- [High] Leaked runtime PM usage counter and missing pm_runtime_disable() in starfive_wdt_probe() error paths.
- [High] Runtime PM usage counter and clocks are permanently leaked in remove if early_enable is true and the watchdog was never opened.
- [High] The hardware watchdog is not restarted on system resume if early_enable is true but the device was not opened.
--
--- Patch [1]: [PATCH] watchdog: starfive: fix runtime PM usage counter leak in pm_start ---
commit 6f33b73ef6f14abdd5d861e7b791ec5774bc787e
Author: manushprajwal <manushprajwal555@gmail.com>
watchdog: starfive: fix runtime PM usage counter leak in pm_start
This commit fixes a runtime PM usage counter leak in starfive_wdt_pm_start().
It ensures pm_runtime_put_noidle() is called when pm_runtime_get_sync()
returns an error.
> diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
> index af55adc4a3c69..ecb704b2e9a3d 100644
> --- a/drivers/watchdog/starfive-wdt.c
> +++ b/drivers/watchdog/starfive-wdt.c
> @@ -373,8 +373,10 @@ static int starfive_wdt_pm_start(struct watchdog_device *wdd)
> struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> int ret = pm_runtime_get_sync(wdd->parent);
>
> - if (ret < 0)
> + if (ret < 0) {
> + pm_runtime_put_noidle(wdd->parent);
> return ret;
> + }
>
> return starfive_wdt_start(wdt);
> }
[Severity: High]
This is a pre-existing issue, but does this still leak the runtime PM usage
counter if starfive_wdt_start() fails?
If starfive_wdt_start() returns a negative error (for example, if it
times out waiting for an interrupt clear), the error is returned directly.
Since the pm_runtime_get_sync() succeeded, doesn't this return path skip
dropping the PM reference, permanently preventing the device from suspending?
[Severity: High]
This is a pre-existing issue, but do the suspend, remove, and shutdown paths
access hardware registers while the device is suspended?
In starfive_wdt_suspend(), the registers are accessed unconditionally:
drivers/watchdog/starfive-wdt.c:starfive_wdt_suspend() {
...
wdt->reload = starfive_wdt_get_count(wdt);
/* Note that WTCNT doesn't need to be saved. */
starfive_wdt_stop(wdt);
...
}
If the watchdog was not opened by userspace and early_enable is false, the
device resides in a runtime-suspended state with clocks disabled. Could
accessing its registers in this state trigger a synchronous external abort
(bus fault)? Similarly, the remove and shutdown paths also call
starfive_wdt_stop() directly.
[Severity: High]
This is a pre-existing issue, but do the error paths in starfive_wdt_probe()
leak the runtime PM usage counter or miss disabling PM?
If pm_runtime_resume_and_get() fails:
drivers/watchdog/starfive-wdt.c:starfive_wdt_probe() {
...
ret = pm_runtime_resume_and_get(&pdev->dev);
if (ret < 0)
return ret;
...
}
Should this call pm_runtime_disable() before returning?
Additionally, if a subsequent step fails, it jumps to err_exit:
drivers/watchdog/starfive-wdt.c:starfive_wdt_probe() {
...
err_exit:
starfive_wdt_disable_clock(wdt);
pm_runtime_disable(&pdev->dev);
return ret;
}
Does this path need to call pm_runtime_put_noidle() to drop the usage
counter before disabling PM?
[Severity: High]
This is a pre-existing issue, but does starfive_wdt_remove() leak the
runtime PM reference and clocks if early_enable is true?
If early_enable is set, the probe function leaves the PM usage count at 1.
If userspace never opens the watchdog, WDOG_ACTIVE remains false.
During remove:
drivers/watchdog/starfive-wdt.c:starfive_wdt_remove() {
...
if (pm_runtime_enabled(&pdev->dev))
pm_runtime_disable(&pdev->dev);
...
}
Since watchdog_unregister_device() bypasses the stop callback when
WDOG_ACTIVE is false, and remove() only disables runtime PM without dropping
the usage counter, does this prevent the device from ever suspending and
leave the clocks running indefinitely?
[Severity: High]
This is a pre-existing issue, but does starfive_wdt_resume() fail to
restart the hardware if early_enable is true but the device is unopened?
drivers/watchdog/starfive-wdt.c:starfive_wdt_resume() {
...
if (watchdog_active(&wdt->wdd))
return starfive_wdt_start(wdt);
return 0;
}
If the watchdog was started via early_enable but not opened by userspace,
watchdog_active() is false. Since starfive_wdt_suspend() unconditionally
stops the hardware, does this leave the system without hardware watchdog
protection after resuming?
[Severity: Critical]
This is a pre-existing issue, but does starfive_wdt_get_timeleft()
allow an unprivileged local denial of service?
drivers/watchdog/starfive-wdt.c:starfive_wdt_get_timeleft() {
...
count = starfive_wdt_get_count(wdt);
...
}
This function is called when a user reads the 0444 sysfs attribute
timeleft. If the watchdog is closed and the device is runtime-suspended,
its clocks are disabled. Could an unprivileged user reading this sysfs
attribute cause an immediate bus fault (kernel panic) by accessing
unclocked registers?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260823112225.1107-1-manushprajwal555@gmail.com?part=1
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] watchdog: starfive: fix runtime PM usage counter leak in pm_start
2026-08-23 11:22 [PATCH] watchdog: starfive: fix runtime PM usage counter leak in pm_start manushprajwal
2026-08-23 11:33 ` sashiko-bot
@ 2026-08-23 14:45 ` Guenter Roeck
1 sibling, 0 replies; 3+ messages in thread
From: Guenter Roeck @ 2026-08-23 14:45 UTC (permalink / raw)
To: manushprajwal, xingyu.wu, ziv.xu; +Cc: wim, linux-watchdog
On 8/23/26 04:22, manushprajwal wrote:
> starfive_wdt_pm_start() calls pm_runtime_get_sync() and returns its
> error code directly on failure. Per Documentation/power/runtime_pm.rst,
> pm_runtime_get_sync() does not drop the usage counter on error, so the
> caller is expected to release the reference itself. The early return
> here skips that, leaking a runtime PM usage count on wdd->parent on
> every failed resume, which prevents the device from being suspended
> correctly afterwards.
>
> Fix it by calling pm_runtime_put_noidle() before returning, mirroring
> the same balance the counterpart starfive_wdt_pm_stop() already
> maintains with pm_runtime_put_sync().
>
> Signed-off-by: manushprajwal <manushprajwal555@gmail.com>
> ---
> drivers/watchdog/starfive-wdt.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
> index af55adc4a..ecb704b2e 100644
> --- a/drivers/watchdog/starfive-wdt.c
> +++ b/drivers/watchdog/starfive-wdt.c
> @@ -373,8 +373,10 @@ static int starfive_wdt_pm_start(struct watchdog_device *wdd)
> struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> int ret = pm_runtime_get_sync(wdd->parent);
>
> - if (ret < 0)
> + if (ret < 0) {
> + pm_runtime_put_noidle(wdd->parent);
> return ret;
> + }
>
> return starfive_wdt_start(wdt);
> }
As Sashiko points out, there is still a runtime PM leak if starfive_wdt_start()
fails (plus all the other issues in the driver, but those can be fixed with
separate patches).
Thanks,
Guenter
^ permalink raw reply [flat|nested] 3+ messages in thread