Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   =?UTF-8?q?Thomas=20Wei=C3=9Fschuh?= <linux@weissschuh.net>
To:     Wim Van Sebroeck <wim@linux-watchdog.org>,
        Guenter Roeck <linux@roeck-us.net>
Cc:     =?UTF-8?q?Thomas=20Wei=C3=9Fschuh?= <linux@weissschuh.net>,
        linux-watchdog@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: [RFC PATCH] watchdog: core: don't reset KEEPALIVEPING through sysfs
Date:   Sun, 27 Nov 2022 16:45:59 +0100
Message-Id: <20221127154559.80899-1-linux@weissschuh.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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@weissschuh.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

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@iguana.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
-- 
2.38.1