[PATCH REVIEW 1/2] watchdog: extend the mutex-protected section in watchdog_cdev_unregister()

[PATCH REVIEW 1/2] watchdog: extend the mutex-protected section in watchdog_cdev_unregister()

[Alexey Klimov]

watchdog_stop() should be called with wd_data->lock mutex locked.
Updates to wdd->status also occur under this look throughout the
watchdog_dev.c as well as functions that deal with pretimeout hrtimer
like watchdog_hrtimer_pretimeout_stop() here.

Signed-off-by: Alexey Klimov <aklimov@redhat.com>
---
 drivers/watchdog/watchdog_dev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 54903f3c851e..804236a094f6 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -1095,6 +1095,8 @@ static void watchdog_cdev_unregister(struct watchdog_device *wdd)
 		old_wd_data = NULL;
 	}
 
+	mutex_lock(&wd_data->lock);
+
 	if (watchdog_active(wdd) &&
 	    test_bit(WDOG_STOP_ON_UNREGISTER, &wdd->status)) {
 		watchdog_stop(wdd);
@@ -1102,7 +1104,6 @@ static void watchdog_cdev_unregister(struct watchdog_device *wdd)
 
 	watchdog_hrtimer_pretimeout_stop(wdd);
 
-	mutex_lock(&wd_data->lock);
 	wd_data->wdd = NULL;
 	wdd->wd_data = NULL;
 	mutex_unlock(&wd_data->lock);
-- 
2.37.2

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

[Alexey Klimov]

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.

Signed-off-by: Alexey Klimov <aklimov@redhat.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

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

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

[Guenter Roeck]

On Thu, Aug 18, 2022 at 03:06:24AM +0100, Alexey Klimov wrote:
> 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.
> 
Again, I am missing which problem you are fixing here. You are making
a claim that a lock is needed, but you do not explain _why_ this is really
needed, why test_and_set_bit() is insufficient, and what problem is solved
that has been observed with the current code.

Guenter

> Signed-off-by: Alexey Klimov <aklimov@redhat.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
> 

Re: [PATCH REVIEW 1/2] watchdog: extend the mutex-protected section in watchdog_cdev_unregister()

[Guenter Roeck]

On Thu, Aug 18, 2022 at 03:06:23AM +0100, Alexey Klimov wrote:
> watchdog_stop() should be called with wd_data->lock mutex locked.
> Updates to wdd->status also occur under this look throughout the
> watchdog_dev.c as well as functions that deal with pretimeout hrtimer
> like watchdog_hrtimer_pretimeout_stop() here.
> 

What actual problems are you fixing here ?

Guenter

> Signed-off-by: Alexey Klimov <aklimov@redhat.com>
> ---
>  drivers/watchdog/watchdog_dev.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 54903f3c851e..804236a094f6 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -1095,6 +1095,8 @@ static void watchdog_cdev_unregister(struct watchdog_device *wdd)
>  		old_wd_data = NULL;
>  	}
>  
> +	mutex_lock(&wd_data->lock);
> +
>  	if (watchdog_active(wdd) &&
>  	    test_bit(WDOG_STOP_ON_UNREGISTER, &wdd->status)) {
>  		watchdog_stop(wdd);
> @@ -1102,7 +1104,6 @@ static void watchdog_cdev_unregister(struct watchdog_device *wdd)
>  
>  	watchdog_hrtimer_pretimeout_stop(wdd);
>  
> -	mutex_lock(&wd_data->lock);
>  	wd_data->wdd = NULL;
>  	wdd->wd_data = NULL;
>  	mutex_unlock(&wd_data->lock);
> -- 
> 2.37.2
>