[PATCH] Differentiate scenarios when watchdog is closed

Charles Haithcock posted 1 patch 1 month ago
drivers/watchdog/watchdog_dev.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
[PATCH] Differentiate scenarios when watchdog is closed
Posted by Charles Haithcock 1 month ago
Presenty, when a watchdog device is closed, we print "watchdog did not
stop" in a few different scenarios;

1. When nowayout is set
2. When the watchdog is able to close, has received the magic character
   to stop, but fails to close in device-specific code paths
3. When userspace delierately closes it without stopping it

For 1, we explicitly print we can not close because of nowayout. Nothing
differentiates the other two however.

This change adds a print to indicate the watchdog was closed while still
running.

Signed-off-by: Charles Haithcock <chaithco@redhat.com>
---

 drivers/watchdog/watchdog_dev.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index d7895009a2..a571dea353 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -955,14 +955,17 @@ static int watchdog_release(struct inode *inode, struct file *file)
 	if (!watchdog_active(wdd))
 		err = 0;
 	else if (test_and_clear_bit(_WDOG_ALLOW_RELEASE, &wd_data->status) ||
-		 !(wdd->info->options & WDIOF_MAGICCLOSE))
+		 !(wdd->info->options & WDIOF_MAGICCLOSE)) {
 		err = watchdog_stop(wdd);
 
-	/* If the watchdog was not stopped, send a keepalive ping */
-	if (err < 0) {
-		pr_crit("watchdog%d: watchdog did not stop!\n", wdd->id);
-		watchdog_ping(wdd);
+		/* If the watchdog was not stopped, send a keepalive ping */
+		if (err < 0) {
+			pr_crit("watchdog%d: watchdog did not stop!\n", wdd->id);
+			watchdog_ping(wdd);
+		}
 	}
+	else
+		pr_info("watchdog%d: closing while running!\n", wdd->id);
 
 	watchdog_update_worker(wdd);
 
-- 
2.55.0
Re: [PATCH] Differentiate scenarios when watchdog is closed
Posted by Guenter Roeck 1 month ago
On 8/24/26 13:50, Charles Haithcock wrote:
> Presenty, when a watchdog device is closed, we print "watchdog did not

Presently

Also, the subject should start with the subsystem name ("watchdog:")

> stop" in a few different scenarios;
> 
> 1. When nowayout is set
> 2. When the watchdog is able to close, has received the magic character
>     to stop, but fails to close in device-specific code paths
> 3. When userspace delierately closes it without stopping it

deliberately

> 
> For 1, we explicitly print we can not close because of nowayout. Nothing
> differentiates the other two however.
> 
> This change adds a print to indicate the watchdog was closed while still
> running.
> 
> Signed-off-by: Charles Haithcock <chaithco@redhat.com>
> ---
> 
>   drivers/watchdog/watchdog_dev.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index d7895009a2..a571dea353 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -955,14 +955,17 @@ static int watchdog_release(struct inode *inode, struct file *file)
>   	if (!watchdog_active(wdd))
>   		err = 0;
>   	else if (test_and_clear_bit(_WDOG_ALLOW_RELEASE, &wd_data->status) ||
> -		 !(wdd->info->options & WDIOF_MAGICCLOSE))
> +		 !(wdd->info->options & WDIOF_MAGICCLOSE)) {
>   		err = watchdog_stop(wdd);
>   
> -	/* If the watchdog was not stopped, send a keepalive ping */
> -	if (err < 0) {
> -		pr_crit("watchdog%d: watchdog did not stop!\n", wdd->id);
> -		watchdog_ping(wdd);
> +		/* If the watchdog was not stopped, send a keepalive ping */
> +		if (err < 0) {
> +			pr_crit("watchdog%d: watchdog did not stop!\n", wdd->id);
> +			watchdog_ping(wdd);
> +		}
>   	}
> +	else
> +		pr_info("watchdog%d: closing while running!\n", wdd->id);

As Sashiko points out, this changes behavior if the watchdog is active
and was not stopped. Also, I personally find "closing while running"
not very informative. Also, while technically userspace may close the
watchdog deliberately while it is running, that is not what happens
on a regular basis. I find the previous unconditional "watchdog did
not stop" message more informative and relevant.

If you want to make a change, I would suggest to add an error message
into watchdog_stop() to report an error if the stop callback returns
an error. That would distinguish 2/3 without making functional changes.

Thanks,
Guenter
Re: [PATCH] Differentiate scenarios when watchdog is closed
Posted by chaithco@redhat.com 1 month ago

On Thu, Aug 27 2026 at 09:01:47 AM -07:00:00, Guenter Roeck 
<linux@roeck-us.net> wrote:
> Presently
> [...]
> Also, the subject should start with the subsystem name ("watchdog:")
> [...]
> deliberately

Thank you for catching these! Please accept my apologies. I can fix 
those up in the next submission.

> [...] Also, while technically userspace may close the
> watchdog deliberately while it is running, that is not what happens
> on a regular basis.

This is actually what initiated a bug report at 
https://bugzilla.redhat.com/show_bug.cgi?id=1991285 it turns out 
systemd explicitly does this to help ensure a system shutting down 
actually eventually goes down even if the shutdown process hits some 
snags. It does this on every shutdown. Given the prevalence of systemd, 
this is a regular occurrence. The end result is that, when using iTCO, 
it shows an error on every shutdown when systemd is in use as init.

> If you want to make a change, I would suggest to add an error message
> into watchdog_stop() to report an error if the stop callback returns
> an error. That would distinguish 2/3 without making functional 
> changes.

Thank you! So something like this?

        if (wdd->ops->stop) {
                clear_bit(WDOG_HW_RUNNING, &wdd->status);
                err = wdd->ops->stop(wdd);
+               if (err < 0)
+                       pr_info("watchdog%d: closed while still 
enabled!\n");
                trace_watchdog_stop(wdd, err);
        } else {
                set_bit(WDOG_HW_RUNNING, &wdd->status);


While responding to this, an additional thought occurred to me; given 
the primary reason a user would see this is because systemd is shutting 
down a system, it may be more worth while to have systemd log something 
about closing the watchdog without disarming it to at least explain a 
pr_crit kernel log line. Otherwise, it just looks like "something bad 
happened" with watchdog. I am additionally unsure of what would be best 
to go in watchdog_stop that helps differentiate intentional closing of 
the watchdog without disabling vs malicious/accidental closing. The 
intent would lie within the entity closing the watchdog; "closed while 
still enabled!" still seems like "something bad happened" with info on 
if it was intentional or not.

Thank you!
- Charles
Re: [PATCH] Differentiate scenarios when watchdog is closed
Posted by Guenter Roeck 1 month ago
On 8/27/26 10:09, chaithco@redhat.com wrote:
> 
> 
> On Thu, Aug 27 2026 at 09:01:47 AM -07:00:00, Guenter Roeck <linux@roeck-us.net> wrote:
>> Presently
>> [...]
>> Also, the subject should start with the subsystem name ("watchdog:")
>> [...]
>> deliberately
> 
> Thank you for catching these! Please accept my apologies. I can fix those up in the next submission.
> 
>> [...] Also, while technically userspace may close the
>> watchdog deliberately while it is running, that is not what happens
>> on a regular basis.
> 
> This is actually what initiated a bug report at https://bugzilla.redhat.com/show_bug.cgi?id=1991285 it turns out systemd explicitly does this to help ensure a system shutting down actually eventually goes down even if the shutdown process hits some snags. It does this on every shutdown. Given the prevalence of systemd, this is a regular occurrence. The end result is that, when using iTCO, it shows an error on every shutdown when systemd is in use as init.
> 
>> If you want to make a change, I would suggest to add an error message
>> into watchdog_stop() to report an error if the stop callback returns
>> an error. That would distinguish 2/3 without making functional changes.
> 
> Thank you! So something like this?
> 
>         if (wdd->ops->stop) {
>                 clear_bit(WDOG_HW_RUNNING, &wdd->status);
>                 err = wdd->ops->stop(wdd);
> +               if (err < 0)
> +                       pr_info("watchdog%d: closed while still enabled!\n");

More like

			pr_err(""watchdog%d: Failed to stop watchdog: %pe\n", wdd->id, ERR_PTR(err));

since this would be a real error.

The "watchdog%d: watchdog did not stop!" message will then follow
(unconditionally).

>                 trace_watchdog_stop(wdd, err);
>         } else {
>                 set_bit(WDOG_HW_RUNNING, &wdd->status);
> 
> 
> While responding to this, an additional thought occurred to me; given the primary reason a user would see this is because systemd is shutting down a system, it may be more worth while to have systemd log something about closing the watchdog without disarming it to at least explain a pr_crit kernel log line. Otherwise, it just looks like "something bad happened" with watchdog. I am additionally unsure of what would be best to go in watchdog_stop that helps differentiate intentional closing of the watchdog without disabling vs malicious/accidental closing. The intent would lie within the entity closing the watchdog; "closed while still enabled!" still seems like "something bad happened" with info on if it was intentional or not.
> 

Problem is that we don't know if "something bad happened". The same message
will be seen if the watchdog daemon was killed or crashed. We can not just
assume that closing the watchdog device was intentional.

Guenter

Re: [PATCH] Differentiate scenarios when watchdog is closed
Posted by chaithco@redhat.com 3 weeks, 6 days ago
On Thu, 2026-08-27 at 11:10 -0700, Guenter Roeck wrote:
> Problem is that we don't know if "something bad happened". The same
> message
> will be seen if the watchdog daemon was killed or crashed. We can not
> just
> assume that closing the watchdog device was intentional.

That's the same sticking point I've come across. Differentiating
between positive intentional closure versus malicious or buggy closure
would somehow need to be communicated to the kernel. It makes more
sense that users of the watchdog which intend to close the watchdog
while still running maybe denote such. I opened a PR with systemd at
https://github.com/systemd/systemd/pull/43552 to see if there was some
way this could be tackled from that perspective.
Re: [PATCH] Differentiate scenarios when watchdog is closed
Posted by Guenter Roeck 3 weeks, 5 days ago
On 8/31/26 13:05, chaithco@redhat.com wrote:
> On Thu, 2026-08-27 at 11:10 -0700, Guenter Roeck wrote:
>> Problem is that we don't know if "something bad happened". The same
>> message
>> will be seen if the watchdog daemon was killed or crashed. We can not
>> just
>> assume that closing the watchdog device was intentional.
> 
> That's the same sticking point I've come across. Differentiating
> between positive intentional closure versus malicious or buggy closure
> would somehow need to be communicated to the kernel. It makes more
> sense that users of the watchdog which intend to close the watchdog
> while still running maybe denote such. I opened a PR with systemd at
> https://github.com/systemd/systemd/pull/43552 to see if there was some
> way this could be tackled from that perspective.

I'd want to see a message even if the watchdog is closed but left running
on purpose. The best we can do is to add another message if there was an
error when trying to stop the watchdog. That should not happen too often
(well, not at all, really, unless there is a bug or hardware problem),
so it would not really add noise.

Thanks,
Guenter
Re: [PATCH] Differentiate scenarios when watchdog is closed
Posted by chaithco@redhat.com 3 weeks, 5 days ago
On Tue, 2026-09-01 at 09:06 -0700, Guenter Roeck wrote:
> I'd want to see a message even if the watchdog is closed but left
> running
> on purpose. The best we can do is to add another message if there was
> an
> error when trying to stop the watchdog. That should not happen too
> often
> (well, not at all, really, unless there is a bug or hardware
> problem),
> so it would not really add noise.

Understood; I just submitted a second patch with your recommendations.
Thank you again!