nbd driver for 2.5+: fix for module removal & new block device layer

This patch prevents memory corruption from "rmmod nbd" with the existing 2.5.72 (and earlier) nbd driver. It does this by updating the nbd driver to the new block layer requirement that every disk has its own request_queue structure. This is the first of a series of patchlets designed to break down the essential changes I proposed in my last "enormous" patch. Note that another patchlet will make the whole allocation of per nbd_device memory be dynamic (rather than staticly tied to MAX_NBD). Please try out this patch and let me know how nbd is working for you before versus after. With any luck, some of these smaller patch breakdowns can actually see there way into new kernel releases. Thanks.

diff -urN linux-2.5.72/drivers/block/nbd.c linux-2.5.72-patched/drivers/block/nbd.c
@@ -28,6 +28,7 @@
  *   the transmit lock. <[email protected]>
  * 02-10-11 Allow hung xmit to be aborted via SIGKILL & various fixes.
  *   <[email protected]> <[email protected]>
+ * 03-06-21 Fix memory corruption from module removal. <[email protected]>
  *
  * possible FIXME: make set_sock / set_blksize / set_size / do_it one syscall
  * why not: would need verify_area and friends, would share yet another 

@@ -63,7 +64,18 @@
 
 static struct nbd_device nbd_dev[MAX_NBD];
 
-static spinlock_t nbd_lock = SPIN_LOCK_UNLOCKED;
+/*
+ * Have these per nbd_device as is now required by the new block layer.
+ * This also helps prevent I/O bottle necks between multiple nbd_devices
+ * resulting in better overall response times!
+ *
+ * ldl: Keep these out from nbd_device for now till the whole 2.5 blockdev
+ *   reworking shakes out. Who knows... maybe struct request_queue's
+ *   queue_lock field will someday actually be the spinlock instead of just
+ *   being a pointer to it!
+ */
+static struct request_queue nbd_queue[MAX_NBD];
+static spinlock_t nbd_lock[MAX_NBD];
 
 #define DEBUG( s )
 /* #define DEBUG( s ) printk( s ) 

@@ -538,8 +550,6 @@
  *  (Just smiley confuses emacs :-)
  */
 
-static struct request_queue nbd_queue;
-
 static int __init nbd_init(void)
 {
 	int err = -ENOMEM;

@@ -551,6 +561,11 @@
 	}
 
 	for (i = 0; i < MAX_NBD; i++) {
+		nbd_lock[i] = SPIN_LOCK_UNLOCKED;
+		blk_init_queue(&nbd_queue[i], do_nbd_request, &nbd_lock[i]);
+	}
+
+	for (i = 0; i < MAX_NBD; i++) {
 		struct gendisk *disk = alloc_disk(1);
 		if (!disk)
 			goto out;

@@ -564,7 +579,6 @@
 #ifdef MODULE
 	printk("nbd: registered device at major %d\n", NBD_MAJOR);
 #endif
-	blk_init_queue(&nbd_queue, do_nbd_request, &nbd_lock);
 	devfs_mk_dir("nbd");
 	for (i = 0; i < MAX_NBD; i++) {
 		struct gendisk *disk = nbd_dev[i].disk;

@@ -582,7 +596,7 @@
 		disk->first_minor = i;
 		disk->fops = &nbd_fops;
 		disk->private_data = &nbd_dev[i];
-		disk->queue = &nbd_queue;
+		disk->queue = &nbd_queue[i];
 		sprintf(disk->disk_name, "nbd%d", i);
 		sprintf(disk->devfs_name, "nbd/%d", i);
 		set_capacity(disk, 0x3ffffe);

@@ -602,9 +616,9 @@
 	for (i = 0; i < MAX_NBD; i++) {
 		del_gendisk(nbd_dev[i].disk);
 		put_disk(nbd_dev[i].disk);
+		blk_cleanup_queue(&nbd_queue[i]);
 	}
 	devfs_remove("nbd");
-	blk_cleanup_queue(&nbd_queue);
 	unregister_blkdev(NBD_MAJOR, "nbd");
 }
 

a) you don't have to have queue per device. It often does make
sense (for nbd it's almost certainly a win), but it's not required.

b) you definitely don't have to use separate queue locks. The
thing will work fine with spinlock being shared and I doubt that there will be any noticable extra contention.

