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

[PATCH V2 1/1] 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
disk->fops->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() needn't to touch
disk->fops or defined callbacks.

Cc: Christoph Hellwig <hch@lst.de>
Cc: czhong@redhat.com
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
V2:
	- remove comment as suggested by Christoph
	- improve commit log a bit

 block/bdev.c | 12 +++++++-----
 1 file changed, 7 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);
-- 
2.31.1

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

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Ming Lei]

On Thu, Nov 11, 2021 at 10:03:43AM +0800, Ming Lei wrote:
> 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
> disk->fops->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() needn't to touch
> disk->fops or defined callbacks.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: czhong@redhat.com
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
> V2:
> 	- remove comment as suggested by Christoph
> 	- improve commit log a bit

Hi Jens,

Ping...

Thanks,
Ming

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

[Jens Axboe]

On Thu, 11 Nov 2021 10:03:43 +0800, Ming Lei wrote:
> 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
> disk->fops->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.
> 
> [...]

Applied, thanks!

[1/1] block: avoid to touch unloaded module instance when opening bdev
      (no commit info)

Best regards,
-- 
Jens Axboe