[PATCH] fs/block_dev.c: Use hd_part to find block inodes

[PATCH] fs/block_dev.c: Use hd_part to find block inodes

[Keith Busch]

When using the GENHD_FL_EXT_DEVT disk flags, a newly added device may
be assigned the same major/minor as one that was previously removed but
opened, and the pesky userspace refuses to close it! The inode for the
old block_device is still open, and so bdget() finds the stale device
instead of allocating a new one. When the newly inserted drive is added,
you'll see a message like:

	nvme0n1: detected capacity change from XXX to 0

and the partitions on the disk will not be usable after that.

This patch uses the underlying disk's partition when trying to find
the block device's opened inode so that two different disks that have
a major/minor collision can coexist.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
Maybe this is terrible idea!?

This came from proposals to the nvme driver that remove the dynamic
partitioning that was recently added, and I wanted to know why exactly
it was failing.

I don't know if there is a good reason to avoid having two devices opened
with the same major/minor, but I think this is safe and tests out okay
when I force that condition.

In all cases, it appears getting the block device will eventually fail
if either the disk or part do not exist, so should be okay to bail and
assign these even earlier.

 fs/block_dev.c |   20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 6d72746..9ba2bc8 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -546,12 +546,15 @@ static inline unsigned long hash(dev_t dev)
 
 static int bdev_test(struct inode *inode, void *data)
 {
-	return BDEV_I(inode)->bdev.bd_dev == *(dev_t *)data;
+	return BDEV_I(inode)->bdev.bd_part == (struct hd_struct *)data;
 }
 
 static int bdev_set(struct inode *inode, void *data)
 {
-	BDEV_I(inode)->bdev.bd_dev = *(dev_t *)data;
+	struct hd_struct *part = (struct hd_struct *)data;;
+
+	BDEV_I(inode)->bdev.bd_part = part;
+	BDEV_I(inode)->bdev.bd_dev = part_devt(part);
 	return 0;
 }
 
@@ -561,9 +564,20 @@ struct block_device *bdget(dev_t dev)
 {
 	struct block_device *bdev;
 	struct inode *inode;
+	struct gendisk *disk;
+	struct hd_struct *part;
+	int partno;
+
+	disk = get_gendisk(dev, &partno);
+	if (!disk)
+		return NULL;
+
+	part = disk_get_part(disk, partno);
+	if (!part)
+		return NULL;
 
 	inode = iget5_locked(blockdev_superblock, hash(dev),
-			bdev_test, bdev_set, &dev);
+			bdev_test, bdev_set, part);
 
 	if (!inode)
 		return NULL;
-- 
1.7.10.4

Re: [PATCH] fs/block_dev.c: Use hd_part to find block inodes

[Christoph Hellwig]

On Fri, Aug 22, 2014 at 10:28:16AM -0600, Keith Busch wrote:
> When using the GENHD_FL_EXT_DEVT disk flags, a newly added device may
> be assigned the same major/minor as one that was previously removed but
> opened, and the pesky userspace refuses to close it!

Which means life time rules for those dev_t allocations are broken.
Please fix it to not release the dev_t until the device isn't referenced
at all.

Re: [PATCH] fs/block_dev.c: Use hd_part to find block inodes

[Keith Busch]

On Fri, 22 Aug 2014, Christoph Hellwig wrote:
> On Fri, Aug 22, 2014 at 10:28:16AM -0600, Keith Busch wrote:
>> When using the GENHD_FL_EXT_DEVT disk flags, a newly added device may
>> be assigned the same major/minor as one that was previously removed but
>> opened, and the pesky userspace refuses to close it!
>
> Which means life time rules for those dev_t allocations are broken.
> Please fix it to not release the dev_t until the device isn't referenced
> at all.

Okay, thanks. So a proper fix would not let extended devt minors get
reused while still referenced, so we can't release it unconditionally
from del_gendisk(). I think the following does that, but I had to add
a reference counter to gendisk.

