| Message ID | 20220818020624.2386475-2-aklimov@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [REVIEW,1/2] watchdog: extend the mutex-protected section in watchdog_cdev_unregister() | expand |
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 >
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 >
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; }
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(-)