Re: [PATCH 6/6] watchdog: tegra_wdt: Stop on Tegra20 VDE power off

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] watchdog: tegra_wdt: refcount leak in
  tegra20_wdt_powergate_notify()
- [Medium] watchdog: tegra_wdt: stale WDOG_ACTIVE after
  tegra20_wdt_powergate_notify()
- [Medium] watchdog: tegra_wdt: race on WDOG_HW_RUNNING in
  tegra20_wdt_powergate_notify()
--

commit 7bc831b226455fa0dc8d2f0743ce747806275870
Author: Artur Kowalski <arturkow2000@gmail.com>

watchdog: tegra_wdt: Stop on Tegra20 VDE power off

This commit registers a Tegra PMC powergate notifier on Tegra20 to stop the
watchdog before the VDE partition is powered off and prevent it from being
started afterwards.

> diff --git a/drivers/watchdog/tegra_wdt.c b/drivers/watchdog/tegra_wdt.c
> index 7c2c21f90de52..d36310c50f782 100644
> --- a/drivers/watchdog/tegra_wdt.c
> +++ b/drivers/watchdog/tegra_wdt.c

[ ... ]

> @@ -256,6 +276,35 @@ static int tegra20_wdt_stop(struct watchdog_device *wdd)
>  	writel(0, wdt->tmr_regs + TIMER_PTV);
>  	writel(TIMER_PCR_INTR_CLR, wdt->tmr_regs + TIMER_PCR);
> +}
> +
> +static int tegra20_wdt_start(struct watchdog_device *wdd)
> +{
> +	struct tegra_wdt *wdt = watchdog_get_drvdata(wdd);
> +	int ret = 0;
> +
> +	mutex_lock(&wdt->lock);
> +
> +	if (wdt->vde_off) {
> +		dev_warn(wdd->parent,
> +			 "not starting, a reset would hang with VDE powered off\n");
> +		ret = -EBUSY;
> +	} else {
> +		__tegra20_wdt_start(wdt);
> +	}
> +
> +	mutex_unlock(&wdt->lock);
> +
> +	return ret;
> +}

[Severity: Medium]
Can tegra20_wdt_powergate_notify() race with watchdog_open() ->
watchdog_start() on WDOG_HW_RUNNING and WDOG_ACTIVE?

tegra20_wdt_powergate_notify() serializes on wdt->lock, whereas
watchdog_open() and watchdog_start() inspect and update wdd->status under
wd_data->lock:

CPU0                                  CPU1
watchdog_open()
  watchdog_start()
    tegra20_wdt_start()
      mutex_lock(&wdt->lock)
      __tegra20_wdt_start(wdt)
      mutex_unlock(&wdt->lock)
                                      tegra20_wdt_powergate_notify()
                                        mutex_lock(&wdt->lock)
                                        wdt->vde_off = true
                                        __tegra20_wdt_stop(wdt)
                                        clear_bit(WDOG_HW_RUNNING, ...)
                                        mutex_unlock(&wdt->lock)
    set_bit(WDOG_ACTIVE, ...)
    set_bit(WDOG_HW_RUNNING, ...)  <-- set after watchdog was stopped

This leaves WDOG_ACTIVE and WDOG_HW_RUNNING set and allows open() to
succeed after the VDE partition was powered off and the hardware timer was
stopped.

If WDOG_HW_RUNNING was already set at boot, a similar race in
watchdog_open() either calls __watchdog_ping() -> tegra20_wdt_ping() (which
does not check wdt->vde_off and returns 0) or sees WDOG_HW_RUNNING cleared
before watchdog_start() and calls module_put() at out_mod without a
matching try_module_get() in watchdog_open().

