linux-kernel - Re: [PATCH 1/2] watchdog: Check WDOG_STOP_ON

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200213191230.GA17448@roeck-us.net>
Date:   Thu, 13 Feb 2020 11:12:30 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Dmitry Safonov <dima@...sta.com>
Cc:     linux-kernel@...r.kernel.org,
        Dmitry Safonov <0x7f454c46@...il.com>,
        Wim Van Sebroeck <wim@...ux-watchdog.org>,
        linux-watchdog@...r.kernel.org
Subject: Re: [PATCH 1/2] watchdog: Check WDOG_STOP_ON_REBOOT in reboot
 notifier

On Thu, Feb 13, 2020 at 05:59:57PM +0000, Dmitry Safonov wrote:
> Many watchdog drivers use watchdog_stop_on_reboot() helper in order
> to stop the watchdog on system reboot. Unfortunately, this logic is
> coded in driver's probe function and doesn't allows user to decide what
> to do during shutdown/reboot.
> 
> On the other side, Xen and Qemu watchdog drivers (xen_wdt and i6300esb)
> may be configured to either send NMI or turn off/reboot VM as
> the watchdog action. As the kernel may stuck at any state, sending NMIs
> can't reliably reboot the VM.
> 
> At Arista, we benefited from the following set-up: the emulated watchdogs
> trigger VM reset and softdog is set to catch less severe conditions to
> generate vmcore. Just before reboot watchdog's timeout is increased
> to some good-enough value (3 mins). That keeps watchdog always running
> and guarantees that VM doesn't stuck.
> 
> As a preparation to move the watchdog's decision to stop on reboot or
> not in userspace, allow WDOG_STOP_ON_REBOOT to be set during runtime,
> not only on driver's probing. Always register reboot notifier and check
> WDOG_STOP_ON_REBOOT inside it (on actual reboot).
> 

Does that really have to be decided at runtime, by the user ?
How about doing it with a module parameter ?

Also, I am not sure if an ioctl is the best means to do this, if it indeed
makes sense to decide it at runtime. ioctl implies an open watchdog device,
which interferes with the watchdog daemon. This means that the watchdog
daemon would have to be modified to support this, making this a quite expensive
change. It also implies that the action would have to be known when the
watchdog daemon is started, suggesting that a module parameter should be
sufficient.

Guenter

> Signed-off-by: Dmitry Safonov <dima@...sta.com>
> ---
>  drivers/watchdog/watchdog_core.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index 861daf4f37b2..ebf80ff3e8ce 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -153,6 +153,10 @@ static int watchdog_reboot_notifier(struct notifier_block *nb,
>  	struct watchdog_device *wdd;
>  
>  	wdd = container_of(nb, struct watchdog_device, reboot_nb);
> +
> +	if (!test_bit(WDOG_STOP_ON_REBOOT, &wdd->status))
> +		return NOTIFY_DONE;
> +
>  	if (code == SYS_DOWN || code == SYS_HALT) {
>  		if (watchdog_active(wdd)) {
>  			int ret;
> @@ -254,17 +258,14 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>  		}
>  	}
>  
> -	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) {
> -		wdd->reboot_nb.notifier_call = watchdog_reboot_notifier;
> -
> -		ret = register_reboot_notifier(&wdd->reboot_nb);
> -		if (ret) {
> -			pr_err("watchdog%d: Cannot register reboot notifier (%d)\n",
> -			       wdd->id, ret);
> -			watchdog_dev_unregister(wdd);
> -			ida_simple_remove(&watchdog_ida, id);
> -			return ret;
> -		}
> +	wdd->reboot_nb.notifier_call = watchdog_reboot_notifier;
> +	ret = register_reboot_notifier(&wdd->reboot_nb);
> +	if (ret) {
> +		pr_err("watchdog%d: Cannot register reboot notifier (%d)\n",
> +				wdd->id, ret);
> +		watchdog_dev_unregister(wdd);
> +		ida_simple_remove(&watchdog_ida, id);
> +		return ret;
>  	}
>  
>  	if (wdd->ops->restart) {
> @@ -321,9 +322,7 @@ static void __watchdog_unregister_device(struct watchdog_device *wdd)
>  	if (wdd->ops->restart)
>  		unregister_restart_handler(&wdd->restart_nb);
>  
> -	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status))
> -		unregister_reboot_notifier(&wdd->reboot_nb);
> -
> +	unregister_reboot_notifier(&wdd->reboot_nb);
>  	watchdog_dev_unregister(wdd);
>  	ida_simple_remove(&watchdog_ida, wdd->id);
>  }
> -- 
> 2.25.0
>