[PATCH v2 0/2] watchdog: pika_wdt: portability and timer lifecycle fixes

[PATCH v2 0/2] watchdog: pika_wdt: portability and timer lifecycle fixes

[Rosen Penev]

Replace ppc4xx-specific MMIO accessors with portable helpers and
fix two high-severity timer bugs: a UAF on module exit (timer
fires after iounmap) and a close race that defeats the watchdog
reset on unexpected daemon termination.

v2: fix preexisting issues

Rosen Penev (2):
  watchdog: pika_wdt: fix timer leak and close race
  watchdog: pika_wdt: replace in_be32/out_be32 with
    ioread32be/iowrite32be

 drivers/watchdog/Kconfig    |  3 ++-
 drivers/watchdog/pika_wdt.c | 14 ++++++++------
 2 files changed, 10 insertions(+), 7 deletions(-)

--
2.54.0

[PATCH v2 1/2] watchdog: pika_wdt: fix timer leak and close race

[Rosen Penev]

Two timer lifecycle bugs in the PIKA watchdog driver:

1. pikawdt_exit() unmaps the FPGA IO memory without stopping the ping
   timer. A timer tick after iounmap executes pikawdt_ping() on freed
   memory. Add timer_delete_sync() before iounmap.

2. pikawdt_release() uses timer_delete() which does not synchronize with
   a concurrently executing timer handler. If pikawdt_ping() runs on
   another CPU after timer_delete() marks the timer as deleted but before
   the open bit is cleared, it can call mod_timer() and re-arm itself.
   Once the open bit is subsequently cleared, the condition
   (!nowayout && !open) becomes true and the timer keeps patting the
   watchdog forever, defeating the intended reset-on-unexpected-close.
   Fix by clearing the open bit before calling timer_delete_sync(), which
   waits for any in-flight handler to complete.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/watchdog/pika_wdt.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/pika_wdt.c b/drivers/watchdog/pika_wdt.c
index 87b8988d2520..60a846cb3601 100644
--- a/drivers/watchdog/pika_wdt.c
+++ b/drivers/watchdog/pika_wdt.c
@@ -127,11 +127,12 @@ static int pikawdt_open(struct inode *inode, struct file *file)
  */
 static int pikawdt_release(struct inode *inode, struct file *file)
 {
+	clear_bit(0, &pikawdt_private.open);
+
 	/* stop internal ping */
 	if (!pikawdt_private.expect_close)
-		timer_delete(&pikawdt_private.timer);
+		timer_delete_sync(&pikawdt_private.timer);
 
-	clear_bit(0, &pikawdt_private.open);
 	pikawdt_private.expect_close = 0;
 	return 0;
 }
@@ -291,6 +292,7 @@ static void __exit pikawdt_exit(void)
 {
 	misc_deregister(&pikawdt_miscdev);
 
+	timer_delete_sync(&pikawdt_private.timer);
 	iounmap(pikawdt_private.fpga);
 }
 
-- 
2.54.0

Re: [PATCH v2 1/2] watchdog: pika_wdt: fix timer leak and close race

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Moving `clear_bit()` before `timer_delete_sync()` in `pikawdt_release()` introduces a race condition with concurrent `open()` calls, leading to the deletion of a newly-armed timer and a spurious system reset.
- [Medium] Module teardown in `pikawdt_exit()` uses `timer_delete_sync()` instead of the mandatory `timer_shutdown_sync()` API.
--

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

watchdog: pika_wdt: fix timer leak and close race

This commit fixes two timer lifecycle bugs in the PIKA watchdog driver. It
adds timer_delete_sync() in pikawdt_exit() to stop the ping timer before
unmapping the FPGA IO memory, and replaces timer_delete() with
timer_delete_sync() in pikawdt_release() while clearing the open bit
earlier to avoid a race.

