Do not call drivers when invalidating partitions for -ENOMEDIUM

2 files changed, 197 insertions, 5 deletions

diff --git a/kernel.spec b/kernel.spec
index 80fc7b95a..c4bf4b5dc 100644
--- a/kernel.spec
+++ b/kernel.spec
@@ -762,18 +762,19 @@ Patch21234: Bluetooth-Remove-bogus-inline-decl-from-l2cap_chan_connect.patch
 
 #rhbz 754518
 Patch21235: scsi-sd_revalidate_disk-prevent-NULL-ptr-deref.patch
+Patch21236: scsi-fix-sd_revalidate_disk-oops.patch
 
 #rhbz 714828
-Patch21236: autofs4-lockdep.patch
+Patch21240: autofs4-lockdep.patch
 
-Patch21237: mcelog-rcu-splat.patch
-Patch21238: x86-Avoid-invoking-RCU-when-CPU-is-idle.patch
+Patch21250: mcelog-rcu-splat.patch
+Patch21260: x86-Avoid-invoking-RCU-when-CPU-is-idle.patch
 
 #rhbz 790367
-Patch21239: s390x-enable-keys-compat.patch
+Patch21270: s390x-enable-keys-compat.patch
 
 #rhbz 795544
-Patch21240: ums_realtek-do-not-use-stack-memory-for-DMA-in-__do_.patch
+Patch21280: ums_realtek-do-not-use-stack-memory-for-DMA-in-__do_.patch
 
 # compat-wireless patches
 Patch50000: compat-wireless-config-fixups.patch
@@ -1485,6 +1486,7 @@ ApplyPatch Bluetooth-Remove-bogus-inline-decl-from-l2cap_chan_connect.patch
 
 #rhbz 754518
 ApplyPatch scsi-sd_revalidate_disk-prevent-NULL-ptr-deref.patch
+ApplyPatch scsi-fix-sd_revalidate_disk-oops.patch
 
 #rhbz 714828
 ApplyPatch autofs4-lockdep.patch
@@ -2376,6 +2378,9 @@ fi
 #                 ||----w |
 #                 ||     ||
 %changelog
+* Mon Feb 20 2012 Dave Jones <davej@redhat.com>
+- Do not call drivers when invalidating partitions for -ENOMEDIUM
+
 * Mon Feb 20 2012 Josh Boyer <jwboyer@redhat.com>
 - Avoid using stack variables in ums_realtek (again) (rhbz 795544)
 
