Message-ID: <6ee65abe-8620-956d-a4a2-4ec5ec6257a5@roeck-us.net>
Date:   Sun, 27 Nov 2022 08:47:12 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Thomas Weißschuh <linux@...ssschuh.net>,
        Wim Van Sebroeck <wim@...ux-watchdog.org>
Cc:     linux-watchdog@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] watchdog: core: don't reset KEEPALIVEPING through
 sysfs

On 11/27/22 07:45, Thomas Weißschuh wrote:
> Reading the watchdog status via ioctl(WDIOC_GETSTATUS) or sysfs will
> reset the status bit KEEPALIVEPING.
> 
> This is done so an application can validate that the watchdog was pinged
> since the last read of the status.
> 
> For the ioctl-based interface this is fine as only one application can
> manage a watchdog interface at a time, so it can properly handle this
> implicit state modification.
> 
> The sysfs "status" file however is world-readable. This means that the
> watchdog state can be modified by any other unprivileged process on the
> system.
> 
> As the sysfs interface can also not be used to set this bit, let's
> remove the capability to clear it.
> 
> Fixes: 90b826f17a4e ("watchdog: Implement status function in watchdog core")
> Signed-off-by: Thomas Weißschuh <linux@...ssschuh.net>
> 
> ---
> 
> I was not able to find an application (besides wdctl) that uses this
> bit. But if applications are to use it, it should probably be reliable.
> 
> Other possible solutions I can think of:
> * Only reset the bit when the file opened privileged
> * Only reset the bit when the file opened writable
> 

All suggested solutions would be changing the ABI, which would be problematic.

As you have proposed elsewhere, it is possible for applications to chose where
to get the status from: sysfs or ioctl. It may well be that there is some
application out there which uses the sysfs attribute to read the status
and the ioctl otherwise. That would be odd, but possible.

Also, I can not imagine a real world use except for maybe reading the status
bit using sysfs from one application and checking if the watchdog demon actually
pinged it as it should ... but that is exactly what you are trying to disable
here.

Overall, this is probably the least valuable status bit. Any application should
know if it pinged the watchdog or not.

So: What real world problem have you observed that you are trying to solve ?
If there is no real observed problem, we should not entertain changing the ABI.
Actually, we can not change the ABI We would have to add another non-invasive
attribute that doesn't change the status when read. That should really be
worth the trouble.

Guenter

> Instead of using a status bit to check if the watchdog was pinged it
> would probably be more robust to use a sequence counter or timestamp.
> Especially as it seems more operations are being exposed over sysfs over
> time.
> 
> I'm not sure it's worth it though.
> ---
>   Documentation/ABI/testing/sysfs-class-watchdog |  3 ++-
>   drivers/watchdog/watchdog_dev.c                | 13 +++++++++----
>   2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-watchdog b/Documentation/ABI/testing/sysfs-class-watchdog
> index 585caecda3a5..182a8a9e530e 100644
> --- a/Documentation/ABI/testing/sysfs-class-watchdog
> +++ b/Documentation/ABI/testing/sysfs-class-watchdog
> @@ -38,7 +38,8 @@ Contact:	Wim Van Sebroeck <wim@...ana.be>
>   Description:
>   		It is a read only file. It contains watchdog device's
>   		internal status bits. It is equivalent to WDIOC_GETSTATUS
> -		of ioctl interface.
> +		of ioctl interface, except that the status bit
> +		WDIOF_KEEPALIVEPING is not reset.
>   
>   What:		/sys/class/watchdog/watchdogn/timeleft
>   Date:		August 2015
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 55574ed42504..7e3e964c4d6f 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -320,13 +320,15 @@ static int watchdog_stop(struct watchdog_device *wdd)
>   /*
>    * watchdog_get_status - wrapper to get the watchdog status
>    * @wdd: The watchdog device to get the status from
> + * @reset_keepaliveping: Reset the status bit _WDOG_KEEPALIVE
>    *
>    * Get the watchdog's status flags.
>    * The caller must hold wd_data->lock.
>    *
>    * Return: watchdog's status flags.
>    */
> -static unsigned int watchdog_get_status(struct watchdog_device *wdd)
> +static unsigned int watchdog_get_status(struct watchdog_device *wdd,
> +					bool reset_keepaliveping)
>   {
>   	struct watchdog_core_data *wd_data = wdd->wd_data;
>   	unsigned int status;
> @@ -345,9 +347,12 @@ static unsigned int watchdog_get_status(struct watchdog_device *wdd)
>   	if (test_bit(_WDOG_ALLOW_RELEASE, &wd_data->status))
>   		status |= WDIOF_MAGICCLOSE;
>   
> -	if (test_and_clear_bit(_WDOG_KEEPALIVE, &wd_data->status))
> +	if (test_bit(_WDOG_KEEPALIVE, &wd_data->status))
>   		status |= WDIOF_KEEPALIVEPING;
>   
> +	if (reset_keepaliveping)
> +		clear_bit(_WDOG_KEEPALIVE, &wd_data->status);
> +
>   	if (IS_ENABLED(CONFIG_WATCHDOG_HRTIMER_PRETIMEOUT))
>   		status |= WDIOF_PRETIMEOUT;
>   
> @@ -476,7 +481,7 @@ static ssize_t status_show(struct device *dev, struct device_attribute *attr,
>   	unsigned int status;
>   
>   	mutex_lock(&wd_data->lock);
> -	status = watchdog_get_status(wdd);
> +	status = watchdog_get_status(wdd, false);
>   	mutex_unlock(&wd_data->lock);
>   
>   	return sysfs_emit(buf, "0x%x\n", status);
> @@ -753,7 +758,7 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
>   			sizeof(struct watchdog_info)) ? -EFAULT : 0;
>   		break;
>   	case WDIOC_GETSTATUS:
> -		val = watchdog_get_status(wdd);
> +		val = watchdog_get_status(wdd, true);
>   		err = put_user(val, p);
>   		break;
>   	case WDIOC_GETBOOTSTATUS:
> 
> base-commit: faf68e3523c21d07c5f7fdabd0daf6301ff8db3f

Powered by blists - more mailing lists