> @@ -296,6 +350,45 @@ static const struct watchdog_ops tegra20_wdt_ops = {
>  	.set_timeout = tegra20_wdt_set_timeout,
>  };
>  
> +static int tegra20_wdt_powergate_notify(struct notifier_block *nb,
> +					unsigned long event, void *data)
> +{
> +	struct tegra_wdt *wdt = container_of(nb, struct tegra_wdt,
> +					     powergate_nb);
> +	unsigned int id = *(unsigned int *)data;
> +
> +	if (event != TEGRA_PMC_POWERGATE_POWER_OFF ||
> +	    id != TEGRA_POWERGATE_VDEC)
> +		return NOTIFY_DONE;
> +
> +	mutex_lock(&wdt->lock);
> +
> +	if (!wdt->vde_off) {
> +		wdt->vde_off = true;
> +		__tegra20_wdt_stop(wdt);
> +
> +		/*
> +		 * Stop the core's pings. If user space has the device open,
> +		 * its pings don't restart the watchdog either.
> +		 */
> +		clear_bit(WDOG_HW_RUNNING, &wdt->wdd.status);

[Severity: Medium]
Does clearing only WDOG_HW_RUNNING here leave WDOG_ACTIVE set when
/dev/watchdog is already open?

If user space later closes /dev/watchdog without the magic close character
(or with nowayout enabled), watchdog_release() leaves WDOG_ACTIVE set while
dropping the module and device references because WDOG_HW_RUNNING is false:

drivers/watchdog/watchdog_dev.c:watchdog_release() {
    ...
done:
	running = wdd && watchdog_hw_running(wdd);
	mutex_unlock(&wd_data->lock);
	if (!running) {
		module_put(wd_data->cdev.owner);
		put_device(&wd_data->dev);
	}
	return 0;
}

On a subsequent open(/dev/watchdog), watchdog_start() sees
watchdog_active(wdd) == true and returns 0 immediately without calling
tegra20_wdt_start(), so opening the stopped watchdog succeeds instead of
failing with -EBUSY.  While the file remains open, WDIOC_SETTIMEOUT also
sees watchdog_active(wdd) in tegra20_wdt_set_timeout(), updates
wdd->timeout, and then fails with -EBUSY in tegra20_wdt_start().

> +
> +		dev_info(wdt->wdd.parent,
> +			 "VDE powered off, watchdog stopped\n");
> +	}
> +
> +	mutex_unlock(&wdt->lock);
> +
> +	return NOTIFY_OK;
> +}

[ ... ]

> @@ -326,18 +419,46 @@ static int tegra20_wdt_init(struct platform_device *pdev, struct tegra_wdt *wdt,
>  	/*
>  	 * The bootloader may leave the watchdog running. Take it over with
>  	 * our own timeout and timer 1, and let the core ping it until user
>  	 * space opens the device.
>  	 */
> -	if (readl(wdt->rst_source) & T20_RST_SOURCE_WDT_EN) {
> +	mutex_lock(&wdt->lock);
> +
> +	if (!wdt->vde_off &&
> +	    (readl(wdt->rst_source) & T20_RST_SOURCE_WDT_EN)) {
>  		dev_info(dev, "watchdog left running by the bootloader, taking it over (timeout %u sec)\n",
>  			 wdt->wdd.timeout);
> -		tegra20_wdt_start(&wdt->wdd);
> +		__tegra20_wdt_start(wdt);
>  		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
>  	}

[Severity: Medium]
Does clearing WDOG_HW_RUNNING in tegra20_wdt_powergate_notify() leak the
module and wd_data->dev references acquired for a boot-enabled watchdog?

When WDOG_HW_RUNNING is set here before devm_watchdog_register_device(),
watchdog_cdev_register() takes a module reference and a device reference:

drivers/watchdog/watchdog_dev.c:watchdog_cdev_register() {
    ...
	if (watchdog_hw_running(wdd)) {
		__module_get(wdd->ops->owner);
		get_device(&wd_data->dev);
    ...
}

The watchdog core only drops those references after user space opens the
device while WDOG_HW_RUNNING is still set and later closes it with
WDOG_HW_RUNNING cleared.  If the VDE partition powers off before
/dev/watchdog is opened, tegra20_wdt_powergate_notify() clears
WDOG_HW_RUNNING and sets wdt->vde_off = true, so any later watchdog_open()
fails with -EBUSY in tegra20_wdt_start() and those references are never
released, preventing the module from being unloaded.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-t20-for-upstream-v1-0-5923449951fe@gmail.com?part=6