c) please, make allocation of queue dynamic _and_ separate from
any other allocated objects.

Unfortunately the way the sysfs code is implemented for struct gendisk it is actually the case that every gendisk has to have its own request_queue or else dcache.c BUG's and memory gets corrupted when you put and release this disks:

Jun 21 12:59:07 cyprus kernel: nbd: registered device at major 43
Jun 21 12:59:25 cyprus kernel: ------------[ cut here ]------------
Jun 21 12:59:25 cyprus kernel: kernel BUG at include/linux/dcache.h:273!
Jun 21 12:59:25 cyprus kernel: invalid operand: 0000 [#1]
Jun 21 12:59:25 cyprus kernel: CPU:    0
Jun 21 12:59:25 cyprus kernel: EIP:    0060:[<c017849d>]    Tainted: GF
Jun 21 12:59:25 cyprus kernel: EFLAGS: 00210246
Jun 21 12:59:25 cyprus kernel: EIP is at sysfs_remove_dir+0x1d/0x13f
Jun 21 12:59:25 cyprus kernel: eax: 00000000   ebx: e0947ac4   ecx: c026b6e0   edx: 00000000
Jun 21 12:59:25 cyprus kernel: esi: 00000078   edi: db10b900   ebp: d1f2fee0   esp: d1f2fec8
Jun 21 12:59:25 cyprus kernel: ds: 007b   es: 007b   ss: 0068
Jun 21 12:59:25 cyprus kernel: Process rmmod (pid: 3794, threadinfo=d1f2e000 task=cf2bb300)
Jun 21 12:59:25 cyprus kernel: Stack: d4a45d00 dfda21c0 dfda21e8 e0947ac4 00000078 d0c10344 d1f2fef8 c0205873
Jun 21 12:59:25 cyprus kernel:        e0947ac4 c0450280 e0947ac4 e0947ac4 d1f2ff08 c02058b4 e0947ac4 d0c10280
Jun 21 12:59:25 cyprus kernel:        d1f2ff18 c02673ee e0947ac4 d0c10280 d1f2ff2c c026b224 d0c10280 d0c10280
Jun 21 12:59:25 cyprus kernel: Call Trace:
Jun 21 12:59:25 cyprus kernel:  [<e0947ac4>] nbd_queue+0x44/0x180 [nbd]
Jun 21 12:59:25 cyprus kernel:  [<c0205873>] kobject_del+0x43/0x70
Jun 21 12:59:25 cyprus kernel:  [<e0947ac4>] nbd_queue+0x44/0x180 [nbd]
Jun 21 12:59:25 cyprus last message repeated 2 times
Jun 21 12:59:25 cyprus kernel:  [<c02058b4>] kobject_unregister+0x14/0x20
Jun 21 12:59:25 cyprus kernel:  [<e0947ac4>] nbd_queue+0x44/0x180 [nbd]
Jun 21 12:59:25 cyprus kernel:  [<c02673ee>] elv_unregister_queue+0x1e/0x40
Jun 21 12:59:25 cyprus kernel:  [<e0947ac4>] nbd_queue+0x44/0x180 [nbd]
Jun 21 12:59:25 cyprus kernel:  [<c026b224>] unlink_gendisk+0x14/0x40
Jun 21 12:59:25 cyprus kernel:  [<c0177301>] del_gendisk+0x61/0xe0
Jun 21 12:59:25 cyprus kernel:  [<e0945a5c>] nbd_dev+0x1dc/0x2200 [nbd]
Jun 21 12:59:25 cyprus kernel:  [<e0944c5d>] +0x1d/0x60 [nbd]
Jun 21 12:59:25 cyprus kernel:  [<e0947c00>] +0x0/0x140 [nbd]
Jun 21 12:59:25 cyprus kernel:  [<c012ce9e>] sys_delete_module+0x12e/0x170
Jun 21 12:59:25 cyprus kernel:  [<e0947c00>] +0x0/0x140 [nbd]
Jun 21 12:59:25 cyprus kernel:  [<c013e5c7>] sys_munmap+0x57/0x80
Jun 21 12:59:25 cyprus kernel:  [<c010ac4d>] sysenter_past_esp+0x52/0x71
Jun 21 12:59:25 cyprus kernel:
Jun 21 12:59:25 cyprus kernel: Code: 0f 0b 11 01 3b fa 3a c0 ff 07 85 ff 0f 84 08 01 00 00 8b 47

Probably not noticeable no.

Will do!

The approach you took is probably _worse_ than a single lock, since you don't even cache align the locks. I'd say just keep the single nbd_lock and use that in all queues, seperate locks are a questionable win but do take up extra space.

Jens Axboe

Here's a possible patch #2... I believe the address pointed to by bio_data(bio) is not always contiguous over the length of bio->bi_size and was responsible for locking my machine up sometimes. My biggest reason for apprehension on believeing that I'm 100% correct on this is that there's still what appears to be a source of memory corruption in the patchlet modified nbd driver even after this patch. I know the driver is still not correctly notifying processes of the bytesize on open but the size reported appears to be big enough. Anyway, thanks for all of the feedback so far!!!!

diff -urN linux-2.5.72-p1/drivers/block/nbd.c linux-2.5.72-p2/drivers/block/nbd.c
@@ -28,7 +28,10 @@
  *   the transmit lock. <[email protected]>
  * 02-10-11 Allow hung xmit to be aborted via SIGKILL & various fixes.
  *   <[email protected]> <[email protected]>
- * 03-06-21 Fix memory corruption from module removal. <[email protected]>
+ * 03-06-21 Make nbd work with linux 2.5 block layer code. This fixes memory
+ *   corruption from module removal too. <[email protected]>
+ * 03-06-21 Fix incorrect bio struct access that could have lead to memory
+ *   corruption on receiving disk data. <[email protected]>
  *
  * possible FIXME: make set_sock / set_blksize / set_size / do_it one syscall
  * why not: would need verify_area and friends, would share yet another 

@@ -256,6 +259,12 @@
 	return NULL;
 }
 
