* fix a race between del_gendisk and BLKRRPART @ 2021-05-12 16:50 Christoph Hellwig 2021-05-12 16:50 ` [PATCH 1/2] block: prevent block device lookups at the beginning of del_gendisk Christoph Hellwig 2021-05-12 16:50 ` [PATCH 2/2] block: fix a race between del_gendisk and BLKRRPART Christoph Hellwig 0 siblings, 2 replies; 4+ messages in thread From: Christoph Hellwig @ 2021-05-12 16:50 UTC (permalink / raw) To: axboe; +Cc: Gulam Mohamed, Ming Lei, linux-block 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. Diffstat: block/genhd.c | 19 ++----------------- fs/block_dev.c | 12 ++++-------- include/linux/genhd.h | 2 -- 3 files changed, 6 insertions(+), 27 deletions(-) ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/2] block: prevent block device lookups at the beginning of del_gendisk 2021-05-12 16:50 fix a race between del_gendisk and BLKRRPART Christoph Hellwig @ 2021-05-12 16:50 ` Christoph Hellwig 2021-05-13 0:11 ` Ming Lei 2021-05-12 16:50 ` [PATCH 2/2] block: fix a race between del_gendisk and BLKRRPART Christoph Hellwig 1 sibling, 1 reply; 4+ messages in thread From: Christoph Hellwig @ 2021-05-12 16:50 UTC (permalink / raw) To: axboe; +Cc: Gulam Mohamed, Ming Lei, linux-block 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] block: prevent block device lookups at the beginning of del_gendisk 2021-05-12 16:50 ` [PATCH 1/2] block: prevent block device lookups at the beginning of del_gendisk Christoph Hellwig @ 2021-05-13 0:11 ` Ming Lei 0 siblings, 0 replies; 4+ messages in thread From: Ming Lei @ 2021-05-13 0:11 UTC (permalink / raw) To: Christoph Hellwig; +Cc: axboe, Gulam Mohamed, linux-block 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] block: fix a race between del_gendisk and BLKRRPART 2021-05-12 16:50 fix a race between del_gendisk and BLKRRPART Christoph Hellwig 2021-05-12 16:50 ` [PATCH 1/2] block: prevent block device lookups at the beginning of del_gendisk Christoph Hellwig @ 2021-05-12 16:50 ` Christoph Hellwig 1 sibling, 0 replies; 4+ messages in thread From: Christoph Hellwig @ 2021-05-12 16:50 UTC (permalink / raw) To: axboe; +Cc: Gulam Mohamed, Ming Lei, linux-block 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 de1a760da62d..577f112c3818 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-05-13 0:14 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-12 16:50 fix a race between del_gendisk and BLKRRPART Christoph Hellwig 2021-05-12 16:50 ` [PATCH 1/2] block: prevent block device lookups at the beginning of del_gendisk Christoph Hellwig 2021-05-13 0:11 ` Ming Lei 2021-05-12 16:50 ` [PATCH 2/2] block: fix a race between del_gendisk and BLKRRPART Christoph Hellwig
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.