[PATCH] block: avoid to touch unloaded module instance when opening bdev

[PATCH] block: avoid to touch unloaded module instance when opening bdev

[Ming Lei]

disk->fops->owner is grabbed in blkdev_get_no_open() after the disk
kobject refcount is increased. This way can't make sure that
disk->fops->owner is still alive since del_gendisk() still can move
on if the kobject refcount of disk is grabbed by open() and ->open()
isn't called yet.

Fixes the issue by moving try_module_get() into blkdev_get_by_dev()
with ->open_mutex() held, then we can drain the in-progress open()
in del_gendisk(). Meantime new open() won't succeed because disk
becomes not alive.

This way is reasonable because blkdev_get_no_open() doesn't need
to grab disk->fops->owner which is required only if callback in
disk->fops is needed.

Cc: Christoph Hellwig <hch@lst.de>
Cc: stable@vger.kernel.org
Cc: czhong@redhat.com
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/bdev.c  | 12 +++++++-----
 block/genhd.c |  6 ++++++
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/block/bdev.c b/block/bdev.c
index b4dab2fb6a74..b1d087e5e205 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -753,8 +753,7 @@ struct block_device *blkdev_get_no_open(dev_t dev)
 
 	if (!bdev)
 		return NULL;
-	if ((bdev->bd_disk->flags & GENHD_FL_HIDDEN) ||
-	    !try_module_get(bdev->bd_disk->fops->owner)) {
+	if ((bdev->bd_disk->flags & GENHD_FL_HIDDEN)) {
 		put_device(&bdev->bd_device);
 		return NULL;
 	}
@@ -764,7 +763,6 @@ struct block_device *blkdev_get_no_open(dev_t dev)
 
 void blkdev_put_no_open(struct block_device *bdev)
 {
-	module_put(bdev->bd_disk->fops->owner);
 	put_device(&bdev->bd_device);
 }
 
@@ -820,12 +818,14 @@ struct block_device *blkdev_get_by_dev(dev_t dev, fmode_t mode, void *holder)
 	ret = -ENXIO;
 	if (!disk_live(disk))
 		goto abort_claiming;
+	if (!try_module_get(disk->fops->owner))
+		goto abort_claiming;
 	if (bdev_is_partition(bdev))
 		ret = blkdev_get_part(bdev, mode);
 	else
 		ret = blkdev_get_whole(bdev, mode);
 	if (ret)
-		goto abort_claiming;
+		goto put_module;
 	if (mode & FMODE_EXCL) {
 		bd_finish_claiming(bdev, holder);
 
@@ -847,7 +847,8 @@ struct block_device *blkdev_get_by_dev(dev_t dev, fmode_t mode, void *holder)
 	if (unblock_events)
 		disk_unblock_events(disk);
 	return bdev;
-
+put_module:
+	module_put(disk->fops->owner);
 abort_claiming:
 	if (mode & FMODE_EXCL)
 		bd_abort_claiming(bdev, holder);
@@ -956,6 +957,7 @@ void blkdev_put(struct block_device *bdev, fmode_t mode)
 		blkdev_put_whole(bdev, mode);
 	mutex_unlock(&disk->open_mutex);
 
+	module_put(disk->fops->owner);
 	blkdev_put_no_open(bdev);
 }
 EXPORT_SYMBOL(blkdev_put);
diff --git a/block/genhd.c b/block/genhd.c
index a4e9e8ebd941..5f427fdc9e23 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -576,6 +576,12 @@ void del_gendisk(struct gendisk *disk)
 	blk_integrity_del(disk);
 	disk_del_events(disk);
 
+	/*
+	 * New open() will be failed since disk becomes not alive, and old
+	 * open() has either grabbed the module refcnt or been failed in
+	 * case of deleting from module_exit(), so disk->fops->owner won't
+	 * be unloaded if the disk is opened.
+	 */
 	mutex_lock(&disk->open_mutex);
 	remove_inode_hash(disk->part0->bd_inode);
 	blk_drop_partitions(disk);
-- 
2.31.1

Re: [PATCH] block: avoid to touch unloaded module instance when opening bdev

[Christoph Hellwig]

The technical change looks good, but this comment not only has quite
a few of grammar issues, but also seems rather pointless:

> +	/*
> +	 * New open() will be failed since disk becomes not alive, and old
> +	 * open() has either grabbed the module refcnt or been failed in
> +	 * case of deleting from module_exit(), so disk->fops->owner won't
> +	 * be unloaded if the disk is opened.
> +	 */
>  	mutex_lock(&disk->open_mutex);
>  	remove_inode_hash(disk->part0->bd_inode);
>  	blk_drop_partitions(disk);
> -- 
> 2.31.1
---end quoted