Re: [PATCH REVIEW 2/2] watchdog: add wd_data->lock mutex locking to watchdog_open()

[Christophe Leroy]

Le 18/08/2022 à 04:06, Alexey Klimov a écrit :
> [Vous ne recevez pas souvent de courriers de aklimov@xxxxxxxxxx. 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@xxxxxxxxxx>
> ---
>   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
>