Re: [PATCH] watchdog: pika_wdt: replace in_be32/out_be32 with ioread32be/iowrite32be

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Relaxing Kconfig dependency allows compilation on unsupported architectures, causing build failures.

Pre-existing issues:
- [High] The module fails to delete its background timer on exit, leading to a use-after-free and kernel panic.
- [High] A race condition during an unexpected close allows the watchdog timer to re-arm indefinitely, failing to reset the machine.
--

commit 432026085d9a8c09851b8a2cd20ef5d62df73ad1
Author: Rosen Penev <rosenp@gmail.com>

watchdog: pika_wdt: replace in_be32/out_be32 with ioread32be/iowrite32be

Convert the ppc4xx-specific in_be32/out_be32 to the portable
ioread32be/iowrite32be. Also relax the Kconfig dependency from
(PPC64 && COMPILE_TEST) to just COMPILE_TEST to get build coverage.

> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index dc78729ba2a5d..a7e53091f27c5 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -2065,7 +2065,7 @@ config 8xxx_WDT
>  
>  config PIKA_WDT
>  	tristate "PIKA FPGA Watchdog"
> -	depends on WARP || (PPC64 && COMPILE_TEST)
> +	depends on WARP || COMPILE_TEST

[Severity: Low]
Will relaxing this dependency cause build failures on architectures without
I/O memory support?

Because the driver utilizes memory-mapped I/O functions like ioread32be()
and iowrite32be(), removing the implicit requirement for HAS_IOMEM (which
PPC64 previously satisfied) might allow it to be compiled on architectures
that lack I/O memory (such as S390 without PCI).

Should this include a dependency on HAS_IOMEM?

[ ... ]

[Severity: High]
This is a pre-existing issue, but does pikawdt_exit() leak a running timer?

If the watchdog device is closed properly via the magic close character,
the driver leaves the timer running in the background. If the module is
then unloaded, the exit function doesn't stop the timer:

drivers/watchdog/pika_wdt.c:pikawdt_exit() {
        misc_deregister(&pikawdt_miscdev);
        iounmap(pikawdt_private.fpga);
}

When the kernel timer wheel expires and attempts to execute pikawdt_ping()
in unmapped memory, will this result in a use-after-free and a kernel
panic?

Could a call to timer_delete_sync() be added here to prevent this?

[Severity: High]
This isn't a bug introduced by this patch, but is there a race condition
during an unexpected close that defeats the watchdog reset?

In pikawdt_release(), an unexpected close attempts to stop the ping timer:

drivers/watchdog/pika_wdt.c:pikawdt_release() {
        ...
        if (!pikawdt_private.expect_close)
                timer_delete(&pikawdt_private.timer);

        clear_bit(0, &pikawdt_private.open);
        ...
}

Because timer_delete() does not synchronize with concurrently running
handlers, if pikawdt_ping() is executing on another CPU, it can evaluate
its condition as true and call mod_timer(), re-arming the timer after it
was deleted:

drivers/watchdog/pika_wdt.c:pikawdt_ping() {
        ...
        if (time_before(jiffies, pikawdt_private.next_heartbeat) ||
                        (!nowayout && !pikawdt_private.open)) {
                pikawdt_reset();
                mod_timer(&pikawdt_private.timer, jiffies + WDT_TIMEOUT);
        }
        ...
}

Since pikawdt_release() clears the open bit immediately after, the
improperly re-armed timer will evaluate its fallback condition
(!nowayout && !pikawdt_private.open) as true on subsequent ticks.

Won't this cause it to pat the watchdog forever, completely preventing a
system reset during an unexpected daemon termination?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602035620.623085-1-rosenp@gmail.com?part=1