Re: [PATCH v1] watchdog: ni903x_wdt: Check ACPI_COMPANION() against NULL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The watchdog device is exposed to userspace before its hardware initialization is fully complete in `ni903x_acpi_probe`.
- [High] The driver's removal function races with the watchdog core worker and userspace, potentially leaving the hardware running after driver unload.
--

commit 6254d703c5994e457b8ddb7846a5094ab95cecda
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

watchdog: ni903x_wdt: Check ACPI_COMPANION() against NULL

This commit adds a check to ensure the ACPI companion object exists
before proceeding with the ni903x_wdt watchdog driver probe.

> diff --git a/drivers/watchdog/ni903x_wdt.c b/drivers/watchdog/ni903x_wdt.c
> --- a/drivers/watchdog/ni903x_wdt.c
> +++ b/drivers/watchdog/ni903x_wdt.c
> @@ -183,9 +183,14 @@ static int ni903x_acpi_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct watchdog_device *wdd;
>  	struct ni903x_wdt *wdt;
> +	acpi_handle handle;
>  	acpi_status status;
>  	int ret;
>  
> +	handle = ACPI_HANDLE(dev);
> +	if (!handle)
> +		return -ENODEV;
> +

This isn't a regression introduced by this commit, but while reviewing the
probe function modified here, I noticed a potential race condition.

In ni903x_acpi_probe(), the watchdog device is registered before the
hardware initialization is fully complete:

ni903x_acpi_probe() {
    ...
    ret = watchdog_register_device(wdd);
    if (ret)
        return ret;

    outb(NIWD_CONTROL_RESET | NIWD_CONTROL_MODE,
         wdt->io_base + NIWD_CONTROL);
    ...
}

Since the character device becomes visible to userspace immediately upon
registration, could a fast userspace process open the device and trigger
ni903x_wdd_start() before the hardware is initialized?

If ni903x_wdd_start() configures the watchdog by writing to the control
register, wouldn't the subsequent outb() in the probe thread clobber this
configuration without preserving the bits set by userspace?

This might leave the watchdog in an unpredictable state and prevent recovery
on a freeze.

There is also a similar pre-existing issue in the removal path.

If ni903x_acpi_remove() calls ni903x_wdd_stop() before
watchdog_unregister_device(), the device remains registered with the
watchdog core while stopping.

Could userspace or the watchdog core worker concurrently execute
ni903x_wdd_ping() during this time?

If ni903x_wdd_ping() performs a read-modify-write on the control register,
and ni903x_wdd_stop() writes a stop command to the same register, is it
possible for the ping operation to write back the active state and re-enable
the watchdog if the stop operation executes between the read and write?

If the driver then unloads while the hardware watchdog continues running
with no driver to pet it, will the system spuriously reboot?

Would it be safer to call watchdog_unregister_device() before stopping the
hardware, or perhaps use watchdog_stop_on_unregister()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/2280455.irdbgypaU6@rafael.j.wysocki?part=1