* [PATCH] md: fix unchecked interruptible mutex locks
@ 2011-04-14 6:48 Alexey Khoroshilov
2011-04-14 7:08 ` NeilBrown
0 siblings, 1 reply; 3+ messages in thread
From: Alexey Khoroshilov @ 2011-04-14 6:48 UTC (permalink / raw)
To: Neil Brown; +Cc: linux-raid, linux-kernel
mddev_lock() is an alias for mutex_lock_interruptible() at the moment,
but there are places where returned value of mddev_lock() is not checked.
The patch proposed introduces mddev_lock_interruptible(),
which is an equivalent for old mddev_lock(), while mddev_lock()
becomes an alias for uninterruptible mutex_lock().
Calls of mddev_lock(), where the result is being checked,
are replaced by mddev_lock_interruptible().
All the other calls become uninterruptible.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
drivers/md/md.c | 21 +++++++++++++--------
1 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index b12b377..cc6d183 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -623,7 +623,12 @@ static mddev_t * mddev_find(dev_t unit)
goto retry;
}
-static inline int mddev_lock(mddev_t * mddev)
+static inline void mddev_lock(mddev_t *mddev)
+{
+ return mutex_lock(&mddev->reconfig_mutex);
+}
+
+static inline int __must_check mddev_lock_interruptible(mddev_t *mddev)
{
return mutex_lock_interruptible(&mddev->reconfig_mutex);
}
@@ -2706,7 +2711,7 @@ rdev_attr_show(struct kobject *kobj, struct
attribute *attr, char *page)
if (!entry->show)
return -EIO;
- rv = mddev ? mddev_lock(mddev) : -EBUSY;
+ rv = mddev ? mddev_lock_interruptible(mddev) : -EBUSY;
if (!rv) {
if (rdev->mddev == NULL)
rv = -EBUSY;
@@ -2730,7 +2735,7 @@ rdev_attr_store(struct kobject *kobj, struct
attribute *attr,
return -EIO;
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
- rv = mddev ? mddev_lock(mddev): -EBUSY;
+ rv = mddev ? mddev_lock_interruptible(mddev) : -EBUSY;
if (!rv) {
if (rdev->mddev == NULL)
rv = -EBUSY;
@@ -4199,7 +4204,7 @@ md_attr_show(struct kobject *kobj, struct
attribute *attr, char *page)
if (!entry->show)
return -EIO;
- rv = mddev_lock(mddev);
+ rv = mddev_lock_interruptible(mddev);
if (!rv) {
rv = entry->show(mddev, page);
mddev_unlock(mddev);
@@ -4219,7 +4224,7 @@ md_attr_store(struct kobject *kobj, struct
attribute *attr,
return -EIO;
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
- rv = mddev_lock(mddev);
+ rv = mddev_lock_interruptible(mddev);
if (mddev->hold_active == UNTIL_IOCTL)
mddev->hold_active = 0;
if (!rv) {
@@ -4951,7 +4956,7 @@ static void autorun_devices(int part)
"md: cannot allocate memory for md drive.\n");
break;
}
- if (mddev_lock(mddev))
+ if (mddev_lock_interruptible(mddev))
printk(KERN_WARNING "md: %s locked, cannot run\n",
mdname(mddev));
else if (mddev->raid_disks || mddev->major_version
@@ -5771,7 +5776,7 @@ static int md_ioctl(struct block_device *bdev,
fmode_t mode,
goto abort;
}
- err = mddev_lock(mddev);
+ err = mddev_lock_interruptible(mddev);
if (err) {
printk(KERN_INFO
"md: ioctl lock interrupted, reason %d, cmd %d\n",
@@ -6379,7 +6384,7 @@ static int md_seq_show(struct seq_file *seq, void *v)
return 0;
}
- if (mddev_lock(mddev) < 0)
+ if (mddev_lock_interruptible(mddev) < 0)
return -EINTR;
if (mddev->pers || mddev->raid_disks || !list_empty(&mddev->disks)) {
-- 1.7.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] md: fix unchecked interruptible mutex locks
2011-04-14 6:48 [PATCH] md: fix unchecked interruptible mutex locks Alexey Khoroshilov
@ 2011-04-14 7:08 ` NeilBrown
2011-04-14 7:56 ` Alexey Khoroshilov
0 siblings, 1 reply; 3+ messages in thread
From: NeilBrown @ 2011-04-14 7:08 UTC (permalink / raw)
To: Alexey Khoroshilov; +Cc: linux-raid, linux-kernel
On Thu, 14 Apr 2011 10:48:30 +0400 Alexey Khoroshilov <khoroshilov@ispras.ru>
wrote:
> mddev_lock() is an alias for mutex_lock_interruptible() at the moment,
> but there are places where returned value of mddev_lock() is not checked.
>
> The patch proposed introduces mddev_lock_interruptible(),
> which is an equivalent for old mddev_lock(), while mddev_lock()
> becomes an alias for uninterruptible mutex_lock().
> Calls of mddev_lock(), where the result is being checked,
> are replaced by mddev_lock_interruptible().
> All the other calls become uninterruptible.
Thanks for reporting this.
I don't agree with your solution though.
I would mark mddev_lock as __must_check__ (or however it is spelt) and fix
the errors produced.
I think the call in md_stop_writes is the only one where an uninterruptible
wait is needed - I'd probaby just change that to an explicit "mutex_lock",
but I wouldn't be against creating "mddev_lock_uninterruptible" for that case.
NeilBrown
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> ---
> drivers/md/md.c | 21 +++++++++++++--------
> 1 files changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index b12b377..cc6d183 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -623,7 +623,12 @@ static mddev_t * mddev_find(dev_t unit)
> goto retry;
> }
>
> -static inline int mddev_lock(mddev_t * mddev)
> +static inline void mddev_lock(mddev_t *mddev)
> +{
> + return mutex_lock(&mddev->reconfig_mutex);
> +}
> +
> +static inline int __must_check mddev_lock_interruptible(mddev_t *mddev)
> {
> return mutex_lock_interruptible(&mddev->reconfig_mutex);
> }
> @@ -2706,7 +2711,7 @@ rdev_attr_show(struct kobject *kobj, struct
> attribute *attr, char *page)
> if (!entry->show)
> return -EIO;
>
> - rv = mddev ? mddev_lock(mddev) : -EBUSY;
> + rv = mddev ? mddev_lock_interruptible(mddev) : -EBUSY;
> if (!rv) {
> if (rdev->mddev == NULL)
> rv = -EBUSY;
> @@ -2730,7 +2735,7 @@ rdev_attr_store(struct kobject *kobj, struct
> attribute *attr,
> return -EIO;
> if (!capable(CAP_SYS_ADMIN))
> return -EACCES;
> - rv = mddev ? mddev_lock(mddev): -EBUSY;
> + rv = mddev ? mddev_lock_interruptible(mddev) : -EBUSY;
> if (!rv) {
> if (rdev->mddev == NULL)
> rv = -EBUSY;
> @@ -4199,7 +4204,7 @@ md_attr_show(struct kobject *kobj, struct
> attribute *attr, char *page)
>
> if (!entry->show)
> return -EIO;
> - rv = mddev_lock(mddev);
> + rv = mddev_lock_interruptible(mddev);
> if (!rv) {
> rv = entry->show(mddev, page);
> mddev_unlock(mddev);
> @@ -4219,7 +4224,7 @@ md_attr_store(struct kobject *kobj, struct
> attribute *attr,
> return -EIO;
> if (!capable(CAP_SYS_ADMIN))
> return -EACCES;
> - rv = mddev_lock(mddev);
> + rv = mddev_lock_interruptible(mddev);
> if (mddev->hold_active == UNTIL_IOCTL)
> mddev->hold_active = 0;
> if (!rv) {
> @@ -4951,7 +4956,7 @@ static void autorun_devices(int part)
> "md: cannot allocate memory for md drive.\n");
> break;
> }
> - if (mddev_lock(mddev))
> + if (mddev_lock_interruptible(mddev))
> printk(KERN_WARNING "md: %s locked, cannot run\n",
> mdname(mddev));
> else if (mddev->raid_disks || mddev->major_version
> @@ -5771,7 +5776,7 @@ static int md_ioctl(struct block_device *bdev,
> fmode_t mode,
> goto abort;
> }
>
> - err = mddev_lock(mddev);
> + err = mddev_lock_interruptible(mddev);
> if (err) {
> printk(KERN_INFO
> "md: ioctl lock interrupted, reason %d, cmd %d\n",
> @@ -6379,7 +6384,7 @@ static int md_seq_show(struct seq_file *seq, void *v)
> return 0;
> }
>
> - if (mddev_lock(mddev) < 0)
> + if (mddev_lock_interruptible(mddev) < 0)
> return -EINTR;
>
> if (mddev->pers || mddev->raid_disks || !list_empty(&mddev->disks)) {
> -- 1.7.1
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] md: fix unchecked interruptible mutex locks
2011-04-14 7:08 ` NeilBrown
@ 2011-04-14 7:56 ` Alexey Khoroshilov
0 siblings, 0 replies; 3+ messages in thread
From: Alexey Khoroshilov @ 2011-04-14 7:56 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid, linux-kernel
On 04/14/2011 11:08 AM, NeilBrown wrote:
>
> I would mark mddev_lock as __must_check__ (or however it is spelt) and fix
> the errors produced.
>
> I think the call in md_stop_writes is the only one where an uninterruptible
> wait is needed - I'd probaby just change that to an explicit "mutex_lock",
> but I wouldn't be against creating "mddev_lock_uninterruptible" for that case.
>
That is fine for me, but actually I do not see a good way to fix
rdev_size_store as well (at least for mddev_lock(my_mddev)).
Please find another try.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
drivers/md/md.c | 22 ++++++++++++++++------
1 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index b12b377..4f83f16 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -623,11 +623,16 @@ static mddev_t * mddev_find(dev_t unit)
goto retry;
}
-static inline int mddev_lock(mddev_t * mddev)
+static inline int __must_check mddev_lock(mddev_t *mddev)
{
return mutex_lock_interruptible(&mddev->reconfig_mutex);
}
+static inline void mddev_lock_uninterruptible(mddev_t *mddev)
+{
+ return mutex_lock(&mddev->reconfig_mutex);
+}
+
static inline int mddev_is_locked(mddev_t *mddev)
{
return mutex_is_locked(&mddev->reconfig_mutex);
@@ -2610,13 +2615,18 @@ rdev_size_store(mdk_rdev_t *rdev, const char *buf, size_t len)
*/
mddev_t *mddev;
int overlap = 0;
+ int intr = 0;
struct list_head *tmp;
mddev_unlock(my_mddev);
for_each_mddev(mddev, tmp) {
mdk_rdev_t *rdev2;
- mddev_lock(mddev);
+ if (mddev_lock(mddev)) {
+ mddev_put(mddev);
+ intr = 1;
+ break;
+ }
list_for_each_entry(rdev2, &mddev->disks, same_set)
if (rdev->bdev == rdev2->bdev &&
rdev != rdev2 &&
@@ -2632,8 +2642,8 @@ rdev_size_store(mdk_rdev_t *rdev, const char *buf, size_t len)
break;
}
}
- mddev_lock(my_mddev);
- if (overlap) {
+ mddev_lock_uninterruptible(my_mddev);
+ if (overlap || intr) {
/* Someone else could have slipped in a size
* change here, but doing so is just silly.
* We put oldsectors back because we *know* it is
@@ -2641,7 +2651,7 @@ rdev_size_store(mdk_rdev_t *rdev, const char *buf, size_t len)
* itself
*/
rdev->sectors = oldsectors;
- return -EBUSY;
+ return overlap ? -EBUSY : -EINTR;
}
}
return len;
@@ -4748,7 +4758,7 @@ static void __md_stop_writes(mddev_t *mddev)
void md_stop_writes(mddev_t *mddev)
{
- mddev_lock(mddev);
+ mddev_lock_uninterruptible(mddev);
__md_stop_writes(mddev);
mddev_unlock(mddev);
}
--
1.7.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-04-14 7:56 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-04-14 6:48 [PATCH] md: fix unchecked interruptible mutex locks Alexey Khoroshilov
2011-04-14 7:08 ` NeilBrown
2011-04-14 7:56 ` Alexey Khoroshilov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.