Re: [PATCH 002 of 6] md: Fix use-after-free bug when dropping an rdev from an md array.

Previous thread: [git pull] Input updates for 2.6.24-rc3 by Dmitry Torokhov on Sunday, January 13, 2008 - 11:01 pm. (2 messages)

Next thread: [PATCH 1/3] hotplug cpu move tasks in empty cpusets to parent node_online_map fix by Paul Jackson on Sunday, January 13, 2008 - 11:41 pm. (1 message)
From: Neil Brown
Date: Sunday, January 13, 2008 - 11:28 pm

How about this, against current -mm

On both the read and write path for an rdev attribute, we
call mddev_lock, first checking that mddev is not NULL.
Once we get the lock, we check again.
If rdev->mddev is not NULL, we know it will stay that way as it only
gets cleared under the same lock.

While in the rdev show/store routines, we know that the mddev cannot
get freed, do to the kobject relationships.

rdev_size_store is awkward because it has to drop the lock.  So we
take a copy of rdev->mddev before the drop, and we are safe...

Comments?

NeilBrown

Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./drivers/md/md.c |   35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff .prev/drivers/md/md.c ./drivers/md/md.c
--- .prev/drivers/md/md.c	2008-01-14 12:26:15.000000000 +1100
+++ ./drivers/md/md.c	2008-01-14 17:05:53.000000000 +1100
@@ -1998,9 +1998,11 @@ rdev_size_store(mdk_rdev_t *rdev, const 
 	char *e;
 	unsigned long long size = simple_strtoull(buf, &e, 10);
 	unsigned long long oldsize = rdev->size;
+	mddev_t *my_mddev = rdev->mddev;
+
 	if (e==buf || (*e && *e != '\n'))
 		return -EINVAL;
-	if (rdev->mddev->pers)
+	if (my_mddev->pers)
 		return -EBUSY;
 	rdev->size = size;
 	if (size > oldsize && rdev->mddev->external) {
@@ -2013,7 +2015,7 @@ rdev_size_store(mdk_rdev_t *rdev, const 
 		int overlap = 0;
 		struct list_head *tmp, *tmp2;
 
-		mddev_unlock(rdev->mddev);
+		mddev_unlock(my_mddev);
 		for_each_mddev(mddev, tmp) {
 			mdk_rdev_t *rdev2;
 
@@ -2033,7 +2035,7 @@ rdev_size_store(mdk_rdev_t *rdev, const 
 				break;
 			}
 		}
-		mddev_lock(rdev->mddev);
+		mddev_lock(my_mddev);
 		if (overlap) {
 			/* Someone else could have slipped in a size
 			 * change here, but doing so is just silly.
@@ -2045,8 +2047,8 @@ rdev_size_store(mdk_rdev_t *rdev, const 
 			return -EBUSY;
 		}
 	}
-	if (size < rdev->mddev->size || rdev->mddev->size == 0)
-		rdev->mddev->size = size;
+	if (size ...
From: Al Viro
Date: Monday, January 14, 2008 - 5:59 am

[Empty message]
From: Al Viro
Date: Monday, January 14, 2008 - 6:56 am

More fun questions: what are the locking requirements for ->resize()?
You are calling it with no exclusion whatsoever...  What about
bind_rdev_to_array()?  At the very least, you want to protect
mddev->disks, and AFAICS new_dev_store() has no exclusion at all.
And I suspect that you have other things in there in need of protection
(finding free desc_nr, for one); can all of those be handled by simple
spinlocks?
--

Previous thread: [git pull] Input updates for 2.6.24-rc3 by Dmitry Torokhov on Sunday, January 13, 2008 - 11:01 pm. (2 messages)

Next thread: [PATCH 1/3] hotplug cpu move tasks in empty cpusets to parent node_online_map fix by Paul Jackson on Sunday, January 13, 2008 - 11:41 pm. (1 message)