watchdog: Add Locking support

This patch fixes some potential multithreading issues, despite only
allowing one process to open the /dev/watchdog device, we can still get
called multiple times at the same time, since a program could be using thread,
or could share the fd after a fork.

This causes 2 potential problems:
1) watchdog_start / open do an unlocked test_n_set / test_n_clear,
   if these 2 race, the watchdog could be stopped while the active
   bit indicates it is running or visa versa.

2) Most watchdog_dev drivers probably assume that only one
   watchdog-op will get called at a time, this is not necessary
   true atm.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index ce1fa22aa70bf31c275d14d9a614a4a63a03e361..08d34e11bc54233a8e3eb33022d022d288bb7fc5 100644 (file)
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -50,6 +50,7 @@ struct watchdog_device {
        unsigned int min_timeout;
        unsigned int max_timeout;
        void *driver_data;
+       struct mutex lock;
        unsigned long status;
 };
 
@@ -74,6 +75,7 @@ It contains following fields:
 * driver_data: a pointer to the drivers private data of a watchdog device.
   This data should only be accessed via the watchdog_set_drvdata and
   watchdog_get_drvdata routines.
+* lock: Mutex for WatchDog Timer Driver Core internal use only.
 * status: this field contains a number of status bits that give extra
   information about the status of the device (Like: is the watchdog timer
   running/active, is the nowayout bit set, is the device opened via

diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 86a57673abf9ced7e5c45d39535574ab681053fe..6aa46a90ff028691f97627765f593d864bc885a1 100644 (file)
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -79,6 +79,7 @@ int watchdog_register_device(struct watchdog_device *wdd)
         * corrupted in a later stage then we expect a kernel panic!
         */
 
+       mutex_init(&wdd->lock);
        id = ida_simple_get(&watchdog_ida, 0, MAX_DOGS, GFP_KERNEL);
        if (id < 0)
                return id;

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 76d2572fed258a733e42d26725963f029e89a76a..4d295d229a07defc24d6b0aa8feb10a9dbf70a7c 100644 (file)
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -63,6 +63,8 @@ static int watchdog_ping(struct watchdog_device *wddev)
 {
        int err = 0;
 
+       mutex_lock(&wddev->lock);
+
        if (!watchdog_active(wddev))
                goto out_ping;
 
@@ -72,6 +74,7 @@ static int watchdog_ping(struct watchdog_device *wddev)
                err = wddev->ops->start(wddev); /* restart watchdog */
 
 out_ping:
+       mutex_unlock(&wddev->lock);
        return err;
 }
 
@@ -88,6 +91,8 @@ static int watchdog_start(struct watchdog_device *wddev)
 {
        int err = 0;
 
+       mutex_lock(&wddev->lock);
+
        if (watchdog_active(wddev))
                goto out_start;
 
@@ -96,6 +101,7 @@ static int watchdog_start(struct watchdog_device *wddev)
                set_bit(WDOG_ACTIVE, &wddev->status);
 
 out_start:
+       mutex_unlock(&wddev->lock);
        return err;
 }
 
@@ -113,6 +119,8 @@ static int watchdog_stop(struct watchdog_device *wddev)
 {
        int err = 0;
 
+       mutex_lock(&wddev->lock);
+
        if (!watchdog_active(wddev))
                goto out_stop;
 
@@ -127,6 +135,7 @@ static int watchdog_stop(struct watchdog_device *wddev)
                clear_bit(WDOG_ACTIVE, &wddev->status);
 
 out_stop:
+       mutex_unlock(&wddev->lock);
        return err;
 }
 
@@ -147,8 +156,11 @@ static int watchdog_get_status(struct watchdog_device *wddev,
        if (!wddev->ops->status)
                return -EOPNOTSUPP;
 
+       mutex_lock(&wddev->lock);
+
        *status = wddev->ops->status(wddev);
 
+       mutex_unlock(&wddev->lock);
        return err;
 }
 
@@ -171,8 +183,11 @@ static int watchdog_set_timeout(struct watchdog_device *wddev,
            (timeout < wddev->min_timeout || timeout > wddev->max_timeout))
                return -EINVAL;
 
+       mutex_lock(&wddev->lock);
+
        err = wddev->ops->set_timeout(wddev, timeout);
 
+       mutex_unlock(&wddev->lock);
        return err;
 }
 
@@ -193,8 +208,11 @@ static int watchdog_get_timeleft(struct watchdog_device *wddev,
        if (!wddev->ops->get_timeleft)
                return -EOPNOTSUPP;
 
+       mutex_lock(&wddev->lock);
+
        *timeleft = wddev->ops->get_timeleft(wddev);
 
+       mutex_unlock(&wddev->lock);
        return err;
 }
 
@@ -213,8 +231,11 @@ static int watchdog_ioctl_op(struct watchdog_device *wddev, unsigned int cmd,
        if (!wddev->ops->ioctl)
                return -ENOIOCTLCMD;
 
+       mutex_lock(&wddev->lock);
+
        err = wddev->ops->ioctl(wddev, cmd, arg);
 
+       mutex_unlock(&wddev->lock);
        return err;
 }
 

diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index c3545c5d918a936a5ff579acac773b9a05201872..da1dc1b52744c7d867625c92a9acdcff8ec082dd 100644 (file)
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -104,6 +104,7 @@ struct watchdog_ops {
  * @min_timeout:The watchdog devices minimum timeout value.
  * @max_timeout:The watchdog devices maximum timeout value.
  * @driver-data:Pointer to the drivers private data.
+ * @lock:      Lock for watchdog core internal use only.
  * @status:    Field that contains the devices internal status bits.
  *
  * The watchdog_device structure contains all information about a
@@ -111,6 +112,9 @@ struct watchdog_ops {
  *
  * The driver-data field may not be accessed directly. It must be accessed
  * via the watchdog_set_drvdata and watchdog_get_drvdata helpers.
+ *
+ * The lock field is for watchdog core internal use only and should not be
+ * touched.
  */
 struct watchdog_device {
        int id;
@@ -124,6 +128,7 @@ struct watchdog_device {
        unsigned int min_timeout;
        unsigned int max_timeout;
        void *driver_data;
+       struct mutex lock;
        unsigned long status;
 /* Bit numbers for status flags */
 #define WDOG_ACTIVE            0       /* Is the watchdog running/active */