[fixes] Re: Locking bugs in 2.4 md.c

[Lars Marowsky-Bree <lmb@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 946 bytes --]

On 2003-09-19T12:08:29,
   Neil Brown <neilb@cse.unsw.edu.au> said:

Hi Neil,

your patch needs the following two addendums.

Andi Kleen: Locking in md_seq_next is wrong, should happen in
md_seq_start

Andrea & I: Refcounting for the raidautostart feature is wrong.
This is actually a long-standing bug which is catched by your diffs,
which notice the wrong counter and BUG().

What would happen: user-space open()s /dev/md0, issues
ioctl(RAID_AUTOSTART) - which creates all md devices and sets active ==
0 ultimately -, and then close()s /dev/md0, which decrements active to
-1. Oops next time md_make_request gets called.

Patch fixes this and adds safeguard to md_release() already.


Sincerely,
    Lars Marowsky-Brée <lmb@suse.de>

-- 
High Availability & Clustering		ever tried. ever failed. no matter.
SuSE Labs				try again. fail again. fail better.
Research & Development, SUSE LINUX AG		-- Samuel Beckett


[-- Attachment #1.2: md-seqlock --]
[-- Type: text/plain, Size: 442 bytes --]

--- linux/drivers/md/md.c-o	2003-09-24 09:45:39.000000000 +0000
+++ linux/drivers/md/md.c	2003-09-24 09:47:23.000000000 +0000
@@ -3287,6 +3287,7 @@
 
 	if (l > 0x10000)
 		return NULL;
+	down_read(&all_mddevs_sem);
 	if (!l--)
 		/* header */
 		return (void*)1;
@@ -3304,7 +3305,6 @@
 	struct list_head *tmp;
 	mddev_t *next_mddev, *mddev = v;
 
-	down_read(&all_mddevs_sem);
 	++*pos;
 	if (v == (void*)2)
 		return NULL;

[-- Attachment #1.3: md-refcount-fix --]
[-- Type: text/plain, Size: 1351 bytes --]

--- linux-2.4.21/drivers/md/md.c	2003-09-24 10:54:35.000000000 +0200
+++ linux-2.4.21.new/drivers/md/md.c	2003-09-24 10:53:34.000000000 +0200
@@ -60,7 +60,7 @@
 #endif
 
 #ifndef MODULE
-static void autostart_arrays (void);
+static void autostart_arrays (kdev_t);
 #endif
 
 static mdk_personality_t *pers[MAX_PERSONALITY];
@@ -2753,7 +2753,7 @@
 #ifndef MODULE
 		case RAID_AUTORUN:
 			err = 0;
-			autostart_arrays();
+			autostart_arrays(dev);
 			goto done;
 #endif
 
@@ -3017,8 +3017,11 @@
 {
 	mddev_t *mddev = kdev_to_mddev(inode->i_rdev);
 	if (mddev) {
-		atomic_dec(&mddev->active);
-		atomic_dec(&mddev->active);
+		if (atomic_dec_and_test(&mddev->active))
+			/* Refcount should _never_ drop to zero here! */
+			BUG();
+		else
+			atomic_dec(&mddev->active);
 	}
 	return 0;
 }
@@ -3923,7 +3926,7 @@
 }
 
 
-static void autostart_arrays(void)
+static void autostart_arrays(kdev_t countdev)
 {
 	mdk_rdev_t *rdev;
 	int i;
@@ -3954,7 +3957,7 @@
 	}
 	dev_cnt = 0;
 
-	autorun_devices(-1);
+	autorun_devices(countdev);
 }
 
 static struct {
@@ -4183,7 +4186,7 @@
 	if (raid_setup_args.noautodetect)
 		printk(KERN_INFO "md: Skipping autodetection of RAID arrays. (raid=noautodetect)\n");
 	else
-		autostart_arrays();
+		autostart_arrays(-1);
 	md_setup_drive();
 	return 0;
 }

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]