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

[sashiko-bot@kernel.org]

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