---
diff --git a/block/genhd.c b/block/genhd.c
index 791f419..a41478a 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -453,6 +453,12 @@ void blk_free_devt(dev_t devt)
  	}
  }

+static void free_ext_dev(struct kref *kref)
+{
+	struct gendisk *disk = container_of(kref, struct gendisk, kref);
+	blk_free_devt(disk_to_dev(disk)->devt);
+}
+
  static char *bdevt_str(dev_t devt, char *buf)
  {
  	if (MAJOR(devt) <= 0xff && MINOR(devt) <= 0xff) {
@@ -665,7 +671,6 @@ void del_gendisk(struct gendisk *disk)
  		sysfs_remove_link(block_depr, dev_name(disk_to_dev(disk)));
  	pm_runtime_set_memalloc_noio(disk_to_dev(disk), false);
  	device_del(disk_to_dev(disk));
-	blk_free_devt(disk_to_dev(disk)->devt);
  }
  EXPORT_SYMBOL(del_gendisk);

@@ -1283,6 +1288,7 @@ struct gendisk *alloc_disk_node(int minors, int node_id)
  		disk_to_dev(disk)->class = &block_class;
  		disk_to_dev(disk)->type = &disk_type;
  		device_initialize(disk_to_dev(disk));
+		kref_init(&disk->kref);
  	}
  	return disk;
  }
@@ -1303,6 +1309,7 @@ struct kobject *get_disk(struct gendisk *disk)
  		module_put(owner);
  		return NULL;
  	}
+	kref_get(&disk->kref);
  	return kobj;

  }
@@ -1311,8 +1318,10 @@ EXPORT_SYMBOL(get_disk);

  void put_disk(struct gendisk *disk)
  {
-	if (disk)
+	if (disk) {
+		kref_put(&disk->kref, free_ext_dev);
  		kobject_put(&disk_to_dev(disk)->kobj);
+	}
  }

  EXPORT_SYMBOL(put_disk);
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index ec274e0..d51b099 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -200,6 +200,7 @@ struct gendisk {
  	struct blk_integrity *integrity;
  #endif
  	int node_id;
+	struct kref kref;
  };

  static inline struct gendisk *part_to_disk(struct hd_struct *part)
--

Re: [PATCH] fs/block_dev.c: Use hd_part to find block inodes

[Keith Busch]

On Fri, 22 Aug 2014, Keith Busch wrote:
> On Fri, 22 Aug 2014, Christoph Hellwig wrote:
>> On Fri, Aug 22, 2014 at 10:28:16AM -0600, Keith Busch wrote:
>>> When using the GENHD_FL_EXT_DEVT disk flags, a newly added device may
>>> be assigned the same major/minor as one that was previously removed but
>>> opened, and the pesky userspace refuses to close it!
>> 
>> Which means life time rules for those dev_t allocations are broken.
>> Please fix it to not release the dev_t until the device isn't referenced
>> at all.
>
> Okay, thanks. So a proper fix would not let extended devt minors get
> reused while still referenced, so we can't release it unconditionally
> from del_gendisk(). I think the following does that, but I had to add
> a reference counter to gendisk.

Sorry for the rapid churn; I hadn't followed the function pointers through
to discover the correct place to release the devt. This one's much simpler:
---
diff --git a/block/genhd.c b/block/genhd.c
index 791f419..321f1fd 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -665,7 +665,6 @@ void del_gendisk(struct gendisk *disk)
  		sysfs_remove_link(block_depr, dev_name(disk_to_dev(disk)));
  	pm_runtime_set_memalloc_noio(disk_to_dev(disk), false);
  	device_del(disk_to_dev(disk));
-	blk_free_devt(disk_to_dev(disk)->devt);
  }
  EXPORT_SYMBOL(del_gendisk);

@@ -1098,6 +1097,7 @@ static void disk_release(struct device *dev)
  {
  	struct gendisk *disk = dev_to_disk(dev);

+	blk_free_devt(dev->devt);
  	disk_release_events(disk);
  	kfree(disk->random);
  	disk_replace_part_tbl(disk, NULL);
--