fix a race between del_gendisk and BLKRRPART v2

fix a race between del_gendisk and BLKRRPART v2

[Christoph Hellwig]

Hi all,

this is based of a patch from Gulam and suggestions from Ming and fixes a
race between del_gendisk and BLKRRPART while also removing a global lock.

Changes since v1:
 - fix the GENHD_FL_UP check in __blkdev_get
 - don't change where remove_inode_hash is called for now
 - improve the commit message

Diffstat:
 block/genhd.c         |   11 +----------
 fs/block_dev.c        |   18 ++++++++----------
 include/linux/genhd.h |    2 --
 3 files changed, 9 insertions(+), 22 deletions(-)

[PATCH 1/2] block: prevent block device lookups at the beginning of del_gendisk

[Christoph Hellwig]

As an artifact of how gendisk lookup used to work in earlier kernels,
GENHD_FL_UP is only cleared very late in del_gendisk, and a global lock
is used to prevent opens from succeeding while del_gendisk is tearing
down the gendisk.  Switch to clearing the flag early and under bd_mutex
so that callers can use bd_mutex to stabilize the flag, which removes
the need for the global mutex.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/genhd.c         | 11 +----------
 fs/block_dev.c        | 15 +++++----------
 include/linux/genhd.h |  2 --
 3 files changed, 6 insertions(+), 22 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 39ca97b0edc6..9f8cb7beaad1 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -29,8 +29,6 @@
 
 static struct kobject *block_depr;
 
-DECLARE_RWSEM(bdev_lookup_sem);
-
 /* for extended dynamic devt allocation, currently only one major is used */
 #define NR_EXT_DEVT		(1 << MINORBITS)
 static DEFINE_IDA(ext_devt_ida);
@@ -609,13 +607,8 @@ void del_gendisk(struct gendisk *disk)
 	blk_integrity_del(disk);
 	disk_del_events(disk);
 
-	/*
-	 * Block lookups of the disk until all bdevs are unhashed and the
-	 * disk is marked as dead (GENHD_FL_UP cleared).
-	 */
-	down_write(&bdev_lookup_sem);
-
 	mutex_lock(&disk->part0->bd_mutex);
+	disk->flags &= ~GENHD_FL_UP;
 	blk_drop_partitions(disk);
 	mutex_unlock(&disk->part0->bd_mutex);
 
@@ -629,8 +622,6 @@ void del_gendisk(struct gendisk *disk)
 	remove_inode_hash(disk->part0->bd_inode);
 
 	set_capacity(disk, 0);
-	disk->flags &= ~GENHD_FL_UP;
-	up_write(&bdev_lookup_sem);
 
 	if (!(disk->flags & GENHD_FL_HIDDEN)) {
 		sysfs_remove_link(&disk_to_dev(disk)->kobj, "bdi");
diff --git a/fs/block_dev.c b/fs/block_dev.c
index eb265d72fce8..580bae995b87 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1298,6 +1298,9 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode)
 	struct gendisk *disk = bdev->bd_disk;
 	int ret = 0;
 