> diff --git a/drivers/watchdog/pika_wdt.c b/drivers/watchdog/pika_wdt.c
> index 87b8988d25206..60a846cb36011 100644
> --- a/drivers/watchdog/pika_wdt.c
> +++ b/drivers/watchdog/pika_wdt.c
> @@ -127,11 +127,12 @@ static int pikawdt_open(struct inode *inode, struct file *file)
>   */
>  static int pikawdt_release(struct inode *inode, struct file *file)
>  {
> +	clear_bit(0, &pikawdt_private.open);
> +
>  	/* stop internal ping */
>  	if (!pikawdt_private.expect_close)
> -		timer_delete(&pikawdt_private.timer);
> +		timer_delete_sync(&pikawdt_private.timer);

[Severity: High]
Does moving clear_bit() before timer_delete_sync() in pikawdt_release()
introduce a race condition with concurrent open() calls?

If process A closes the device unexpectedly, it clears the open bit early.
Process B on another CPU could then successfully open the device and arm
the internal ping timer.

Process A would then resume and execute timer_delete_sync(), which deletes
the timer that process B just started.

Process B would believe it successfully acquired the watchdog, but the
internal ping timer is stopped, and the hardware watchdog could eventually
reset the system.

>  
> -	clear_bit(0, &pikawdt_private.open);
>  	pikawdt_private.expect_close = 0;
>  	return 0;
>  }
> @@ -291,6 +292,7 @@ static void __exit pikawdt_exit(void)
>  {
>  	misc_deregister(&pikawdt_miscdev);
>  
> +	timer_delete_sync(&pikawdt_private.timer);

[Severity: Medium]
Should pikawdt_exit() be using timer_shutdown_sync() instead?

Since this is the module teardown path right before unmapping the device
memory, timer_shutdown_sync() is strictly required to permanently prevent
the timer from being re-armed.

>  	iounmap(pikawdt_private.fpga);
>  }
>

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

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

[Rosen Penev]

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.

Add HAS_IOMEM dependency for these functions.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/watchdog/Kconfig    | 3 ++-
 drivers/watchdog/pika_wdt.c | 8 ++++----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 8319c503319a..855736eaca2c 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -2040,7 +2040,8 @@ config 8xxx_WDT
 
 config PIKA_WDT
 	tristate "PIKA FPGA Watchdog"
-	depends on WARP || (PPC64 && COMPILE_TEST)
+	depends on WARP || COMPILE_TEST
+	depends on HAS_IOMEM
 	default WARP
 	help
 	  This enables the watchdog in the PIKA FPGA. Currently used on
diff --git a/drivers/watchdog/pika_wdt.c b/drivers/watchdog/pika_wdt.c
index 60a846cb3601..beb3ba1ab84f 100644
--- a/drivers/watchdog/pika_wdt.c
+++ b/drivers/watchdog/pika_wdt.c
@@ -77,10 +77,10 @@ static inline void pikawdt_reset(void)
 	 *           seconds. Valid ranges are 1 to 15 seconds. The value can
 	 *           be modified dynamically.
 	 */
-	unsigned reset = in_be32(pikawdt_private.fpga + 0x14);
+	unsigned reset = ioread32be(pikawdt_private.fpga + 0x14);
 	/* enable with max timeout - 15 seconds */
 	reset |= (1 << 7) + (WDT_HW_TIMEOUT << 8);
-	out_be32(pikawdt_private.fpga + 0x14, reset);
+	iowrite32be(reset, pikawdt_private.fpga + 0x14);
 }
 
 /*
@@ -243,7 +243,7 @@ static int __init pikawdt_init(void)
 		return -ENOMEM;
 	}
 
-	ident.firmware_version = in_be32(pikawdt_private.fpga + 0x1c) & 0xffff;
+	ident.firmware_version = ioread32be(pikawdt_private.fpga + 0x1c) & 0xffff;
 
 	/* POST information is in the sd area. */
 	np = of_find_compatible_node(NULL, NULL, "pika,fpga-sd");
@@ -265,7 +265,7 @@ static int __init pikawdt_init(void)
 	 * Bit 31,   WDOG: Set to 1 when the last reset was caused by a watchdog
 	 *           timeout.
 	 */
-	post1 = in_be32(fpga + 0x40);
+	post1 = ioread32be(fpga + 0x40);
 	if (post1 & 0x80000000)
 		pikawdt_private.bootstatus = WDIOF_CARDRESET;
 
-- 
2.54.0