* [PATCH REVIEW 1/2] watchdog: extend the mutex-protected section in watchdog_cdev_unregister()
@ 2022-08-18 2:06 Alexey Klimov
2022-08-18 2:06 ` [PATCH REVIEW 2/2] watchdog: add wd_data->lock mutex locking to watchdog_open() Alexey Klimov
2022-08-18 16:55 ` [PATCH REVIEW 1/2] watchdog: extend the mutex-protected section in watchdog_cdev_unregister() Guenter Roeck
0 siblings, 2 replies; 5+ messages in thread
From: Alexey Klimov @ 2022-08-18 2:06 UTC (permalink / raw)
To: linux-watchdog; +Cc: wim, linux, linux-kernel, klimov.linux
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH REVIEW 2/2] watchdog: add wd_data->lock mutex locking to watchdog_open()
2022-08-18 2:06 [PATCH REVIEW 1/2] watchdog: extend the mutex-protected section in watchdog_cdev_unregister() Alexey Klimov
@ 2022-08-18 2:06 ` Alexey Klimov
2022-08-18 9:09 ` Christophe Leroy
2022-08-18 16:57 ` Guenter Roeck
2022-08-18 16:55 ` [PATCH REVIEW 1/2] watchdog: extend the mutex-protected section in watchdog_cdev_unregister() Guenter Roeck
1 sibling, 2 replies; 5+ messages in thread
From: Alexey Klimov @ 2022-08-18 2:06 UTC (permalink / raw)
To: linux-watchdog; +Cc: wim, linux, linux-kernel, klimov.linux
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH REVIEW 2/2] watchdog: add wd_data->lock mutex locking to watchdog_open()
2022-08-18 2:06 ` [PATCH REVIEW 2/2] watchdog: add wd_data->lock mutex locking to watchdog_open() Alexey Klimov
@ 2022-08-18 9:09 ` Christophe Leroy
2022-08-18 16:57 ` Guenter Roeck
1 sibling, 0 replies; 5+ messages in thread
From: Christophe Leroy @ 2022-08-18 9:09 UTC (permalink / raw)
To: Alexey Klimov, linux-watchdog; +Cc: wim, linux, linux-kernel, klimov.linux
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH REVIEW 1/2] watchdog: extend the mutex-protected section in watchdog_cdev_unregister()
2022-08-18 2:06 [PATCH REVIEW 1/2] watchdog: extend the mutex-protected section in watchdog_cdev_unregister() Alexey Klimov
2022-08-18 2:06 ` [PATCH REVIEW 2/2] watchdog: add wd_data->lock mutex locking to watchdog_open() Alexey Klimov
@ 2022-08-18 16:55 ` Guenter Roeck
1 sibling, 0 replies; 5+ messages in thread
From: Guenter Roeck @ 2022-08-18 16:55 UTC (permalink / raw)
To: Alexey Klimov; +Cc: linux-watchdog, wim, linux-kernel, klimov.linux
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH REVIEW 2/2] watchdog: add wd_data->lock mutex locking to watchdog_open()
2022-08-18 2:06 ` [PATCH REVIEW 2/2] watchdog: add wd_data->lock mutex locking to watchdog_open() Alexey Klimov
2022-08-18 9:09 ` Christophe Leroy
@ 2022-08-18 16:57 ` Guenter Roeck
1 sibling, 0 replies; 5+ messages in thread
From: Guenter Roeck @ 2022-08-18 16:57 UTC (permalink / raw)
To: Alexey Klimov; +Cc: linux-watchdog, wim, linux-kernel, klimov.linux
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-08-18 16:57 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-18 2:06 [PATCH REVIEW 1/2] watchdog: extend the mutex-protected section in watchdog_cdev_unregister() Alexey Klimov
2022-08-18 2:06 ` [PATCH REVIEW 2/2] watchdog: add wd_data->lock mutex locking to watchdog_open() Alexey Klimov
2022-08-18 9:09 ` Christophe Leroy
2022-08-18 16:57 ` Guenter Roeck
2022-08-18 16:55 ` [PATCH REVIEW 1/2] watchdog: extend the mutex-protected section in watchdog_cdev_unregister() Guenter Roeck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).