linux-kernel - Re: [PATCH V1 1/1] Fix race between iscsi logout and systemd-udevd

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 12 May 2021 08:35:05 +0200
From:   Christoph Hellwig <hch@....de>
To:     Ming Lei <ming.lei@...hat.com>
Cc:     Gulam Mohamed <gulam.mohamed@...cle.com>, viro@...iv.linux.org.uk,
        axboe@...nel.dk, linux-fsdevel@...r.kernel.org,
        linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
        hch@....de, martin.petersen@...cle.com, junxiao.bi@...cle.com
Subject: Re: [PATCH V1 1/1] Fix race between iscsi logout and systemd-udevd

On Wed, May 12, 2021 at 11:23:59AM +0800, Ming Lei wrote:
> 
> 1) code path BLKRRPART:
> 	mutex_lock(bdev->bd_mutex)
> 	down_read(&bdev_lookup_sem);
> 
> 2) del_gendisk():
> 	down_write(&bdev_lookup_sem);
> 	mutex_lock(&disk->part0->bd_mutex);
> 
> Given GENHD_FL_UP is only checked when opening one bdev, and
> fsync_bdev() and __invalidate_device() needn't to open bdev, so
> the following way may work for your issue:

If we move the clearing of GENHD_FL_UP earlier we can do away with
bdev_lookup_sem entirely I think.  Something like this untested patch:

diff --git a/block/genhd.c b/block/genhd.c
index a5847560719c..ef717084b343 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->open_mutex);
+	disk->flags &= ~GENHD_FL_UP;
 	blk_drop_partitions(disk);
 	mutex_unlock(&disk->open_mutex);
 
@@ -627,10 +620,7 @@ void del_gendisk(struct gendisk *disk)
 	 * 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 8dd8e2fd1401..bde23940190f 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1377,33 +1377,24 @@ 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;
 	if (!kobject_get_unless_zero(&disk_to_dev(disk)->kobj))
 		goto bdput;
-	if ((disk->flags & (GENHD_FL_UP | GENHD_FL_HIDDEN)) != GENHD_FL_UP)
-		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;
 }
 
@@ -1462,7 +1453,10 @@ struct block_device *blkdev_get_by_dev(dev_t dev, fmode_t mode, void *holder)
 
 	disk_block_events(disk);
 
+	ret = -ENXIO;
 	mutex_lock(&disk->open_mutex);
+	if ((disk->flags & (GENHD_FL_UP | GENHD_FL_HIDDEN)) != GENHD_FL_UP)
+		goto abort_claiming;
 	if (bdev_is_partition(bdev))
 		ret = blkdev_get_part(bdev, mode);
 	else