linux-kernel - Re: [PATCH REVIEW 2/2] watchdog: add wd_data->lock mutex locking to watchdog

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: <368f894f-e433-6a9b-31c6-35dfa2842576@csgroup.eu>
Date:   Thu, 18 Aug 2022 09:09:03 +0000
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Alexey Klimov <aklimov@...hat.com>,
        "linux-watchdog@...r.kernel.org" <linux-watchdog@...r.kernel.org>
CC:     "wim@...ux-watchdog.org" <wim@...ux-watchdog.org>,
        "linux@...ck-us.net" <linux@...ck-us.net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "klimov.linux@...il.com" <klimov.linux@...il.com>
Subject: Re: [PATCH REVIEW 2/2] watchdog: add wd_data->lock mutex locking to
 watchdog_open()



Le 18/08/2022 à 04:06, Alexey Klimov a écrit :
> [Vous ne recevez pas souvent de courriers de aklimov@...hat.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
> 
> watchdog_open() does not have wd_data->lock locks at all unlike
> the watchdog_release() for instance. Also watchdog_open() calls
> watchdog_start() that should be called with wd_data->lock acquired.
> The mutex lock should be acquired in the beginning of the function
> after getting struct watchdog_core_data wd_data pointer to deal with
> different status fields and be able to call watchdog_start(); and
> released on exit and on different error paths.

Why do you need the mutex for the open ?

open is protected via atomic operation test_and_set_bit(), so what ?

If the mutex is hold while calling open, it means that the device is 
already open so test_and_set_bit will fail.

> 
> Signed-off-by: Alexey Klimov <aklimov@...hat.com>
> ---
>   drivers/watchdog/watchdog_dev.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 804236a094f6..d07a864036d9 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -836,7 +836,7 @@ static int watchdog_open(struct inode *inode, struct file *file)
>          struct watchdog_core_data *wd_data;
>          struct watchdog_device *wdd;
>          bool hw_running;
> -       int err;
> +       int err = -EBUSY;
> 
>          /* Get the corresponding watchdog device */
>          if (imajor(inode) == MISC_MAJOR)
> @@ -845,9 +845,10 @@ static int watchdog_open(struct inode *inode, struct file *file)
>                  wd_data = container_of(inode->i_cdev, struct watchdog_core_data,
>                                         cdev);
> 
> +       mutex_lock(&wd_data->lock);
>          /* the watchdog is single open! */
>          if (test_and_set_bit(_WDOG_DEV_OPEN, &wd_data->status))
> -               return -EBUSY;
> +               goto out_unlock;
> 
>          wdd = wd_data->wdd;
> 
> @@ -856,10 +857,8 @@ static int watchdog_open(struct inode *inode, struct file *file)
>           * to be unloaded.
>           */
>          hw_running = watchdog_hw_running(wdd);
> -       if (!hw_running && !try_module_get(wdd->ops->owner)) {
> -               err = -EBUSY;
> +       if (!hw_running && !try_module_get(wdd->ops->owner))
>                  goto out_clear;
> -       }
> 
>          err = watchdog_start(wdd);
>          if (err < 0)
> @@ -878,6 +877,7 @@ static int watchdog_open(struct inode *inode, struct file *file)
>           * applied.
>           */
>          wd_data->open_deadline = KTIME_MAX;
> +       mutex_unlock(&wd_data->lock);
> 
>          /* dev/watchdog is a virtual (and thus non-seekable) filesystem */
>          return stream_open(inode, file);
> @@ -886,6 +886,8 @@ static int watchdog_open(struct inode *inode, struct file *file)
>          module_put(wd_data->wdd->ops->owner);
>   out_clear:
>          clear_bit(_WDOG_DEV_OPEN, &wd_data->status);
> +out_unlock:
> +       mutex_unlock(&wd_data->lock);
>          return err;
>   }
> 
> --
> 2.37.2
>