md: fix unchecked interruptible mutex locks

PATCH In review Alexey Khoroshilov 2 participants 3 messages Apr 14, 2011 MD RAID

Share & tools

Alexey Khoroshilov

author

2011-04-14 06:48 UTC #0 lore raw

PATCH 1/1 md: fix unchecked interruptible mutex locks

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 <[email protected]>

---
 drivers/md/md.c |   21 +++++++++++++--------
 1 files changed, 13 insertions(+), 8 deletions(-)

drivers/md/md.c +6 -1

index b12b377..cc6d183 100644
@@ -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

NeilBrown

2011-04-14 07:08 UTC #1 lore raw

Quoted 9 lines · On Thu, 14 Apr 2011 10:48:30 +0400 Alexey Khoroshilov <[email protected]>…

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

Quoted 95 lines

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by Alexey Khoroshilov <[email protected]>

---
 drivers/md/md.c |   21 +++++++++++++--------
 1 files changed, 13 insertions(+), 8 deletions(-)

drivers/md/md.c +7 -2

index b12b377..cc6d183 100644
@@ -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)

drivers/md/md.c +1 -1

 			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)

drivers/md/md.c +1 -1

 			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);

drivers/md/md.c +1 -1

 		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;

drivers/md/md.c +2 -2

 	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

drivers/md/md.c +1 -2

 			"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

Alexey Khoroshilov

author

2011-04-14 07:56 UTC #2 lore raw

Quoted 8 lines · NeilBrown

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 <[email protected]>

---
 drivers/md/md.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

drivers/md/md.c +16 -6

index b12b377..4f83f16 100644
@@ -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);
 }

Signature · 1 line

1.7.1