diff --git a/scsi-fix-sd_revalidate_disk-oops.patch b/scsi-fix-sd_revalidate_disk-oops.patch
new file mode 100644
index 000000000..efc657c7f
--- /dev/null
+++ b/scsi-fix-sd_revalidate_disk-oops.patch
@@ -0,0 +1,187 @@
+Hi,
+
+Thank you for review and comments.
+
+On 02/16/12 02:26, Tejun Heo wrote:
+> On Wed, Feb 15, 2012 at 11:56:19AM +0900, Jun'ichi Nomura wrote:
+>> +int invalidate_partitions(struct gendisk *disk, struct block_device *bdev)
+>> +{
+>> +	int res;
+>> +
+>> +	res = drop_partitions(disk, bdev);
+>> +	if (res)
+>> +		return res;
+>> +
+> 
+> Hmmm... shouldn't we have set_capacity(disk, 0) here?
+
+Added.
+I wasn't sure whether I should leave it to drivers.
+But it seems capacity 0 for ENOMEDIUM device is reasonable.
+
+>> +	check_disk_size_change(disk, bdev);
+>> +	bdev->bd_invalidated = 0;
+>> +	/* tell userspace that the media / partition table may have changed */
+>> +	kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE);
+> 
+> Also, we really shouldn't be generating KOBJ_CHANGE after every
+> -ENOMEDIUM open.  This can easily lead to infinite loop.  We should
+> generate this iff we actually dropped partitions && modified the size.
+
+invalidate_partitions() is called only when bd_invalidated is set.
+So KOBJ_CHANGE is not raised for every ENOMEDIUM open.
+
+I put it explicit in the function to make it safer for
+possible misuse.
+
+How about this?
+
+---------------------------------------------------------
+Do not call drivers when invalidating partitions for -ENOMEDIUM
+
+When a scsi driver returns -ENOMEDIUM for open(),
+__blkdev_get() calls rescan_partitions(), which ends up calling
+sd_revalidate_disk() without getting a refcount of scsi_device.
+
+That could lead to oops like this:
+
+  process A                  process B
+  ----------------------------------------------
+  sys_open
+    __blkdev_get
+      sd_open
+        returns -ENOMEDIUM
+                             scsi_remove_device
+                               <scsi_device torn down>
+      rescan_partitions
+        sd_revalidate_disk
+          <oops>
+
+Oopses are reported here:
+http://marc.info/?l=linux-scsi&m=132388619710052
+
+This patch separates the partition invalidation from rescan_partitions()
+and use it for -ENOMEDIUM case. 
+
+Index: linux-3.3/block/partition-generic.c
+===================================================================
+--- linux-3.3.orig/block/partition-generic.c	2012-02-15 09:00:25.147293790 +0900
++++ linux-3.3/block/partition-generic.c	2012-02-16 10:48:22.257680685 +0900
+@@ -389,17 +389,11 @@ static bool disk_unlock_native_capacity(
+ 	}
+ }
+ 
+-int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
++static int drop_partitions(struct gendisk *disk, struct block_device *bdev)
+ {
+-	struct parsed_partitions *state = NULL;
+ 	struct disk_part_iter piter;
+ 	struct hd_struct *part;
+-	int p, highest, res;
+-rescan:
+-	if (state && !IS_ERR(state)) {
+-		kfree(state);
+-		state = NULL;
+-	}
++	int res;
+ 
+ 	if (bdev->bd_part_count)
+ 		return -EBUSY;
+@@ -412,6 +406,24 @@ rescan:
+ 		delete_partition(disk, part->partno);
+ 	disk_part_iter_exit(&piter);
+ 
++	return 0;
++}
++
++int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
++{
++	struct parsed_partitions *state = NULL;
++	struct hd_struct *part;
++	int p, highest, res;
++rescan:
++	if (state && !IS_ERR(state)) {
++		kfree(state);
++		state = NULL;
++	}
++
++	res = drop_partitions(disk, bdev);
++	if (res)
++		return res;
++
+ 	if (disk->fops->revalidate_disk)
+ 		disk->fops->revalidate_disk(disk);
+ 	check_disk_size_change(disk, bdev);
+@@ -515,6 +527,26 @@ rescan:
+ 	return 0;
+ }
+ 
++int invalidate_partitions(struct gendisk *disk, struct block_device *bdev)
++{
++	int res;
++
++	if (!bdev->bd_invalidated)
++		return 0;
++
++	res = drop_partitions(disk, bdev);
++	if (res)
++		return res;
++
++	set_capacity(disk, 0);
++	check_disk_size_change(disk, bdev);
++	bdev->bd_invalidated = 0;
++	/* tell userspace that the media / partition table may have changed */
++	kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE);
++
++	return 0;
++}
++
+ unsigned char *read_dev_sector(struct block_device *bdev, sector_t n, Sector *p)
+ {
+ 	struct address_space *mapping = bdev->bd_inode->i_mapping;
+Index: linux-3.3/include/linux/genhd.h
+===================================================================
+--- linux-3.3.orig/include/linux/genhd.h	2012-02-09 12:21:53.000000000 +0900
++++ linux-3.3/include/linux/genhd.h	2012-02-16 10:47:43.783681813 +0900
+@@ -596,6 +596,7 @@ extern char *disk_name (struct gendisk *
+ 
+ extern int disk_expand_part_tbl(struct gendisk *disk, int target);
+ extern int rescan_partitions(struct gendisk *disk, struct block_device *bdev);
++extern int invalidate_partitions(struct gendisk *disk, struct block_device *bdev);
+ extern struct hd_struct * __must_check add_partition(struct gendisk *disk,
+ 						     int partno, sector_t start,
+ 						     sector_t len, int flags,
+Index: linux-3.3/fs/block_dev.c
+===================================================================
+--- linux-3.3.orig/fs/block_dev.c	2012-02-09 12:21:53.000000000 +0900
++++ linux-3.3/fs/block_dev.c	2012-02-16 10:47:52.602681441 +0900
+@@ -1183,8 +1183,12 @@ static int __blkdev_get(struct block_dev
+ 			 * The latter is necessary to prevent ghost
+ 			 * partitions on a removed medium.
+ 			 */
+-			if (bdev->bd_invalidated && (!ret || ret == -ENOMEDIUM))
+-				rescan_partitions(disk, bdev);
++			if (bdev->bd_invalidated) {
++				if (!ret)
++					rescan_partitions(disk, bdev);
++				else if (ret == -ENOMEDIUM)
++					invalidate_partitions(disk, bdev);
++			}
+ 			if (ret)
+ 				goto out_clear;
+ 		} else {
+@@ -1214,8 +1218,12 @@ static int __blkdev_get(struct block_dev
+ 			if (bdev->bd_disk->fops->open)
+ 				ret = bdev->bd_disk->fops->open(bdev, mode);
+ 			/* the same as first opener case, read comment there */
+-			if (bdev->bd_invalidated && (!ret || ret == -ENOMEDIUM))
+-				rescan_partitions(bdev->bd_disk, bdev);
++			if (bdev->bd_invalidated) {
++				if (!ret)
++					rescan_partitions(bdev->bd_disk, bdev);
++				else if (ret == -ENOMEDIUM)
++					invalidate_partitions(bdev->bd_disk, bdev);
++			}
+ 			if (ret)
+ 				goto out_unlock_bdev;
+ 		}