avoid out of bounds zone bitmap access

avoid out of bounds zone bitmap access

[Christoph Hellwig]

Hi Jens,

this series (against your for-linus branch) fixes a problem where updates
to the zone information in the queue is not atomic, leading to a possible
out of bounds access.  Please take a look and let me know if you think
if this is ok for 5.5.

[PATCH 1/8] null_blk: fix zone size paramter check

[Christoph Hellwig]

From: Damien Le Moal <damien.lemoal@wdc.com>

For zoned=1 mode, the zone size must be a power of 2. Check this not
only when the zone size is specified during modprobe, but also when
creating a zoned null_blk device using configfs.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/null_blk_main.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index 795fda576824..53ba9c7f2786 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -1607,7 +1607,7 @@ static int null_init_tag_set(struct nullb *nullb, struct blk_mq_tag_set *set)
 	return blk_mq_alloc_tag_set(set);
 }
 
-static void null_validate_conf(struct nullb_device *dev)
+static int null_validate_conf(struct nullb_device *dev)
 {
 	dev->blocksize = round_down(dev->blocksize, 512);
 	dev->blocksize = clamp_t(unsigned int, dev->blocksize, 512, 4096);
@@ -1634,6 +1634,14 @@ static void null_validate_conf(struct nullb_device *dev)
 	/* can not stop a queue */
 	if (dev->queue_mode == NULL_Q_BIO)
 		dev->mbps = 0;
+
+	if (dev->zoned &&
+	    (!dev->zone_size || !is_power_of_2(dev->zone_size))) {
+		pr_err("zone_size must be power-of-two\n");
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
 #ifdef CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION
@@ -1666,7 +1674,9 @@ static int null_add_dev(struct nullb_device *dev)
 	struct nullb *nullb;
 	int rv;
 
-	null_validate_conf(dev);
+	rv = null_validate_conf(dev);
+	if (rv)
+		return rv;
 
 	nullb = kzalloc_node(sizeof(*nullb), GFP_KERNEL, dev->home_node);
 	if (!nullb) {
@@ -1792,11 +1802,6 @@ static int __init null_init(void)
 		g_bs = PAGE_SIZE;
 	}
 
-	if (!is_power_of_2(g_zone_size)) {
-		pr_err("zone_size must be power-of-two\n");
-		return -EINVAL;
-	}
-
 	if (g_home_node != NUMA_NO_NODE && g_home_node >= nr_online_nodes) {
 		pr_err("invalid home_node value\n");
 		g_home_node = NUMA_NO_NODE;
-- 
2.20.1

[PATCH 2/8] null_blk: cleanup null_gendisk_register

[Christoph Hellwig]

Use a saner size calculation, and do a trivial cleanup on the zone
revalidation to prepare to future changes.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/null_blk_main.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index 53ba9c7f2786..dd6026289fbf 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -1559,14 +1559,14 @@ static int init_driver_queues(struct nullb *nullb)
 
 static int null_gendisk_register(struct nullb *nullb)
 {
+	sector_t size = ((sector_t)nullb->dev->size * SZ_1M) >> SECTOR_SHIFT;
 	struct gendisk *disk;
-	sector_t size;
+	int ret;
 
 	disk = nullb->disk = alloc_disk_node(1, nullb->dev->home_node);
 	if (!disk)
 		return -ENOMEM;
-	size = (sector_t)nullb->dev->size * 1024 * 1024ULL;
-	set_capacity(disk, size >> 9);
+	set_capacity(disk, size);
 
 	disk->flags |= GENHD_FL_EXT_DEVT | GENHD_FL_SUPPRESS_PARTITION_INFO;
 	disk->major		= null_major;
@@ -1577,9 +1577,8 @@ static int null_gendisk_register(struct nullb *nullb)
 	strncpy(disk->disk_name, nullb->disk_name, DISK_NAME_LEN);
 
 	if (nullb->dev->zoned) {
-		int ret = blk_revalidate_disk_zones(disk);
-
-		if (ret != 0)
+		ret = blk_revalidate_disk_zones(disk);
+		if (ret)
 			return ret;
 	}
 
-- 
2.20.1

[PATCH 3/8] block: remove the empty line at the end of blk-zoned.c

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-zoned.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 6fad6f3f6980..618786f8275c 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -488,4 +488,3 @@ int blk_revalidate_disk_zones(struct gendisk *disk)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(blk_revalidate_disk_zones);
-
-- 
2.20.1

[PATCH 4/8] block: simplify blkdev_nr_zones

[Christoph Hellwig]

Simplify the arguments to blkdev_nr_zones by passing a gendisk instead
of the block_device and capacity.  This also removes the need for
__blkdev_nr_zones as all callers are outside the fast path and can
deal with the additional branch.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-zoned.c            | 26 ++++++++------------------
 block/ioctl.c                |  2 +-
 drivers/md/dm-zoned-target.c |  2 +-
 include/linux/blkdev.h       |  5 ++---
 4 files changed, 12 insertions(+), 23 deletions(-)

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 618786f8275c..65a9bdc9fe27 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -70,30 +70,20 @@ void __blk_req_zone_write_unlock(struct request *rq)
 }
 EXPORT_SYMBOL_GPL(__blk_req_zone_write_unlock);
 
-static inline unsigned int __blkdev_nr_zones(struct request_queue *q,
-					     sector_t nr_sectors)
-{
-	sector_t zone_sectors = blk_queue_zone_sectors(q);
-
-	return (nr_sectors + zone_sectors - 1) >> ilog2(zone_sectors);
-}
-
 /**
  * blkdev_nr_zones - Get number of zones
- * @bdev:	Target block device
+ * @disk:	Target gendisk
  *
- * Description:
- *    Return the total number of zones of a zoned block device.
- *    For a regular block device, the number of zones is always 0.
+ * Return the total number of zones of a zoned block device.  For a block
+ * device without zone capabilities, the number of zones is always 0.
  */
-unsigned int blkdev_nr_zones(struct block_device *bdev)
+unsigned int blkdev_nr_zones(struct gendisk *disk)
 {
-	struct request_queue *q = bdev_get_queue(bdev);
+	sector_t zone_sectors = blk_queue_zone_sectors(disk->queue);
 
-	if (!blk_queue_is_zoned(q))
+	if (!blk_queue_is_zoned(disk->queue))
 		return 0;
-
-	return __blkdev_nr_zones(q, get_capacity(bdev->bd_disk));
+	return (get_capacity(disk) + zone_sectors - 1) >> ilog2(zone_sectors);
 }
 EXPORT_SYMBOL_GPL(blkdev_nr_zones);
 
@@ -447,7 +437,7 @@ static int blk_update_zone_info(struct gendisk *disk, unsigned int nr_zones,
 int blk_revalidate_disk_zones(struct gendisk *disk)
 {
 	struct request_queue *q = disk->queue;
-	unsigned int nr_zones = __blkdev_nr_zones(q, get_capacity(disk));
+	unsigned int nr_zones = blkdev_nr_zones(disk);
 	struct blk_revalidate_zone_args args = { .disk = disk };
 	int ret = 0;
 
diff --git a/block/ioctl.c b/block/ioctl.c
index 7ac8a66c9787..5de98b97af2a 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -512,7 +512,7 @@ int blkdev_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 	case BLKGETZONESZ:
 		return put_uint(arg, bdev_zone_sectors(bdev));
 	case BLKGETNRZONES:
-		return put_uint(arg, blkdev_nr_zones(bdev));
+		return put_uint(arg, blkdev_nr_zones(bdev->bd_disk));
 	case HDIO_GETGEO:
 		return blkdev_getgeo(bdev, argp);
 	case BLKRAGET:
diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
index 4574e0dedbd6..70a1063161c0 100644
--- a/drivers/md/dm-zoned-target.c
+++ b/drivers/md/dm-zoned-target.c
@@ -727,7 +727,7 @@ static int dmz_get_zoned_device(struct dm_target *ti, char *path)
 	dev->zone_nr_blocks = dmz_sect2blk(dev->zone_nr_sectors);
 	dev->zone_nr_blocks_shift = ilog2(dev->zone_nr_blocks);
 
-	dev->nr_zones = blkdev_nr_zones(dev->bdev);
+	dev->nr_zones = blkdev_nr_zones(dev->bdev->bd_disk);
 
 	dmz->dev = dev;
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 6012e2592628..c5852de402b6 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -357,8 +357,7 @@ typedef int (*report_zones_cb)(struct blk_zone *zone, unsigned int idx,
 #define BLK_ALL_ZONES  ((unsigned int)-1)
 int blkdev_report_zones(struct block_device *bdev, sector_t sector,
 			unsigned int nr_zones, report_zones_cb cb, void *data);
-
-extern unsigned int blkdev_nr_zones(struct block_device *bdev);
+unsigned int blkdev_nr_zones(struct gendisk *disk);
 extern int blkdev_zone_mgmt(struct block_device *bdev, enum req_opf op,
 			    sector_t sectors, sector_t nr_sectors,
 			    gfp_t gfp_mask);
@@ -371,7 +370,7 @@ extern int blkdev_zone_mgmt_ioctl(struct block_device *bdev, fmode_t mode,
 
 #else /* CONFIG_BLK_DEV_ZONED */
 
-static inline unsigned int blkdev_nr_zones(struct block_device *bdev)
+static inline unsigned int blkdev_nr_zones(struct gendisk *disk)
 {
 	return 0;
 }
-- 
2.20.1

[PATCH 5/8] block: replace seq_zones_bitmap with conv_zones_bitmap

[Christoph Hellwig]

Invert the meaning of seq_zones_bitmap by keeping a bitmap of
conventional zones.  This allows not having a bitmap for devices
that do not have conventional zones.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-zoned.c      | 18 +++++++++---------
 include/linux/blkdev.h | 14 ++++++++------
 2 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 65a9bdc9fe27..9c3931051f4f 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -332,15 +332,15 @@ static inline unsigned long *blk_alloc_zone_bitmap(int node,
 
 void blk_queue_free_zone_bitmaps(struct request_queue *q)
 {
-	kfree(q->seq_zones_bitmap);
-	q->seq_zones_bitmap = NULL;
+	kfree(q->conv_zones_bitmap);
+	q->conv_zones_bitmap = NULL;
 	kfree(q->seq_zones_wlock);
 	q->seq_zones_wlock = NULL;
 }
 
 struct blk_revalidate_zone_args {
 	struct gendisk	*disk;
-	unsigned long	*seq_zones_bitmap;
+	unsigned long	*conv_zones_bitmap;
 	unsigned long	*seq_zones_wlock;
 	sector_t	sector;
 };
@@ -394,8 +394,8 @@ static int blk_revalidate_zone_cb(struct blk_zone *zone, unsigned int idx,
 		return -ENODEV;
 	}
 
-	if (zone->type != BLK_ZONE_TYPE_CONVENTIONAL)
-		set_bit(idx, args->seq_zones_bitmap);
+	if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL)
+		set_bit(idx, args->conv_zones_bitmap);
 
 	args->sector += zone->len;
 	return 0;
@@ -415,8 +415,8 @@ static int blk_update_zone_info(struct gendisk *disk, unsigned int nr_zones,
 	args->seq_zones_wlock = blk_alloc_zone_bitmap(q->node, nr_zones);
 	if (!args->seq_zones_wlock)
 		return -ENOMEM;
-	args->seq_zones_bitmap = blk_alloc_zone_bitmap(q->node, nr_zones);
-	if (!args->seq_zones_bitmap)
+	args->conv_zones_bitmap = blk_alloc_zone_bitmap(q->node, nr_zones);
+	if (!args->conv_zones_bitmap)
 		return -ENOMEM;
 
 	ret = disk->fops->report_zones(disk, 0, nr_zones,
@@ -465,7 +465,7 @@ int blk_revalidate_disk_zones(struct gendisk *disk)
 	if (ret >= 0) {
 		q->nr_zones = nr_zones;
 		swap(q->seq_zones_wlock, args.seq_zones_wlock);
-		swap(q->seq_zones_bitmap, args.seq_zones_bitmap);
+		swap(q->conv_zones_bitmap, args.conv_zones_bitmap);
 		ret = 0;
 	} else {
 		pr_warn("%s: failed to revalidate zones\n", disk->disk_name);
@@ -474,7 +474,7 @@ int blk_revalidate_disk_zones(struct gendisk *disk)
 	blk_mq_unfreeze_queue(q);
 
 	kfree(args.seq_zones_wlock);
-	kfree(args.seq_zones_bitmap);
+	kfree(args.conv_zones_bitmap);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(blk_revalidate_disk_zones);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index c5852de402b6..503c4d4c5884 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -503,9 +503,9 @@ struct request_queue {
 	/*
 	 * Zoned block device information for request dispatch control.
 	 * nr_zones is the total number of zones of the device. This is always
-	 * 0 for regular block devices. seq_zones_bitmap is a bitmap of nr_zones
-	 * bits which indicates if a zone is conventional (bit clear) or
-	 * sequential (bit set). seq_zones_wlock is a bitmap of nr_zones
+	 * 0 for regular block devices. conv_zones_bitmap is a bitmap of nr_zones
+	 * bits which indicates if a zone is conventional (bit set) or
+	 * sequential (bit clear). seq_zones_wlock is a bitmap of nr_zones
 	 * bits which indicates if a zone is write locked, that is, if a write
 	 * request targeting the zone was dispatched. All three fields are
 	 * initialized by the low level device driver (e.g. scsi/sd.c).
@@ -518,7 +518,7 @@ struct request_queue {
 	 * blk_mq_unfreeze_queue().
 	 */
 	unsigned int		nr_zones;
-	unsigned long		*seq_zones_bitmap;
+	unsigned long		*conv_zones_bitmap;
 	unsigned long		*seq_zones_wlock;
 #endif /* CONFIG_BLK_DEV_ZONED */
 
@@ -723,9 +723,11 @@ static inline unsigned int blk_queue_zone_no(struct request_queue *q,
 static inline bool blk_queue_zone_is_seq(struct request_queue *q,
 					 sector_t sector)
 {
-	if (!blk_queue_is_zoned(q) || !q->seq_zones_bitmap)
+	if (!blk_queue_is_zoned(q))
 		return false;
-	return test_bit(blk_queue_zone_no(q, sector), q->seq_zones_bitmap);
+	if (!q->conv_zones_bitmap)
+		return true;
+	return !test_bit(blk_queue_zone_no(q, sector), q->conv_zones_bitmap);
 }
 #else /* CONFIG_BLK_DEV_ZONED */
 static inline unsigned int blk_queue_nr_zones(struct request_queue *q)
-- 
2.20.1

Re: [PATCH 5/8] block: replace seq_zones_bitmap with conv_zones_bitmap

[Javier González]

On 03.12.2019 10:39, Christoph Hellwig wrote:
>Invert the meaning of seq_zones_bitmap by keeping a bitmap of
>conventional zones.  This allows not having a bitmap for devices
>that do not have conventional zones.
>
>Signed-off-by: Christoph Hellwig <hch@lst.de>
>---
> block/blk-zoned.c      | 18 +++++++++---------
> include/linux/blkdev.h | 14 ++++++++------
> 2 files changed, 17 insertions(+), 15 deletions(-)
>
>diff --git a/block/blk-zoned.c b/block/blk-zoned.c
>index 65a9bdc9fe27..9c3931051f4f 100644
>--- a/block/blk-zoned.c
>+++ b/block/blk-zoned.c
>@@ -332,15 +332,15 @@ static inline unsigned long *blk_alloc_zone_bitmap(int node,
>
> void blk_queue_free_zone_bitmaps(struct request_queue *q)
> {
>-	kfree(q->seq_zones_bitmap);
>-	q->seq_zones_bitmap = NULL;
>+	kfree(q->conv_zones_bitmap);
>+	q->conv_zones_bitmap = NULL;
> 	kfree(q->seq_zones_wlock);
> 	q->seq_zones_wlock = NULL;
> }
>
> struct blk_revalidate_zone_args {
> 	struct gendisk	*disk;
>-	unsigned long	*seq_zones_bitmap;
>+	unsigned long	*conv_zones_bitmap;
> 	unsigned long	*seq_zones_wlock;
> 	sector_t	sector;
> };
>@@ -394,8 +394,8 @@ static int blk_revalidate_zone_cb(struct blk_zone *zone, unsigned int idx,
> 		return -ENODEV;
> 	}
>
>-	if (zone->type != BLK_ZONE_TYPE_CONVENTIONAL)
>-		set_bit(idx, args->seq_zones_bitmap);
>+	if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL)
>+		set_bit(idx, args->conv_zones_bitmap);
>
> 	args->sector += zone->len;
> 	return 0;
>@@ -415,8 +415,8 @@ static int blk_update_zone_info(struct gendisk *disk, unsigned int nr_zones,
> 	args->seq_zones_wlock = blk_alloc_zone_bitmap(q->node, nr_zones);
> 	if (!args->seq_zones_wlock)
> 		return -ENOMEM;
>-	args->seq_zones_bitmap = blk_alloc_zone_bitmap(q->node, nr_zones);
>-	if (!args->seq_zones_bitmap)
>+	args->conv_zones_bitmap = blk_alloc_zone_bitmap(q->node, nr_zones);
>+	if (!args->conv_zones_bitmap)
> 		return -ENOMEM;
>
> 	ret = disk->fops->report_zones(disk, 0, nr_zones,
>@@ -465,7 +465,7 @@ int blk_revalidate_disk_zones(struct gendisk *disk)
> 	if (ret >= 0) {
> 		q->nr_zones = nr_zones;
> 		swap(q->seq_zones_wlock, args.seq_zones_wlock);
>-		swap(q->seq_zones_bitmap, args.seq_zones_bitmap);
>+		swap(q->conv_zones_bitmap, args.conv_zones_bitmap);
> 		ret = 0;
> 	} else {
> 		pr_warn("%s: failed to revalidate zones\n", disk->disk_name);
>@@ -474,7 +474,7 @@ int blk_revalidate_disk_zones(struct gendisk *disk)
> 	blk_mq_unfreeze_queue(q);
>
> 	kfree(args.seq_zones_wlock);
>-	kfree(args.seq_zones_bitmap);
>+	kfree(args.conv_zones_bitmap);
> 	return ret;
> }
> EXPORT_SYMBOL_GPL(blk_revalidate_disk_zones);
>diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>index c5852de402b6..503c4d4c5884 100644
>--- a/include/linux/blkdev.h
>+++ b/include/linux/blkdev.h
>@@ -503,9 +503,9 @@ struct request_queue {
> 	/*
> 	 * Zoned block device information for request dispatch control.
> 	 * nr_zones is the total number of zones of the device. This is always
>-	 * 0 for regular block devices. seq_zones_bitmap is a bitmap of nr_zones
>-	 * bits which indicates if a zone is conventional (bit clear) or
>-	 * sequential (bit set). seq_zones_wlock is a bitmap of nr_zones
>+	 * 0 for regular block devices. conv_zones_bitmap is a bitmap of nr_zones
>+	 * bits which indicates if a zone is conventional (bit set) or
>+	 * sequential (bit clear). seq_zones_wlock is a bitmap of nr_zones
> 	 * bits which indicates if a zone is write locked, that is, if a write
> 	 * request targeting the zone was dispatched. All three fields are
> 	 * initialized by the low level device driver (e.g. scsi/sd.c).
>@@ -518,7 +518,7 @@ struct request_queue {
> 	 * blk_mq_unfreeze_queue().
> 	 */
> 	unsigned int		nr_zones;
>-	unsigned long		*seq_zones_bitmap;
>+	unsigned long		*conv_zones_bitmap;
> 	unsigned long		*seq_zones_wlock;
> #endif /* CONFIG_BLK_DEV_ZONED */
>
>@@ -723,9 +723,11 @@ static inline unsigned int blk_queue_zone_no(struct request_queue *q,
> static inline bool blk_queue_zone_is_seq(struct request_queue *q,
> 					 sector_t sector)
> {
>-	if (!blk_queue_is_zoned(q) || !q->seq_zones_bitmap)
>+	if (!blk_queue_is_zoned(q))
> 		return false;
>-	return test_bit(blk_queue_zone_no(q, sector), q->seq_zones_bitmap);
>+	if (!q->conv_zones_bitmap)
>+		return true;
>+	return !test_bit(blk_queue_zone_no(q, sector), q->conv_zones_bitmap);
> }
> #else /* CONFIG_BLK_DEV_ZONED */
> static inline unsigned int blk_queue_nr_zones(struct request_queue *q)
>-- 
>2.20.1
>

Looks good to me.

Reviewed-by: Javier González <javier@javigon.com>

[PATCH 6/8] block: allocate the zone bitmaps lazily

[Christoph Hellwig]

Allocate the conventional zone bitmap and the sequential zone locking
bitmap only when we find a zone of the respective type.  This avoids
wasting memory on the conventional zone bitmap for devices that only
have sequential zones, and will also prepare for other future changes.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-zoned.c | 65 +++++++++++++++++++++++------------------------
 1 file changed, 32 insertions(+), 33 deletions(-)

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 9c3931051f4f..0131f9e14bd1 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -342,6 +342,7 @@ struct blk_revalidate_zone_args {
 	struct gendisk	*disk;
 	unsigned long	*conv_zones_bitmap;
 	unsigned long	*seq_zones_wlock;
+	unsigned int	nr_zones;
 	sector_t	sector;
 };
 
@@ -385,8 +386,22 @@ static int blk_revalidate_zone_cb(struct blk_zone *zone, unsigned int idx,
 	/* Check zone type */
 	switch (zone->type) {
 	case BLK_ZONE_TYPE_CONVENTIONAL:
+		if (!args->conv_zones_bitmap) {
+			args->conv_zones_bitmap =
+				blk_alloc_zone_bitmap(q->node, args->nr_zones);
+			if (!args->conv_zones_bitmap)
+				return -ENOMEM;
+		}
+		set_bit(idx, args->conv_zones_bitmap);
+		break;
 	case BLK_ZONE_TYPE_SEQWRITE_REQ:
 	case BLK_ZONE_TYPE_SEQWRITE_PREF:
+		if (!args->seq_zones_wlock) {
+			args->seq_zones_wlock =
+				blk_alloc_zone_bitmap(q->node, args->nr_zones);
+			if (!args->seq_zones_wlock)
+				return -ENOMEM;
+		}
 		break;
 	default:
 		pr_warn("%s: Invalid zone type 0x%x at sectors %llu\n",
@@ -394,37 +409,10 @@ static int blk_revalidate_zone_cb(struct blk_zone *zone, unsigned int idx,
 		return -ENODEV;
 	}
 
-	if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL)
-		set_bit(idx, args->conv_zones_bitmap);
-
 	args->sector += zone->len;
 	return 0;
 }
 
-static int blk_update_zone_info(struct gendisk *disk, unsigned int nr_zones,
-				struct blk_revalidate_zone_args *args)
-{
-	/*
-	 * Ensure that all memory allocations in this context are done as
-	 * if GFP_NOIO was specified.
-	 */
-	unsigned int noio_flag = memalloc_noio_save();
-	struct request_queue *q = disk->queue;
-	int ret;
-
-	args->seq_zones_wlock = blk_alloc_zone_bitmap(q->node, nr_zones);
-	if (!args->seq_zones_wlock)
-		return -ENOMEM;
-	args->conv_zones_bitmap = blk_alloc_zone_bitmap(q->node, nr_zones);
-	if (!args->conv_zones_bitmap)
-		return -ENOMEM;
-
-	ret = disk->fops->report_zones(disk, 0, nr_zones,
-				       blk_revalidate_zone_cb, args);
-	memalloc_noio_restore(noio_flag);
-	return ret;
-}
-
 /**
  * blk_revalidate_disk_zones - (re)allocate and initialize zone bitmaps
  * @disk:	Target disk
@@ -437,8 +425,10 @@ static int blk_update_zone_info(struct gendisk *disk, unsigned int nr_zones,
 int blk_revalidate_disk_zones(struct gendisk *disk)
 {
 	struct request_queue *q = disk->queue;
-	unsigned int nr_zones = blkdev_nr_zones(disk);
-	struct blk_revalidate_zone_args args = { .disk = disk };
+	struct blk_revalidate_zone_args args = {
+		.disk		= disk,
+		.nr_zones	= blkdev_nr_zones(disk),
+	};
 	int ret = 0;
 
 	if (WARN_ON_ONCE(!blk_queue_is_zoned(q)))
@@ -449,12 +439,21 @@ int blk_revalidate_disk_zones(struct gendisk *disk)
 	 * needs to be updated so that the sysfs exposed value is correct.
 	 */
 	if (!queue_is_mq(q)) {
-		q->nr_zones = nr_zones;
+		q->nr_zones = args.nr_zones;
 		return 0;
 	}
 
-	if (nr_zones)
-		ret = blk_update_zone_info(disk, nr_zones, &args);
+	/*
+	 * Ensure that all memory allocations in this context are done as
+	 * if GFP_NOIO was specified.
+	 */
+	if (args.nr_zones) {
+		unsigned int noio_flag = memalloc_noio_save();
+
+		ret = disk->fops->report_zones(disk, 0, args.nr_zones,
+					       blk_revalidate_zone_cb, &args);
+		memalloc_noio_restore(noio_flag);
+	}
 
 	/*
 	 * Install the new bitmaps, making sure the queue is stopped and
@@ -463,7 +462,7 @@ int blk_revalidate_disk_zones(struct gendisk *disk)
 	 */
 	blk_mq_freeze_queue(q);
 	if (ret >= 0) {
-		q->nr_zones = nr_zones;
+		q->nr_zones = args.nr_zones;
 		swap(q->seq_zones_wlock, args.seq_zones_wlock);
 		swap(q->conv_zones_bitmap, args.conv_zones_bitmap);
 		ret = 0;
-- 
2.20.1

Re: [PATCH 6/8] block: allocate the zone bitmaps lazily

[Javier González]

On 03.12.2019 10:39, Christoph Hellwig wrote:
>Allocate the conventional zone bitmap and the sequential zone locking
>bitmap only when we find a zone of the respective type.  This avoids
>wasting memory on the conventional zone bitmap for devices that only
>have sequential zones, and will also prepare for other future changes.
>
>Signed-off-by: Christoph Hellwig <hch@lst.de>
>---
> block/blk-zoned.c | 65 +++++++++++++++++++++++------------------------
> 1 file changed, 32 insertions(+), 33 deletions(-)
>
>diff --git a/block/blk-zoned.c b/block/blk-zoned.c
>index 9c3931051f4f..0131f9e14bd1 100644
>--- a/block/blk-zoned.c
>+++ b/block/blk-zoned.c
>@@ -342,6 +342,7 @@ struct blk_revalidate_zone_args {
> 	struct gendisk	*disk;
> 	unsigned long	*conv_zones_bitmap;
> 	unsigned long	*seq_zones_wlock;
>+	unsigned int	nr_zones;
> 	sector_t	sector;
> };
>
>@@ -385,8 +386,22 @@ static int blk_revalidate_zone_cb(struct blk_zone *zone, unsigned int idx,
> 	/* Check zone type */
> 	switch (zone->type) {
> 	case BLK_ZONE_TYPE_CONVENTIONAL:
>+		if (!args->conv_zones_bitmap) {
>+			args->conv_zones_bitmap =
>+				blk_alloc_zone_bitmap(q->node, args->nr_zones);
>+			if (!args->conv_zones_bitmap)
>+				return -ENOMEM;
>+		}
>+		set_bit(idx, args->conv_zones_bitmap);
>+		break;
> 	case BLK_ZONE_TYPE_SEQWRITE_REQ:
> 	case BLK_ZONE_TYPE_SEQWRITE_PREF:
>+		if (!args->seq_zones_wlock) {
>+			args->seq_zones_wlock =
>+				blk_alloc_zone_bitmap(q->node, args->nr_zones);
>+			if (!args->seq_zones_wlock)
>+				return -ENOMEM;
>+		}
> 		break;
> 	default:
> 		pr_warn("%s: Invalid zone type 0x%x at sectors %llu\n",
>@@ -394,37 +409,10 @@ static int blk_revalidate_zone_cb(struct blk_zone *zone, unsigned int idx,
> 		return -ENODEV;
> 	}
>
>-	if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL)
>-		set_bit(idx, args->conv_zones_bitmap);
>-
> 	args->sector += zone->len;
> 	return 0;
> }
>
>-static int blk_update_zone_info(struct gendisk *disk, unsigned int nr_zones,
>-				struct blk_revalidate_zone_args *args)
>-{
>-	/*
>-	 * Ensure that all memory allocations in this context are done as
>-	 * if GFP_NOIO was specified.
>-	 */
>-	unsigned int noio_flag = memalloc_noio_save();
>-	struct request_queue *q = disk->queue;
>-	int ret;
>-
>-	args->seq_zones_wlock = blk_alloc_zone_bitmap(q->node, nr_zones);
>-	if (!args->seq_zones_wlock)
>-		return -ENOMEM;
>-	args->conv_zones_bitmap = blk_alloc_zone_bitmap(q->node, nr_zones);
>-	if (!args->conv_zones_bitmap)
>-		return -ENOMEM;
>-
>-	ret = disk->fops->report_zones(disk, 0, nr_zones,
>-				       blk_revalidate_zone_cb, args);
>-	memalloc_noio_restore(noio_flag);
>-	return ret;
>-}
>-
> /**
>  * blk_revalidate_disk_zones - (re)allocate and initialize zone bitmaps
>  * @disk:	Target disk
>@@ -437,8 +425,10 @@ static int blk_update_zone_info(struct gendisk *disk, unsigned int nr_zones,
> int blk_revalidate_disk_zones(struct gendisk *disk)
> {
> 	struct request_queue *q = disk->queue;
>-	unsigned int nr_zones = blkdev_nr_zones(disk);
>-	struct blk_revalidate_zone_args args = { .disk = disk };
>+	struct blk_revalidate_zone_args args = {
>+		.disk		= disk,
>+		.nr_zones	= blkdev_nr_zones(disk),
>+	};
> 	int ret = 0;
>
> 	if (WARN_ON_ONCE(!blk_queue_is_zoned(q)))
>@@ -449,12 +439,21 @@ int blk_revalidate_disk_zones(struct gendisk *disk)
> 	 * needs to be updated so that the sysfs exposed value is correct.
> 	 */
> 	if (!queue_is_mq(q)) {
>-		q->nr_zones = nr_zones;
>+		q->nr_zones = args.nr_zones;
> 		return 0;
> 	}
>
>-	if (nr_zones)
>-		ret = blk_update_zone_info(disk, nr_zones, &args);
>+	/*
>+	 * Ensure that all memory allocations in this context are done as
>+	 * if GFP_NOIO was specified.
>+	 */
>+	if (args.nr_zones) {
>+		unsigned int noio_flag = memalloc_noio_save();
>+
>+		ret = disk->fops->report_zones(disk, 0, args.nr_zones,
>+					       blk_revalidate_zone_cb, &args);
>+		memalloc_noio_restore(noio_flag);
>+	}
>
> 	/*
> 	 * Install the new bitmaps, making sure the queue is stopped and
>@@ -463,7 +462,7 @@ int blk_revalidate_disk_zones(struct gendisk *disk)
> 	 */
> 	blk_mq_freeze_queue(q);
> 	if (ret >= 0) {
>-		q->nr_zones = nr_zones;
>+		q->nr_zones = args.nr_zones;
> 		swap(q->seq_zones_wlock, args.seq_zones_wlock);
> 		swap(q->conv_zones_bitmap, args.conv_zones_bitmap);
> 		ret = 0;
>-- 
>2.20.1

Looks good to me.

Reviewed-by: Javier González <javier@javigon.com>

[PATCH 7/8] block: don't handle bio based drivers in blk_revalidate_disk_zones

[Christoph Hellwig]

bio based drivers only need to update q->nr_zones.  Do that manually
instead of overloading blk_revalidate_disk_zones to keep that function
simpler for the next round of changes that will rely even more on the
request based functionality.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-zoned.c             | 16 +++++-----------
 drivers/block/null_blk_main.c | 12 +++++++++---
 drivers/md/dm-table.c         | 12 +++++++-----
 include/linux/blkdev.h        |  5 -----
 4 files changed, 21 insertions(+), 24 deletions(-)

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 0131f9e14bd1..51d427659ce7 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -419,8 +419,9 @@ static int blk_revalidate_zone_cb(struct blk_zone *zone, unsigned int idx,
  *
  * Helper function for low-level device drivers to (re) allocate and initialize
  * a disk request queue zone bitmaps. This functions should normally be called
- * within the disk ->revalidate method. For BIO based queues, no zone bitmap
- * is allocated.
+ * within the disk ->revalidate method for blk-mq based drivers.  For BIO based
+ * drivers only q->nr_zones needs to be updated so that the sysfs exposed value
+ * is correct.
  */
 int blk_revalidate_disk_zones(struct gendisk *disk)
 {
@@ -433,15 +434,8 @@ int blk_revalidate_disk_zones(struct gendisk *disk)
 
 	if (WARN_ON_ONCE(!blk_queue_is_zoned(q)))
 		return -EIO;
-
-	/*
-	 * BIO based queues do not use a scheduler so only q->nr_zones
-	 * needs to be updated so that the sysfs exposed value is correct.
-	 */
-	if (!queue_is_mq(q)) {
-		q->nr_zones = args.nr_zones;
-		return 0;
-	}
+	if (WARN_ON_ONCE(!queue_is_mq(q)))
+		return -EIO;
 
 	/*
 	 * Ensure that all memory allocations in this context are done as
diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index dd6026289fbf..068cd0ae6e2c 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -1576,11 +1576,17 @@ static int null_gendisk_register(struct nullb *nullb)
 	disk->queue		= nullb->q;
 	strncpy(disk->disk_name, nullb->disk_name, DISK_NAME_LEN);
 
+#ifdef CONFIG_BLK_DEV_ZONED
 	if (nullb->dev->zoned) {
-		ret = blk_revalidate_disk_zones(disk);
-		if (ret)
-			return ret;
+		if (queue_is_mq(nullb->q)) {
+			ret = blk_revalidate_disk_zones(disk);
+			if (ret)
+				return ret;
+		} else {
+			nullb->q->nr_zones = blkdev_nr_zones(disk);
+		}
 	}
+#endif
 
 	add_disk(disk);
 	return 0;
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 2ae0c1913766..0a2cc197f62b 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1954,12 +1954,14 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 	/*
 	 * For a zoned target, the number of zones should be updated for the
 	 * correct value to be exposed in sysfs queue/nr_zones. For a BIO based
-	 * target, this is all that is needed. For a request based target, the
-	 * queue zone bitmaps must also be updated.
-	 * Use blk_revalidate_disk_zones() to handle this.
+	 * target, this is all that is needed.
 	 */
-	if (blk_queue_is_zoned(q))
-		blk_revalidate_disk_zones(t->md->disk);
+#ifdef CONFIG_BLK_DEV_ZONED
+	if (blk_queue_is_zoned(q)) {
+		WARN_ON_ONCE(queue_is_mq(q));
+		q->nr_zones = blkdev_nr_zones(t->md->disk);
+	}
+#endif
 
 	/* Allow reads to exceed readahead limits */
 	q->backing_dev_info->io_pages = limits->max_sectors >> (PAGE_SHIFT - 9);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 503c4d4c5884..47eb22a3b7f9 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -375,11 +375,6 @@ static inline unsigned int blkdev_nr_zones(struct gendisk *disk)
 	return 0;
 }
 
-static inline int blk_revalidate_disk_zones(struct gendisk *disk)
-{
-	return 0;
-}
-
 static inline int blkdev_report_zones_ioctl(struct block_device *bdev,
 					    fmode_t mode, unsigned int cmd,
 					    unsigned long arg)
-- 
2.20.1

Re: [PATCH 7/8] block: don't handle bio based drivers in blk_revalidate_disk_zones

[Javier González]

On 03.12.2019 10:39, Christoph Hellwig wrote:
>bio based drivers only need to update q->nr_zones.  Do that manually
>instead of overloading blk_revalidate_disk_zones to keep that function
>simpler for the next round of changes that will rely even more on the
>request based functionality.
>
>Signed-off-by: Christoph Hellwig <hch@lst.de>
>---
> block/blk-zoned.c             | 16 +++++-----------
> drivers/block/null_blk_main.c | 12 +++++++++---
> drivers/md/dm-table.c         | 12 +++++++-----
> include/linux/blkdev.h        |  5 -----
> 4 files changed, 21 insertions(+), 24 deletions(-)
>
>diff --git a/block/blk-zoned.c b/block/blk-zoned.c
>index 0131f9e14bd1..51d427659ce7 100644
>--- a/block/blk-zoned.c
>+++ b/block/blk-zoned.c
>@@ -419,8 +419,9 @@ static int blk_revalidate_zone_cb(struct blk_zone *zone, unsigned int idx,
>  *
>  * Helper function for low-level device drivers to (re) allocate and initialize
>  * a disk request queue zone bitmaps. This functions should normally be called
>- * within the disk ->revalidate method. For BIO based queues, no zone bitmap
>- * is allocated.
>+ * within the disk ->revalidate method for blk-mq based drivers.  For BIO based
>+ * drivers only q->nr_zones needs to be updated so that the sysfs exposed value
>+ * is correct.
>  */
> int blk_revalidate_disk_zones(struct gendisk *disk)
> {
>@@ -433,15 +434,8 @@ int blk_revalidate_disk_zones(struct gendisk *disk)
>
> 	if (WARN_ON_ONCE(!blk_queue_is_zoned(q)))
> 		return -EIO;
>-
>-	/*
>-	 * BIO based queues do not use a scheduler so only q->nr_zones
>-	 * needs to be updated so that the sysfs exposed value is correct.
>-	 */
>-	if (!queue_is_mq(q)) {
>-		q->nr_zones = args.nr_zones;
>-		return 0;
>-	}
>+	if (WARN_ON_ONCE(!queue_is_mq(q)))
>+		return -EIO;
>
> 	/*
> 	 * Ensure that all memory allocations in this context are done as
>diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
>index dd6026289fbf..068cd0ae6e2c 100644
>--- a/drivers/block/null_blk_main.c
>+++ b/drivers/block/null_blk_main.c
>@@ -1576,11 +1576,17 @@ static int null_gendisk_register(struct nullb *nullb)
> 	disk->queue		= nullb->q;
> 	strncpy(disk->disk_name, nullb->disk_name, DISK_NAME_LEN);
>
>+#ifdef CONFIG_BLK_DEV_ZONED
> 	if (nullb->dev->zoned) {
>-		ret = blk_revalidate_disk_zones(disk);
>-		if (ret)
>-			return ret;
>+		if (queue_is_mq(nullb->q)) {
>+			ret = blk_revalidate_disk_zones(disk);
>+			if (ret)
>+				return ret;
>+		} else {
>+			nullb->q->nr_zones = blkdev_nr_zones(disk);
>+		}
> 	}
>+#endif
>
> 	add_disk(disk);
> 	return 0;
>diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
>index 2ae0c1913766..0a2cc197f62b 100644
>--- a/drivers/md/dm-table.c
>+++ b/drivers/md/dm-table.c
>@@ -1954,12 +1954,14 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
> 	/*
> 	 * For a zoned target, the number of zones should be updated for the
> 	 * correct value to be exposed in sysfs queue/nr_zones. For a BIO based
>-	 * target, this is all that is needed. For a request based target, the
>-	 * queue zone bitmaps must also be updated.
>-	 * Use blk_revalidate_disk_zones() to handle this.
>+	 * target, this is all that is needed.
> 	 */
>-	if (blk_queue_is_zoned(q))
>-		blk_revalidate_disk_zones(t->md->disk);
>+#ifdef CONFIG_BLK_DEV_ZONED
>+	if (blk_queue_is_zoned(q)) {
>+		WARN_ON_ONCE(queue_is_mq(q));
>+		q->nr_zones = blkdev_nr_zones(t->md->disk);
>+	}
>+#endif
>
> 	/* Allow reads to exceed readahead limits */
> 	q->backing_dev_info->io_pages = limits->max_sectors >> (PAGE_SHIFT - 9);
>diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>index 503c4d4c5884..47eb22a3b7f9 100644
>--- a/include/linux/blkdev.h
>+++ b/include/linux/blkdev.h
>@@ -375,11 +375,6 @@ static inline unsigned int blkdev_nr_zones(struct gendisk *disk)
> 	return 0;
> }
>
>-static inline int blk_revalidate_disk_zones(struct gendisk *disk)
>-{
>-	return 0;
>-}
>-
> static inline int blkdev_report_zones_ioctl(struct block_device *bdev,
> 					    fmode_t mode, unsigned int cmd,
> 					    unsigned long arg)
>-- 
>2.20.1
>

Looks good to me.

Reviewed-by: Javier González <javier@javigon.com>

[PATCH 8/8] block: set the zone size in blk_revalidate_disk_zones atomically

[Christoph Hellwig]

The current zone revalidation code has a major problem in that it
doesn't update the zone size and q->nr_zones atomically, leading
to a short window where an out of bounds access to the zone arrays
is possible.

To fix this move the setting of the zone size into the crticial
sections blk_revalidate_disk_zones so that it gets updated together
with the zone bitmaps and q->nr_zones.  This also slightly simplifies
the caller as it deducts the zone size from the report_zones.

This change also allows to check for a power of two zone size in generic
code.

Reported-by: Hans Holmberg <hans@owltronix.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-zoned.c             | 59 ++++++++++++++++++++---------------
 drivers/block/null_blk_main.c |  3 +-
 drivers/scsi/sd_zbc.c         |  2 --
 3 files changed, 35 insertions(+), 29 deletions(-)

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 51d427659ce7..d00fcfd71dfe 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -343,6 +343,7 @@ struct blk_revalidate_zone_args {
 	unsigned long	*conv_zones_bitmap;
 	unsigned long	*seq_zones_wlock;
 	unsigned int	nr_zones;
+	sector_t	zone_sectors;
 	sector_t	sector;
 };
 
@@ -355,25 +356,33 @@ static int blk_revalidate_zone_cb(struct blk_zone *zone, unsigned int idx,
 	struct blk_revalidate_zone_args *args = data;
 	struct gendisk *disk = args->disk;
 	struct request_queue *q = disk->queue;
-	sector_t zone_sectors = blk_queue_zone_sectors(q);
 	sector_t capacity = get_capacity(disk);
 
 	/*
 	 * All zones must have the same size, with the exception on an eventual
 	 * smaller last zone.
 	 */
-	if (zone->start + zone_sectors < capacity &&
-	    zone->len != zone_sectors) {
-		pr_warn("%s: Invalid zoned device with non constant zone size\n",
-			disk->disk_name);
-		return false;
-	}
+	if (zone->start == 0) {
+		if (zone->len == 0 || !is_power_of_2(zone->len)) {
+			pr_warn("%s: Invalid zoned device with non power of two zone size (%llu)\n",
+				disk->disk_name, zone->len);
+			return -ENODEV;
+		}
 
-	if (zone->start + zone->len >= capacity &&
-	    zone->len > zone_sectors) {
-		pr_warn("%s: Invalid zoned device with larger last zone size\n",
-			disk->disk_name);
-		return -ENODEV;
+		args->zone_sectors = zone->len;
+		args->nr_zones = (capacity + zone->len - 1) >> ilog2(zone->len);
+	} else if (zone->start + args->zone_sectors < capacity) {
+		if (zone->len != args->zone_sectors) {
+			pr_warn("%s: Invalid zoned device with non constant zone size\n",
+				disk->disk_name);
+			return -ENODEV;
+		}
+	} else {
+		if (zone->len > args->zone_sectors) {
+			pr_warn("%s: Invalid zoned device with larger last zone size\n",
+				disk->disk_name);
+			return -ENODEV;
+		}
 	}
 
 	/* Check for holes in the zone report */
@@ -428,9 +437,9 @@ int blk_revalidate_disk_zones(struct gendisk *disk)
 	struct request_queue *q = disk->queue;
 	struct blk_revalidate_zone_args args = {
 		.disk		= disk,
-		.nr_zones	= blkdev_nr_zones(disk),
 	};
-	int ret = 0;
+	unsigned int noio_flag;
+	int ret;
 
 	if (WARN_ON_ONCE(!blk_queue_is_zoned(q)))
 		return -EIO;
@@ -438,24 +447,22 @@ int blk_revalidate_disk_zones(struct gendisk *disk)
 		return -EIO;
 
 	/*
-	 * Ensure that all memory allocations in this context are done as
-	 * if GFP_NOIO was specified.
+	 * Ensure that all memory allocations in this context are done as if
+	 * GFP_NOIO was specified.
 	 */
-	if (args.nr_zones) {
-		unsigned int noio_flag = memalloc_noio_save();
-
-		ret = disk->fops->report_zones(disk, 0, args.nr_zones,
-					       blk_revalidate_zone_cb, &args);
-		memalloc_noio_restore(noio_flag);
-	}
+	noio_flag = memalloc_noio_save();
+	ret = disk->fops->report_zones(disk, 0, UINT_MAX,
+				       blk_revalidate_zone_cb, &args);
+	memalloc_noio_restore(noio_flag);
 
 	/*
-	 * Install the new bitmaps, making sure the queue is stopped and
-	 * all I/Os are completed (i.e. a scheduler is not referencing the
-	 * bitmaps).
+	 * Install the new bitmaps and update nr_zones only once the queue is
+	 * stopped and all I/Os are completed (i.e. a scheduler is not
+	 * referencing the bitmaps).
 	 */
 	blk_mq_freeze_queue(q);
 	if (ret >= 0) {
+		blk_queue_chunk_sectors(q, args.zone_sectors);
 		q->nr_zones = args.nr_zones;
 		swap(q->seq_zones_wlock, args.seq_zones_wlock);
 		swap(q->conv_zones_bitmap, args.conv_zones_bitmap);
diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index 068cd0ae6e2c..997b7dc095b9 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -1583,6 +1583,8 @@ static int null_gendisk_register(struct nullb *nullb)
 			if (ret)
 				return ret;
 		} else {
+			blk_queue_chunk_sectors(nullb->q,
+					nullb->dev->zone_size_sects);
 			nullb->q->nr_zones = blkdev_nr_zones(disk);
 		}
 	}
@@ -1746,7 +1748,6 @@ static int null_add_dev(struct nullb_device *dev)
 		if (rv)
 			goto out_cleanup_blk_queue;
 
-		blk_queue_chunk_sectors(nullb->q, dev->zone_size_sects);
 		nullb->q->limits.zoned = BLK_ZONED_HM;
 		blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, nullb->q);
 		blk_queue_required_elevator_features(nullb->q,
diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index 0e5ede48f045..27d72c1d4654 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -412,8 +412,6 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, unsigned char *buf)
 		goto err;
 
 	/* The drive satisfies the kernel restrictions: set it up */
-	blk_queue_chunk_sectors(sdkp->disk->queue,
-			logical_to_sectors(sdkp->device, zone_blocks));
 	blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, sdkp->disk->queue);
 	blk_queue_required_elevator_features(sdkp->disk->queue,
 					     ELEVATOR_F_ZBD_SEQ_WRITE);
-- 
2.20.1

Re: [PATCH 8/8] block: set the zone size in blk_revalidate_disk_zones atomically

[Javier González]

On 03.12.2019 10:39, Christoph Hellwig wrote:
>The current zone revalidation code has a major problem in that it
>doesn't update the zone size and q->nr_zones atomically, leading
>to a short window where an out of bounds access to the zone arrays
>is possible.
>
>To fix this move the setting of the zone size into the crticial

nip: critical

>sections blk_revalidate_disk_zones so that it gets updated together
>with the zone bitmaps and q->nr_zones.  This also slightly simplifies
>the caller as it deducts the zone size from the report_zones.

This part makes sense. Good catch.
>
>This change also allows to check for a power of two zone size in generic
>code.

I think however that this checks should remain at the driver level, or
at least depend on a flag that signals that the zoned device is actually
a power of two.

>
>Reported-by: Hans Holmberg <hans@owltronix.com>
>Signed-off-by: Christoph Hellwig <hch@lst.de>
>---
> block/blk-zoned.c             | 59 ++++++++++++++++++++---------------
> drivers/block/null_blk_main.c |  3 +-
> drivers/scsi/sd_zbc.c         |  2 --
> 3 files changed, 35 insertions(+), 29 deletions(-)
>
>diff --git a/block/blk-zoned.c b/block/blk-zoned.c
>index 51d427659ce7..d00fcfd71dfe 100644
>--- a/block/blk-zoned.c
>+++ b/block/blk-zoned.c
>@@ -343,6 +343,7 @@ struct blk_revalidate_zone_args {
> 	unsigned long	*conv_zones_bitmap;
> 	unsigned long	*seq_zones_wlock;
> 	unsigned int	nr_zones;
>+	sector_t	zone_sectors;
> 	sector_t	sector;
> };
>
>@@ -355,25 +356,33 @@ static int blk_revalidate_zone_cb(struct blk_zone *zone, unsigned int idx,
> 	struct blk_revalidate_zone_args *args = data;
> 	struct gendisk *disk = args->disk;
> 	struct request_queue *q = disk->queue;
>-	sector_t zone_sectors = blk_queue_zone_sectors(q);
> 	sector_t capacity = get_capacity(disk);
>
> 	/*
> 	 * All zones must have the same size, with the exception on an eventual
> 	 * smaller last zone.
> 	 */
>-	if (zone->start + zone_sectors < capacity &&
>-	    zone->len != zone_sectors) {
>-		pr_warn("%s: Invalid zoned device with non constant zone size\n",
>-			disk->disk_name);
>-		return false;
>-	}
>+	if (zone->start == 0) {
>+		if (zone->len == 0 || !is_power_of_2(zone->len)) {
>+			pr_warn("%s: Invalid zoned device with non power of two zone size (%llu)\n",
>+				disk->disk_name, zone->len);
>+			return -ENODEV;
>+		}
>
>-	if (zone->start + zone->len >= capacity &&
>-	    zone->len > zone_sectors) {
>-		pr_warn("%s: Invalid zoned device with larger last zone size\n",
>-			disk->disk_name);
>-		return -ENODEV;
>+		args->zone_sectors = zone->len;
>+		args->nr_zones = (capacity + zone->len - 1) >> ilog2(zone->len);
>+	} else if (zone->start + args->zone_sectors < capacity) {
>+		if (zone->len != args->zone_sectors) {
>+			pr_warn("%s: Invalid zoned device with non constant zone size\n",
>+				disk->disk_name);
>+			return -ENODEV;
>+		}
>+	} else {
>+		if (zone->len > args->zone_sectors) {
>+			pr_warn("%s: Invalid zoned device with larger last zone size\n",
>+				disk->disk_name);
>+			return -ENODEV;
>+		}
> 	}
>
> 	/* Check for holes in the zone report */
>@@ -428,9 +437,9 @@ int blk_revalidate_disk_zones(struct gendisk *disk)
> 	struct request_queue *q = disk->queue;
> 	struct blk_revalidate_zone_args args = {
> 		.disk		= disk,
>-		.nr_zones	= blkdev_nr_zones(disk),
> 	};
>-	int ret = 0;
>+	unsigned int noio_flag;
>+	int ret;
>
> 	if (WARN_ON_ONCE(!blk_queue_is_zoned(q)))
> 		return -EIO;
>@@ -438,24 +447,22 @@ int blk_revalidate_disk_zones(struct gendisk *disk)
> 		return -EIO;
>
> 	/*
>-	 * Ensure that all memory allocations in this context are done as
>-	 * if GFP_NOIO was specified.
>+	 * Ensure that all memory allocations in this context are done as if
>+	 * GFP_NOIO was specified.
> 	 */
>-	if (args.nr_zones) {
>-		unsigned int noio_flag = memalloc_noio_save();
>-
>-		ret = disk->fops->report_zones(disk, 0, args.nr_zones,
>-					       blk_revalidate_zone_cb, &args);
>-		memalloc_noio_restore(noio_flag);
>-	}
>+	noio_flag = memalloc_noio_save();
>+	ret = disk->fops->report_zones(disk, 0, UINT_MAX,
>+				       blk_revalidate_zone_cb, &args);
>+	memalloc_noio_restore(noio_flag);
>
> 	/*
>-	 * Install the new bitmaps, making sure the queue is stopped and
>-	 * all I/Os are completed (i.e. a scheduler is not referencing the
>-	 * bitmaps).
>+	 * Install the new bitmaps and update nr_zones only once the queue is
>+	 * stopped and all I/Os are completed (i.e. a scheduler is not
>+	 * referencing the bitmaps).
> 	 */
> 	blk_mq_freeze_queue(q);
> 	if (ret >= 0) {
>+		blk_queue_chunk_sectors(q, args.zone_sectors);
> 		q->nr_zones = args.nr_zones;
> 		swap(q->seq_zones_wlock, args.seq_zones_wlock);
> 		swap(q->conv_zones_bitmap, args.conv_zones_bitmap);
>diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
>index 068cd0ae6e2c..997b7dc095b9 100644
>--- a/drivers/block/null_blk_main.c
>+++ b/drivers/block/null_blk_main.c
>@@ -1583,6 +1583,8 @@ static int null_gendisk_register(struct nullb *nullb)
> 			if (ret)
> 				return ret;
> 		} else {
>+			blk_queue_chunk_sectors(nullb->q,
>+					nullb->dev->zone_size_sects);
> 			nullb->q->nr_zones = blkdev_nr_zones(disk);
> 		}
> 	}
>@@ -1746,7 +1748,6 @@ static int null_add_dev(struct nullb_device *dev)
> 		if (rv)
> 			goto out_cleanup_blk_queue;
>
>-		blk_queue_chunk_sectors(nullb->q, dev->zone_size_sects);
> 		nullb->q->limits.zoned = BLK_ZONED_HM;
> 		blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, nullb->q);
> 		blk_queue_required_elevator_features(nullb->q,
>diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
>index 0e5ede48f045..27d72c1d4654 100644
>--- a/drivers/scsi/sd_zbc.c
>+++ b/drivers/scsi/sd_zbc.c
>@@ -412,8 +412,6 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, unsigned char *buf)
> 		goto err;
>
> 	/* The drive satisfies the kernel restrictions: set it up */
>-	blk_queue_chunk_sectors(sdkp->disk->queue,
>-			logical_to_sectors(sdkp->device, zone_blocks));
> 	blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, sdkp->disk->queue);
> 	blk_queue_required_elevator_features(sdkp->disk->queue,
> 					     ELEVATOR_F_ZBD_SEQ_WRITE);
>-- 
>2.20.1
>

Re: [PATCH 8/8] block: set the zone size in blk_revalidate_disk_zones atomically

[Christoph Hellwig]

On Tue, Dec 03, 2019 at 03:00:09PM +0100, Javier González wrote:
>> This change also allows to check for a power of two zone size in generic
>> code.
>
> I think however that this checks should remain at the driver level, or
> at least depend on a flag that signals that the zoned device is actually
> a power of two.

The block layer requires a power of two zone size / chunk size,
including having a BUG_ON for that requirement blk_queue_chunk_sectors.
I'd much rather have a proper check in the zone code with proper
diagnostics than triggering a BUG_ON..

Re: [PATCH 8/8] block: set the zone size in blk_revalidate_disk_zones atomically

[Javier González]

> On 3 Dec 2019, at 16.18, Christoph Hellwig <hch@lst.de> wrote:
> 
> On Tue, Dec 03, 2019 at 03:00:09PM +0100, Javier González wrote:
>>> This change also allows to check for a power of two zone size in generic
>>> code.
>> 
>> I think however that this checks should remain at the driver level, or
>> at least depend on a flag that signals that the zoned device is actually
>> a power of two.
> 
> The block layer requires a power of two zone size / chunk size,
> including having a BUG_ON for that requirement blk_queue_chunk_sectors.
> I'd much rather have a proper check in the zone code with proper
> diagnostics than triggering a BUG_ON..

Agree on the BUG_ON part.  But since you’re looking into this part now, would it make sense to do the check in the block layer only if the driver imposes a power of two? We can also do it down the road, but seems like double work.

Re: [PATCH 8/8] block: set the zone size in blk_revalidate_disk_zones atomically

[Christoph Hellwig]

On Tue, Dec 03, 2019 at 04:34:08PM +0100, Javier González wrote:
> Agree on the BUG_ON part.  But since you’re looking into this part now, would it make sense to do the check in the block layer only if the driver imposes a power of two? We can also do it down the road, but seems like double work.

The whole block layer chunk / zone handling has always assumed power
of two zone sizes.  Changing that would introduce expensive divisions
in the fast path.  This patch just moves the check to where it belongs.

Re: [PATCH 8/8] block: set the zone size in blk_revalidate_disk_zones atomically

[Javier González]

> On 3 Dec 2019, at 16.42, Christoph Hellwig <hch@lst.de> wrote:
> 
> On Tue, Dec 03, 2019 at 04:34:08PM +0100, Javier González wrote:
>> Agree on the BUG_ON part.  But since you’re looking into this part now, would it make sense to do the check in the block layer only if the driver imposes a power of two? We can also do it down the road, but seems like double work.
> 
> The whole block layer chunk / zone handling has always assumed power
> of two zone sizes.  Changing that would introduce expensive divisions
> in the fast path.  This patch just moves the check to where it belongs.

Ok. Let’s do the refactor now. Though, we will need to support this for zoned devices that are not powers of two, but we can add this path when time comes. 

You can add my reviewed-by 

Thanks Christoph!
Javier

Re: avoid out of bounds zone bitmap access

[Jens Axboe]

On 12/3/19 2:39 AM, Christoph Hellwig wrote:
> Hi Jens,
> 
> this series (against your for-linus branch) fixes a problem where updates
> to the zone information in the queue is not atomic, leading to a possible
> out of bounds access.  Please take a look and let me know if you think
> if this is ok for 5.5.

I think we should sneak this into 5.5, I have applied it.

-- 
Jens Axboe