+static inline int sock_recv_bvec(struct socket *sock, struct bio_vec *bvec)
+{
+	return nbd_xmit(0, sock, page_address(bvec->bv_page) + bvec->bv_offset,
+			bvec->bv_len, MSG_WAITALL);
+}
+
 #define HARDFAIL( s ) { printk( KERN_ERR "NBD: " s "(result %d)\n", result ); lo->harderror = result; return NULL; }
 struct request *nbd_read_stat(struct nbd_device *lo)
 		/* NULL returned = something went wrong, inform userspace       */ 

@@ -263,10 +272,11 @@
 	int result;
 	struct nbd_reply reply;
 	struct request *req;
+	struct socket *sock = lo->sock;
 
 	DEBUG("reading control, ");
 	reply.magic = 0;
-	result = nbd_xmit(0, lo->sock, (char *) &reply, sizeof(reply), MSG_WAITALL);
+	result = nbd_xmit(0, sock, (char *) &reply, sizeof(reply), MSG_WAITALL);
 	if (result <= 0)
 		HARDFAIL("Recv control failed.");
 	req = nbd_find_request(lo, reply.handle);

@@ -280,14 +290,17 @@
 		FAIL("Other side returned error.");
 
 	if (nbd_cmd(req) == NBD_CMD_READ) {
-		struct bio *bio = req->bio;
+		int i;
+		struct bio *bio;
 		DEBUG("data, ");
-		do {
-			result = nbd_xmit(0, lo->sock, bio_data(bio), bio->bi_size, MSG_WAITALL);
-			if (result <= 0)
-				HARDFAIL("Recv data failed.");
-			bio = bio->bi_next;
-		} while(bio);
+		rq_for_each_bio(bio, req) {
+			struct bio_vec *bvec;
+			bio_for_each_segment(bvec, bio, i) {
+				result = sock_recv_bvec(sock, bvec);
+				if (result <= 0)
+					HARDFAIL("Recv data failed.");
+			}
+		}
 	}
 	DEBUG("done.\n");
 	return req;

You are correct, the current code breaks down for multi-page bio's. It looks like you are missing a kmap still though, bvec->bv_page could be a highmem page.

Jens Axboe