+	if (!(disk->flags & GENHD_FL_UP))
+		return -ENXIO;
+
 	if (!bdev->bd_openers) {
 		if (!bdev_is_partition(bdev)) {
 			ret = 0;
@@ -1332,8 +1335,7 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode)
 			whole->bd_part_count++;
 			mutex_unlock(&whole->bd_mutex);
 
-			if (!(disk->flags & GENHD_FL_UP) ||
-			    !bdev_nr_sectors(bdev)) {
+			if (!bdev_nr_sectors(bdev)) {
 				__blkdev_put(whole, mode, 1);
 				bdput(whole);
 				return -ENXIO;
@@ -1364,16 +1366,12 @@ struct block_device *blkdev_get_no_open(dev_t dev)
 	struct block_device *bdev;
 	struct gendisk *disk;
 
-	down_read(&bdev_lookup_sem);
 	bdev = bdget(dev);
 	if (!bdev) {
-		up_read(&bdev_lookup_sem);
 		blk_request_module(dev);
-		down_read(&bdev_lookup_sem);
-
 		bdev = bdget(dev);
 		if (!bdev)
-			goto unlock;
+			return NULL;
 	}
 
 	disk = bdev->bd_disk;
@@ -1383,14 +1381,11 @@ struct block_device *blkdev_get_no_open(dev_t dev)
 		goto put_disk;
 	if (!try_module_get(bdev->bd_disk->fops->owner))
 		goto put_disk;
-	up_read(&bdev_lookup_sem);
 	return bdev;
 put_disk:
 	put_disk(disk);
 bdput:
 	bdput(bdev);
-unlock:
-	up_read(&bdev_lookup_sem);
 	return NULL;
 }
 
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 7e9660ea967d..6fc26f7bdf71 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -306,8 +306,6 @@ static inline void bd_unlink_disk_holder(struct block_device *bdev,
 }
 #endif /* CONFIG_SYSFS */
 
-extern struct rw_semaphore bdev_lookup_sem;
-
 dev_t blk_lookup_devt(const char *name, int partno);
 void blk_request_module(dev_t devt);
 #ifdef CONFIG_BLOCK
-- 
2.30.2

[PATCH 2/2] block: fix a race between del_gendisk and BLKRRPART

[Christoph Hellwig]

From: Gulam Mohamed <gulam.mohamed@oracle.com>

When BLKRRPART is called concurrently with del_gendisk, the partitions
rescan can create a stale partition that will never be be cleaned up.

Fix this by checking the the disk is up before rescanning partitions
while under bd_mutex.

Signed-off-by: Gulam Mohamed <gulam.mohamed@oracle.com>
[hch: split from a larger patch]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/block_dev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 580bae995b87..4494411fa4d3 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1244,6 +1244,9 @@ int bdev_disk_changed(struct block_device *bdev, bool invalidate)
 
 	lockdep_assert_held(&bdev->bd_mutex);
 
+	if (!(disk->flags & GENHD_FL_UP))
+		return -ENXIO;
+
 rescan:
 	if (bdev->bd_part_count)
 		return -EBUSY;
-- 
2.30.2

Re: [PATCH 1/2] block: prevent block device lookups at the beginning of del_gendisk

[Ming Lei]

On Fri, May 14, 2021 at 03:18:41PM +0200, Christoph Hellwig wrote:
> As an artifact of how gendisk lookup used to work in earlier kernels,
> GENHD_FL_UP is only cleared very late in del_gendisk, and a global lock
> is used to prevent opens from succeeding while del_gendisk is tearing
> down the gendisk.  Switch to clearing the flag early and under bd_mutex
> so that callers can use bd_mutex to stabilize the flag, which removes
> the need for the global mutex.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Ming Lei <ming.lei@redhat.com>

-- 
Ming

Re: [PATCH 2/2] block: fix a race between del_gendisk and BLKRRPART

[Ming Lei]

On Fri, May 14, 2021 at 03:18:42PM +0200, Christoph Hellwig wrote:
> From: Gulam Mohamed <gulam.mohamed@oracle.com>
> 
> When BLKRRPART is called concurrently with del_gendisk, the partitions
> rescan can create a stale partition that will never be be cleaned up.
> 
> Fix this by checking the the disk is up before rescanning partitions
> while under bd_mutex.
> 
> Signed-off-by: Gulam Mohamed <gulam.mohamed@oracle.com>
> [hch: split from a larger patch]
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/block_dev.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 580bae995b87..4494411fa4d3 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1244,6 +1244,9 @@ int bdev_disk_changed(struct block_device *bdev, bool invalidate)
>  
>  	lockdep_assert_held(&bdev->bd_mutex);
>  
> +	if (!(disk->flags & GENHD_FL_UP))
> +		return -ENXIO;
> +
>  rescan:
>  	if (bdev->bd_part_count)
>  		return -EBUSY;

Reviewed-by: Ming Lei <ming.lei@redhat.com>

-- 
Ming

Re: fix a race between del_gendisk and BLKRRPART v2

[Christoph Hellwig]

Jens,

can you take a look at these?

On Fri, May 14, 2021 at 03:18:40PM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> this is based of a patch from Gulam and suggestions from Ming and fixes a
> race between del_gendisk and BLKRRPART while also removing a global lock.
> 
> Changes since v1:
>  - fix the GENHD_FL_UP check in __blkdev_get
>  - don't change where remove_inode_hash is called for now
>  - improve the commit message
> 
> Diffstat:
>  block/genhd.c         |   11 +----------
>  fs/block_dev.c        |   18 ++++++++----------
>  include/linux/genhd.h |    2 --
>  3 files changed, 9 insertions(+), 22 deletions(-)
---end quoted text---

Re: fix a race between del_gendisk and BLKRRPART v2

[Jens Axboe]

On 5/20/21 2:14 AM, Christoph Hellwig wrote:
> Jens,
> 
> can you take a look at these?

Looks good to me, applied.

-- 
Jens Axboe

RE: [PATCH 1/2] block: prevent block device lookups at the beginning of del_gendisk

[Gulam Mohamed]

-----Original Message-----
From: Ming Lei <ming.lei@redhat.com> 
Sent: Saturday, May 15, 2021 12:05 PM
To: Christoph Hellwig <hch@lst.de>
Cc: axboe@kernel.dk; Gulam Mohamed <gulam.mohamed@oracle.com>; linux-block@vger.kernel.org
Subject: Re: [PATCH 1/2] block: prevent block device lookups at the beginning of del_gendisk

On Fri, May 14, 2021 at 03:18:41PM +0200, Christoph Hellwig wrote:
> As an artifact of how gendisk lookup used to work in earlier kernels, 
> GENHD_FL_UP is only cleared very late in del_gendisk, and a global 
> lock is used to prevent opens from succeeding while del_gendisk is 
> tearing down the gendisk.  Switch to clearing the flag early and under 
> bd_mutex so that callers can use bd_mutex to stabilize the flag, which 
> removes the need for the global mutex.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Ming Lei <ming.lei@redhat.com>

I have tested these patches on latest kernel in our environment and didn't find any issue. The fix is working fine.

Tested-by: Gulam Mohamed <gulam.mohamed@oracle.com>

--
Ming

RE: [PATCH 2/2] block: fix a race between del_gendisk and BLKRRPART

[Gulam Mohamed]

-----Original Message-----
From: Ming Lei <ming.lei@redhat.com> 
Sent: Saturday, May 15, 2021 12:05 PM
To: Christoph Hellwig <hch@lst.de>
Cc: axboe@kernel.dk; Gulam Mohamed <gulam.mohamed@oracle.com>; linux-block@vger.kernel.org
Subject: Re: [PATCH 2/2] block: fix a race between del_gendisk and BLKRRPART

On Fri, May 14, 2021 at 03:18:42PM +0200, Christoph Hellwig wrote:
> From: Gulam Mohamed <gulam.mohamed@oracle.com>
> 
> When BLKRRPART is called concurrently with del_gendisk, the partitions 
> rescan can create a stale partition that will never be be cleaned up.
> 
> Fix this by checking the the disk is up before rescanning partitions 
> while under bd_mutex.
> 
> Signed-off-by: Gulam Mohamed <gulam.mohamed@oracle.com>
> [hch: split from a larger patch]
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/block_dev.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/block_dev.c b/fs/block_dev.c index 
> 580bae995b87..4494411fa4d3 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1244,6 +1244,9 @@ int bdev_disk_changed(struct block_device *bdev, 
> bool invalidate)
>  
>  	lockdep_assert_held(&bdev->bd_mutex);
>  
> +	if (!(disk->flags & GENHD_FL_UP))
> +		return -ENXIO;
> +
>  rescan:
>  	if (bdev->bd_part_count)
>  		return -EBUSY;

Reviewed-by: Ming Lei <ming.lei@redhat.com>
Tested-by: Gulam Mohamed <gulam.mohamed@oracle.com>

--
Ming

Re: [PATCH 1/2] block: prevent block device lookups at the beginning of del_gendisk

[Ming Lei]

On Wed, May 12, 2021 at 06:50:49PM +0200, Christoph Hellwig wrote:
> As an aritfact of how gendisk lookup used to work in earlier kernels,
> GENHD_FL_UP is only cleared very late in del_gendisk, and instead a
> global lock is used to prevent opens from actually succeeding.  Clear
> the flag, and remove the bdev inode from the inode hash earlier to
> ensure lookups fail from the very beginning of del_gendisk, and remove
> the now not needed bdev_lookup_sem.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/genhd.c         | 19 ++-----------------
>  fs/block_dev.c        |  9 +--------
>  include/linux/genhd.h |  2 --
>  3 files changed, 3 insertions(+), 27 deletions(-)
> 
> diff --git a/block/genhd.c b/block/genhd.c
> index 39ca97b0edc6..1693e520ec7d 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -29,8 +29,6 @@
>  
>  static struct kobject *block_depr;
>  
> -DECLARE_RWSEM(bdev_lookup_sem);
> -
>  /* for extended dynamic devt allocation, currently only one major is used */
>  #define NR_EXT_DEVT		(1 << MINORBITS)
>  static DEFINE_IDA(ext_devt_ida);
> @@ -609,28 +607,15 @@ void del_gendisk(struct gendisk *disk)
>  	blk_integrity_del(disk);
>  	disk_del_events(disk);
>  
> -	/*
> -	 * Block lookups of the disk until all bdevs are unhashed and the
> -	 * disk is marked as dead (GENHD_FL_UP cleared).
> -	 */
> -	down_write(&bdev_lookup_sem);
> -
>  	mutex_lock(&disk->part0->bd_mutex);
> +	disk->flags &= ~GENHD_FL_UP;
> +	remove_inode_hash(disk->part0->bd_inode);
>  	blk_drop_partitions(disk);
>  	mutex_unlock(&disk->part0->bd_mutex);

Both bdget() and checking FL_UP is done without holding ->bd_mutex, so
del_gendisk() may be run after checking FL_UP is completed in blkdev_get_no_open(),
then new opener is still allowed even after the bdev is invalidated.

Given this patch is for 5.13, I'd suggest to not move remove_inode_hash()
inside the lock block, which isn't necessary, and it is enough to move
clearing FL_UP with the following change together:

diff --git a/fs/block_dev.c b/fs/block_dev.c
index b8abccd03e5d..31a6d54edf6d 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1298,6 +1298,9 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode)
 	struct gendisk *disk = bdev->bd_disk;
 	int ret = 0;
 
+	if (!(disk->flags & GENHD_FL_UP))
+		return -ENXIO;
+
 	if (!bdev->bd_openers) {
 		if (!bdev_is_partition(bdev)) {
 			ret = 0;
@@ -1332,8 +1335,7 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode)
 			whole->bd_part_count++;
 			mutex_unlock(&whole->bd_mutex);
 
-			if (!(disk->flags & GENHD_FL_UP) ||
-			    !bdev_nr_sectors(bdev)) {
+			if (!bdev_nr_sectors(bdev)) {
 				__blkdev_put(whole, mode, 1);
 				bdput(whole);
 				return -ENXIO;


Thanks,
Ming

[PATCH 1/2] block: prevent block device lookups at the beginning of del_gendisk

[Christoph Hellwig]

As an aritfact of how gendisk lookup used to work in earlier kernels,
GENHD_FL_UP is only cleared very late in del_gendisk, and instead a
global lock is used to prevent opens from actually succeeding.  Clear
the flag, and remove the bdev inode from the inode hash earlier to
ensure lookups fail from the very beginning of del_gendisk, and remove
the now not needed bdev_lookup_sem.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/genhd.c         | 19 ++-----------------
 fs/block_dev.c        |  9 +--------
 include/linux/genhd.h |  2 --
 3 files changed, 3 insertions(+), 27 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 39ca97b0edc6..1693e520ec7d 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -29,8 +29,6 @@
 
 static struct kobject *block_depr;
 
-DECLARE_RWSEM(bdev_lookup_sem);
-
 /* for extended dynamic devt allocation, currently only one major is used */
 #define NR_EXT_DEVT		(1 << MINORBITS)
 static DEFINE_IDA(ext_devt_ida);
@@ -609,28 +607,15 @@ void del_gendisk(struct gendisk *disk)
 	blk_integrity_del(disk);
 	disk_del_events(disk);
 
-	/*
-	 * Block lookups of the disk until all bdevs are unhashed and the
-	 * disk is marked as dead (GENHD_FL_UP cleared).
-	 */
-	down_write(&bdev_lookup_sem);
-
 	mutex_lock(&disk->part0->bd_mutex);
+	disk->flags &= ~GENHD_FL_UP;
+	remove_inode_hash(disk->part0->bd_inode);
 	blk_drop_partitions(disk);
 	mutex_unlock(&disk->part0->bd_mutex);
 
 	fsync_bdev(disk->part0);
 	__invalidate_device(disk->part0, true);
-
-	/*
-	 * Unhash the bdev inode for this device so that it can't be looked
-	 * up any more even if openers still hold references to it.
-	 */
-	remove_inode_hash(disk->part0->bd_inode);
-
 	set_capacity(disk, 0);
-	disk->flags &= ~GENHD_FL_UP;
-	up_write(&bdev_lookup_sem);
 
 	if (!(disk->flags & GENHD_FL_HIDDEN)) {
 		sysfs_remove_link(&disk_to_dev(disk)->kobj, "bdi");
diff --git a/fs/block_dev.c b/fs/block_dev.c
index eb265d72fce8..de1a760da62d 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1364,16 +1364,12 @@ struct block_device *blkdev_get_no_open(dev_t dev)
 	struct block_device *bdev;
 	struct gendisk *disk;
 
-	down_read(&bdev_lookup_sem);
 	bdev = bdget(dev);
 	if (!bdev) {
-		up_read(&bdev_lookup_sem);
 		blk_request_module(dev);
-		down_read(&bdev_lookup_sem);
-
 		bdev = bdget(dev);
 		if (!bdev)
-			goto unlock;
+			return NULL;
 	}
 
 	disk = bdev->bd_disk;
@@ -1383,14 +1379,11 @@ struct block_device *blkdev_get_no_open(dev_t dev)
 		goto put_disk;
 	if (!try_module_get(bdev->bd_disk->fops->owner))
 		goto put_disk;
-	up_read(&bdev_lookup_sem);
 	return bdev;
 put_disk:
 	put_disk(disk);
 bdput:
 	bdput(bdev);
-unlock:
-	up_read(&bdev_lookup_sem);
 	return NULL;
 }
 
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 7e9660ea967d..6fc26f7bdf71 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -306,8 +306,6 @@ static inline void bd_unlink_disk_holder(struct block_device *bdev,
 }
 #endif /* CONFIG_SYSFS */
 
-extern struct rw_semaphore bdev_lookup_sem;
-
 dev_t blk_lookup_devt(const char *name, int partno);
 void blk_request_module(dev_t devt);
 #ifdef CONFIG_BLOCK
-- 
2.30.2