diff mbox series

[-next,2/2] block: fix scan partition for exclusively open device again

Message ID 20230217022200.3092987-3-yukuai1@huaweicloud.com
State New
Headers
Series block: fix scan partition for exclusively open device again |

Commit Message

Yu Kuai Feb. 17, 2023, 2:22 a.m. UTC 
  From: Yu Kuai <yukuai3@huawei.com>

As explained in commit 36369f46e917 ("block: Do not reread partition table
on exclusively open device"), reread partition on the device that is
exclusively opened by someone else is problematic.

This patch will make sure partition scan will only be proceed if current
thread open the device exclusively, or the device is not opened
exclusively, and in the later case, other scanners and exclusive openers
will be blocked temporarily until partition scan is done.

Fixes: 10c70d95c0f2 ("block: remove the bd_openers checks in blk_drop_partitions")
Cc: <stable@vger.kernel.org>
Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/genhd.c | 30 ++++++++++++++++++++++++++----
 block/ioctl.c |  2 +-
 2 files changed, 27 insertions(+), 5 deletions(-)

Comments

Christoph Hellwig Feb. 17, 2023, 7:29 a.m. UTC | #1 
Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Jan Kara Feb. 17, 2023, 11:05 a.m. UTC | #2 
On Fri 17-02-23 10:22:00, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> As explained in commit 36369f46e917 ("block: Do not reread partition table
> on exclusively open device"), reread partition on the device that is
> exclusively opened by someone else is problematic.
> 
> This patch will make sure partition scan will only be proceed if current
> thread open the device exclusively, or the device is not opened
> exclusively, and in the later case, other scanners and exclusive openers
> will be blocked temporarily until partition scan is done.
> 
> Fixes: 10c70d95c0f2 ("block: remove the bd_openers checks in blk_drop_partitions")
> Cc: <stable@vger.kernel.org>
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>

Looks good to me, just two minor comments below:

> diff --git a/block/genhd.c b/block/genhd.c
> index b30d5538710c..3ee5577e1586 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -359,6 +359,7 @@ EXPORT_SYMBOL_GPL(disk_uevent);
>  int disk_scan_partitions(struct gendisk *disk, fmode_t mode)
>  {
>  	struct block_device *bdev;
> +	int ret = 0;
>  
>  	if (disk->flags & (GENHD_FL_NO_PART | GENHD_FL_HIDDEN))
>  		return -EINVAL;
> @@ -368,11 +369,27 @@ int disk_scan_partitions(struct gendisk *disk, fmode_t mode)
>  		return -EBUSY;
>  
>  	set_bit(GD_NEED_PART_SCAN, &disk->state);

I'd move the set_bit() after we are sure we have exclusive access to the
bdev. Otherwise we could set GD_NEED_PART_SCAN on a device exclusively open
by someone else and if we race with open in an unfortunate way, we could
trigger unexpected partition scan...

> -	bdev = blkdev_get_by_dev(disk_devt(disk), mode, NULL);
> +	/*
> +	 * If the device is opened exclusively by current thread already, it's
> +	 * safe to scan partitons, otherwise, use bd_prepare_to_claim() to
> +	 * synchronize with other exclusive openers and other partition
> +	 * scanners.
> +	 */
> +	if (!(mode & FMODE_EXCL)) {
> +		ret = bd_prepare_to_claim(disk->part0, disk_scan_partitions);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	bdev = blkdev_get_by_dev(disk_devt(disk), mode & ~FMODE_EXCL, NULL);
>  	if (IS_ERR(bdev))
> -		return PTR_ERR(bdev);
> -	blkdev_put(bdev, mode);
> -	return 0;
> +		ret =  PTR_ERR(bdev);
> +	else
> +		blkdev_put(bdev, mode);
> +
> +	if (!(mode & FMODE_EXCL))
> +		bd_abort_claiming(disk->part0, disk_scan_partitions);
> +	return ret;
>  }
>  
>  /**
> @@ -494,6 +511,11 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk,
>  		if (ret)
>  			goto out_unregister_bdi;
>  
> +		/* Make sure the first partition scan will be proceed */
							   ^^^^^^ "will happen"
probably makes more sense here.

								Honza
diff mbox series

Patch

diff --git a/block/genhd.c b/block/genhd.c
index b30d5538710c..3ee5577e1586 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -359,6 +359,7 @@  EXPORT_SYMBOL_GPL(disk_uevent);
 int disk_scan_partitions(struct gendisk *disk, fmode_t mode)
 {
 	struct block_device *bdev;
+	int ret = 0;
 
 	if (disk->flags & (GENHD_FL_NO_PART | GENHD_FL_HIDDEN))
 		return -EINVAL;
@@ -368,11 +369,27 @@  int disk_scan_partitions(struct gendisk *disk, fmode_t mode)
 		return -EBUSY;
 
 	set_bit(GD_NEED_PART_SCAN, &disk->state);
-	bdev = blkdev_get_by_dev(disk_devt(disk), mode, NULL);
+	/*
+	 * If the device is opened exclusively by current thread already, it's
+	 * safe to scan partitons, otherwise, use bd_prepare_to_claim() to
+	 * synchronize with other exclusive openers and other partition
+	 * scanners.
+	 */
+	if (!(mode & FMODE_EXCL)) {
+		ret = bd_prepare_to_claim(disk->part0, disk_scan_partitions);
+		if (ret)
+			return ret;
+	}
+
+	bdev = blkdev_get_by_dev(disk_devt(disk), mode & ~FMODE_EXCL, NULL);
 	if (IS_ERR(bdev))
-		return PTR_ERR(bdev);
-	blkdev_put(bdev, mode);
-	return 0;
+		ret =  PTR_ERR(bdev);
+	else
+		blkdev_put(bdev, mode);
+
+	if (!(mode & FMODE_EXCL))
+		bd_abort_claiming(disk->part0, disk_scan_partitions);
+	return ret;
 }
 
 /**
@@ -494,6 +511,11 @@  int __must_check device_add_disk(struct device *parent, struct gendisk *disk,
 		if (ret)
 			goto out_unregister_bdi;
 
+		/* Make sure the first partition scan will be proceed */
+		if (get_capacity(disk) && !(disk->flags & GENHD_FL_NO_PART) &&
+		    !test_bit(GD_SUPPRESS_PART_SCAN, &disk->state))
+			set_bit(GD_NEED_PART_SCAN, &disk->state);
+
 		bdev_add(disk->part0, ddev->devt);
 		if (get_capacity(disk))
 			disk_scan_partitions(disk, FMODE_READ);
diff --git a/block/ioctl.c b/block/ioctl.c
index 6dd49d877584..9c5f637ff153 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -528,7 +528,7 @@  static int blkdev_common_ioctl(struct block_device *bdev, fmode_t mode,
 			return -EACCES;
 		if (bdev_is_partition(bdev))
 			return -EINVAL;
-		return disk_scan_partitions(bdev->bd_disk, mode & ~FMODE_EXCL);
+		return disk_scan_partitions(bdev->bd_disk, mode);
 	case BLKTRACESTART:
 	case BLKTRACESTOP:
 	case BLKTRACETEARDOWN: