Re: [PATCHv3 1/2] watchdog: pika_wdt: fix timer lifecycle bugs

[Guenter Roeck]

On 6/8/26 14:04, Rosen Penev wrote:
> Two timer lifecycle bugs in the PIKA watchdog driver:
>
> 1. pikawdt_release() clears the open bit before stopping the timer,
>     allowing a concurrent pikawdt_open() to start a new timer that
>     is then immediately deleted.  Move clear_bit after the timer
>     stop.  Use timer_shutdown_sync() so the handler cannot re-arm
>     after the call returns.  Add timer_setup() in pikawdt_open()
>     to re-initialize the timer for the next open.

The clear bit is not moved, and the approach is wrong. More on that below.

> 2. pikawdt_exit() unmaps FPGA IO memory without first stopping the
>     ping timer.  If the timer handler fires after iounmap(), it
>     executes pikawdt_ping() on freed memory.  Use timer_shutdown_sync()
>     which permanently prevents re-arming.
>
> Assisted-by: opencode:big-pickle
> Signed-off-by: Rosen Penev <rosenp@xxxxxxxxx>
> ---
>   drivers/watchdog/pika_wdt.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/pika_wdt.c b/drivers/watchdog/pika_wdt.c
> index 87b8988d2520..d567103ec5e4 100644
> --- a/drivers/watchdog/pika_wdt.c
> +++ b/drivers/watchdog/pika_wdt.c
> @@ -117,6 +117,7 @@ static int pikawdt_open(struct inode *inode, struct file *file)
>   	if (test_and_set_bit(0, &pikawdt_private.open))
>   		return -EBUSY;
>
> +	timer_setup(&pikawdt_private.timer, pikawdt_ping, 0);

Calling timer_setup() in the open function is wrong because the
timer may already be running.

>   	pikawdt_start();
>
>   	return stream_open(inode, file);
> @@ -129,7 +130,7 @@ static int pikawdt_release(struct inode *inode, struct file *file)
>   {
>   	/* stop internal ping */
>   	if (!pikawdt_private.expect_close)
> -		timer_delete(&pikawdt_private.timer);
> +		timer_shutdown_sync(&pikawdt_private.timer);

This is wrong here. timer_shutdown_sync() is only supposed to be
called if the timer will not be re-enabled. Anything else would be
an API violation.

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