linux-kernel - Re: [PATCH -next 2/2] block: fix scan partition for exclusively open device again

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:   Fri, 17 Feb 2023 12:05:17 +0100
From:   Jan Kara <jack@...e.cz>
To:     Yu Kuai <yukuai1@...weicloud.com>
Cc:     jack@...e.cz, hare@...e.de, hch@...radead.org, axboe@...nel.dk,
        linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
        yukuai3@...wei.com, yi.zhang@...wei.com, yangerkun@...wei.com
Subject: Re: [PATCH -next 2/2] block: fix scan partition for exclusively open
 device again

On Fri 17-02-23 10:22:00, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@...wei.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@...r.kernel.org>
> Suggested-by: Jan Kara <jack@...e.cz>
> Signed-off-by: Yu Kuai <yukuai3@...wei.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
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR