Linux-mmc Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 00/10] RFC: move logical block size checking to the block core
@ 2020-07-21 10:52 Maxim Levitsky
  2020-07-21 10:52 ` [PATCH 01/10] block: introduce blk_is_valid_logical_block_size Maxim Levitsky
                   ` (9 more replies)
  0 siblings, 10 replies; 33+ messages in thread
From: Maxim Levitsky @ 2020-07-21 10:52 UTC (permalink / raw)
  To: linux-kernel
  Cc: Keith Busch, Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg,
	Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Damien Le Moal, Jason Wang, Maxim Levitsky,
	Stefan Hajnoczi, Colin Ian King, Michael S. Tsirkin,
	Paolo Bonzini, Ulf Hansson, Ajay Joshi, Ming Lei,
	open list:SONY MEMORYSTICK SUBSYSTEM, Christoph Hellwig,
	Satya Tangirala, open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov, Maxim Levitsky

This patch series aims to move the logical block size checking to the\r
block code.\r
\r
This was inspired by missing check for valid logical block size in\r
virtio-blk which causes the kernel to crash in a weird way later on\r
when it is invalid.\r
\r
I added blk_is_valid_logical_block_size which returns true iff the\r
block size is one of supported sizes.\r
\r
I added this check to virtio-blk, and also converted  few block drivers\r
that I am familiar with to use this interface instead of their\r
own implementation.\r
\r
Best regards,\r
	Maxim Levitsky\r
\r
Maxim Levitsky (10):\r
  block: introduce blk_is_valid_logical_block_size\r
  block: virtio-blk: check logical block size\r
  block: loop: use blk_is_valid_logical_block_size\r
  block: nbd: use blk_is_valid_logical_block_size\r
  block: null: use blk_is_valid_logical_block_size\r
  block: ms_block: use blk_is_valid_logical_block_size\r
  block: mspro_blk: use blk_is_valid_logical_block_size\r
  block: nvme: use blk_is_valid_logical_block_size\r
  block: scsi: sd: use blk_is_valid_logical_block_size\r
  block: scsi: sr: use blk_is_valid_logical_block_size\r
\r
 block/blk-settings.c                | 18 +++++++++++++++++\r
 drivers/block/loop.c                | 23 +++++----------------\r
 drivers/block/nbd.c                 | 12 ++---------\r
 drivers/block/null_blk_main.c       |  6 +++---\r
 drivers/block/virtio_blk.c          | 15 ++++++++++++--\r
 drivers/memstick/core/ms_block.c    |  2 +-\r
 drivers/memstick/core/mspro_block.c |  6 ++++++\r
 drivers/nvme/host/core.c            | 17 ++++++++--------\r
 drivers/scsi/sd.c                   |  5 +----\r
 drivers/scsi/sr.c                   | 31 ++++++++++++-----------------\r
 include/linux/blkdev.h              |  1 +\r
 11 files changed, 71 insertions(+), 65 deletions(-)\r
\r
-- \r
2.26.2\r
\r


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 01/10] block: introduce blk_is_valid_logical_block_size
  2020-07-21 10:52 [PATCH 00/10] RFC: move logical block size checking to the block core Maxim Levitsky
@ 2020-07-21 10:52 ` Maxim Levitsky
  2020-07-21 11:05   ` Damien Le Moal
  2020-07-21 15:13   ` Christoph Hellwig
  2020-07-21 10:52 ` [PATCH 02/10] block: virtio-blk: check logical block size Maxim Levitsky
                   ` (8 subsequent siblings)
  9 siblings, 2 replies; 33+ messages in thread
From: Maxim Levitsky @ 2020-07-21 10:52 UTC (permalink / raw)
  To: linux-kernel
  Cc: Keith Busch, Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg,
	Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Damien Le Moal, Jason Wang, Maxim Levitsky,
	Stefan Hajnoczi, Colin Ian King, Michael S. Tsirkin,
	Paolo Bonzini, Ulf Hansson, Ajay Joshi, Ming Lei,
	open list:SONY MEMORYSTICK SUBSYSTEM, Christoph Hellwig,
	Satya Tangirala, open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov, Maxim Levitsky

Kernel block layer has never supported logical block
sizes less that SECTOR_SIZE nor larger that PAGE_SIZE.

Some drivers have runtime configurable block size,
so it makes sense to have common helper for that.

Signed-off-by: Maxim Levitsky  <mlevitsk@redhat.com>
---
 block/blk-settings.c   | 18 ++++++++++++++++++
 include/linux/blkdev.h |  1 +
 2 files changed, 19 insertions(+)

diff --git a/block/blk-settings.c b/block/blk-settings.c
index 9a2c23cd97007..3c4ef0d00c2bc 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -311,6 +311,21 @@ void blk_queue_max_segment_size(struct request_queue *q, unsigned int max_size)
 }
 EXPORT_SYMBOL(blk_queue_max_segment_size);
 
+
+/**
+ * blk_check_logical_block_size - check if logical block size is supported
+ * by the kernel
+ * @size:  the logical block size, in bytes
+ *
+ * Description:
+ *   This function checks if the block layers supports given block size
+ **/
+bool blk_is_valid_logical_block_size(unsigned int size)
+{
+	return size >= SECTOR_SIZE && size <= PAGE_SIZE && !is_power_of_2(size);
+}
+EXPORT_SYMBOL(blk_is_valid_logical_block_size);
+
 /**
  * blk_queue_logical_block_size - set logical block size for the queue
  * @q:  the request queue for the device
@@ -323,6 +338,8 @@ EXPORT_SYMBOL(blk_queue_max_segment_size);
  **/
 void blk_queue_logical_block_size(struct request_queue *q, unsigned int size)
 {
+	WARN_ON(!blk_is_valid_logical_block_size(size));
+
 	q->limits.logical_block_size = size;
 
 	if (q->limits.physical_block_size < size)
@@ -330,6 +347,7 @@ void blk_queue_logical_block_size(struct request_queue *q, unsigned int size)
 
 	if (q->limits.io_min < q->limits.physical_block_size)
 		q->limits.io_min = q->limits.physical_block_size;
+
 }
 EXPORT_SYMBOL(blk_queue_logical_block_size);
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 57241417ff2f8..2ed3151397e41 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1099,6 +1099,7 @@ extern void blk_queue_max_write_same_sectors(struct request_queue *q,
 		unsigned int max_write_same_sectors);
 extern void blk_queue_max_write_zeroes_sectors(struct request_queue *q,
 		unsigned int max_write_same_sectors);
+extern bool blk_is_valid_logical_block_size(unsigned int size);
 extern void blk_queue_logical_block_size(struct request_queue *, unsigned int);
 extern void blk_queue_max_zone_append_sectors(struct request_queue *q,
 		unsigned int max_zone_append_sectors);
-- 
2.26.2


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 02/10] block: virtio-blk: check logical block size
  2020-07-21 10:52 [PATCH 00/10] RFC: move logical block size checking to the block core Maxim Levitsky
  2020-07-21 10:52 ` [PATCH 01/10] block: introduce blk_is_valid_logical_block_size Maxim Levitsky
@ 2020-07-21 10:52 ` Maxim Levitsky
  2020-07-21 11:07   ` Damien Le Moal
                     ` (2 more replies)
  2020-07-21 10:52 ` [PATCH 03/10] block: loop: use blk_is_valid_logical_block_size Maxim Levitsky
                   ` (7 subsequent siblings)
  9 siblings, 3 replies; 33+ messages in thread
From: Maxim Levitsky @ 2020-07-21 10:52 UTC (permalink / raw)
  To: linux-kernel
  Cc: Keith Busch, Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg,
	Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Damien Le Moal, Jason Wang, Maxim Levitsky,
	Stefan Hajnoczi, Colin Ian King, Michael S. Tsirkin,
	Paolo Bonzini, Ulf Hansson, Ajay Joshi, Ming Lei,
	open list:SONY MEMORYSTICK SUBSYSTEM, Christoph Hellwig,
	Satya Tangirala, open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov, Maxim Levitsky

Linux kernel only supports logical block sizes which are power of two,
at least 512 bytes and no more that PAGE_SIZE.

Check this instead of crashing later on.

Note that there is no need to check physical block size since it is
only a hint, and virtio-blk already only supports power of two values.

Bugzilla link: https://bugzilla.redhat.com/show_bug.cgi?id=1664619

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 drivers/block/virtio_blk.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 980df853ee497..b5ee87cba00ed 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -809,10 +809,18 @@ static int virtblk_probe(struct virtio_device *vdev)
 	err = virtio_cread_feature(vdev, VIRTIO_BLK_F_BLK_SIZE,
 				   struct virtio_blk_config, blk_size,
 				   &blk_size);
-	if (!err)
+	if (!err) {
+		if (!blk_is_valid_logical_block_size(blk_size)) {
+			dev_err(&vdev->dev,
+				"%s failure: invalid logical block size %d\n",
+				__func__, blk_size);
+			err = -EINVAL;
+			goto out_cleanup_queue;
+		}
 		blk_queue_logical_block_size(q, blk_size);
-	else
+	} else {
 		blk_size = queue_logical_block_size(q);
+	}
 
 	/* Use topology information if available */
 	err = virtio_cread_feature(vdev, VIRTIO_BLK_F_TOPOLOGY,
@@ -872,6 +880,9 @@ static int virtblk_probe(struct virtio_device *vdev)
 	device_add_disk(&vdev->dev, vblk->disk, virtblk_attr_groups);
 	return 0;
 
+out_cleanup_queue:
+	blk_cleanup_queue(vblk->disk->queue);
+	vblk->disk->queue = NULL;
 out_free_tags:
 	blk_mq_free_tag_set(&vblk->tag_set);
 out_put_disk:
-- 
2.26.2


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 03/10] block: loop: use blk_is_valid_logical_block_size
  2020-07-21 10:52 [PATCH 00/10] RFC: move logical block size checking to the block core Maxim Levitsky
  2020-07-21 10:52 ` [PATCH 01/10] block: introduce blk_is_valid_logical_block_size Maxim Levitsky
  2020-07-21 10:52 ` [PATCH 02/10] block: virtio-blk: check logical block size Maxim Levitsky
@ 2020-07-21 10:52 ` Maxim Levitsky
  2020-07-21 11:08   ` Damien Le Moal
  2020-07-21 10:52 ` [PATCH 04/10] block: nbd: " Maxim Levitsky
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 33+ messages in thread
From: Maxim Levitsky @ 2020-07-21 10:52 UTC (permalink / raw)
  To: linux-kernel
  Cc: Keith Busch, Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg,
	Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Damien Le Moal, Jason Wang, Maxim Levitsky,
	Stefan Hajnoczi, Colin Ian King, Michael S. Tsirkin,
	Paolo Bonzini, Ulf Hansson, Ajay Joshi, Ming Lei,
	open list:SONY MEMORYSTICK SUBSYSTEM, Christoph Hellwig,
	Satya Tangirala, open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov, Maxim Levitsky

This allows to remove loop's own check for supported block size

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 drivers/block/loop.c | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 475e1a738560d..9984c8f824271 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -228,19 +228,6 @@ static void __loop_update_dio(struct loop_device *lo, bool dio)
 		blk_mq_unfreeze_queue(lo->lo_queue);
 }
 
-/**
- * loop_validate_block_size() - validates the passed in block size
- * @bsize: size to validate
- */
-static int
-loop_validate_block_size(unsigned short bsize)
-{
-	if (bsize < 512 || bsize > PAGE_SIZE || !is_power_of_2(bsize))
-		return -EINVAL;
-
-	return 0;
-}
-
 /**
  * loop_set_size() - sets device size and notifies userspace
  * @lo: struct loop_device to set the size for
@@ -1119,9 +1106,10 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
 	}
 
 	if (config->block_size) {
-		error = loop_validate_block_size(config->block_size);
-		if (error)
+		if (!blk_is_valid_logical_block_size(config->block_size)) {
+			error = -EINVAL;
 			goto out_unlock;
+		}
 	}
 
 	error = loop_set_status_from_info(lo, &config->info);
@@ -1607,9 +1595,8 @@ static int loop_set_block_size(struct loop_device *lo, unsigned long arg)
 	if (lo->lo_state != Lo_bound)
 		return -ENXIO;
 
-	err = loop_validate_block_size(arg);
-	if (err)
-		return err;
+	if (!blk_is_valid_logical_block_size(arg))
+		return -EINVAL;
 
 	if (lo->lo_queue->limits.logical_block_size == arg)
 		return 0;
-- 
2.26.2


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 04/10] block: nbd: use blk_is_valid_logical_block_size
  2020-07-21 10:52 [PATCH 00/10] RFC: move logical block size checking to the block core Maxim Levitsky
                   ` (2 preceding siblings ...)
  2020-07-21 10:52 ` [PATCH 03/10] block: loop: use blk_is_valid_logical_block_size Maxim Levitsky
@ 2020-07-21 10:52 ` Maxim Levitsky
  2020-07-21 11:09   ` Damien Le Moal
  2020-07-21 10:52 ` [PATCH 05/10] block: null: " Maxim Levitsky
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 33+ messages in thread
From: Maxim Levitsky @ 2020-07-21 10:52 UTC (permalink / raw)
  To: linux-kernel
  Cc: Keith Busch, Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg,
	Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Damien Le Moal, Jason Wang, Maxim Levitsky,
	Stefan Hajnoczi, Colin Ian King, Michael S. Tsirkin,
	Paolo Bonzini, Ulf Hansson, Ajay Joshi, Ming Lei,
	open list:SONY MEMORYSTICK SUBSYSTEM, Christoph Hellwig,
	Satya Tangirala, open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov, Maxim Levitsky

This allows to remove nbd's own check for valid block size

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 drivers/block/nbd.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index ce7e9f223b20b..2cd9c4e824f8b 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1347,14 +1347,6 @@ static void nbd_clear_sock_ioctl(struct nbd_device *nbd,
 		nbd_config_put(nbd);
 }
 
-static bool nbd_is_valid_blksize(unsigned long blksize)
-{
-	if (!blksize || !is_power_of_2(blksize) || blksize < 512 ||
-	    blksize > PAGE_SIZE)
-		return false;
-	return true;
-}
-
 static void nbd_set_cmd_timeout(struct nbd_device *nbd, u64 timeout)
 {
 	nbd->tag_set.timeout = timeout * HZ;
@@ -1379,7 +1371,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 	case NBD_SET_BLKSIZE:
 		if (!arg)
 			arg = NBD_DEF_BLKSIZE;
-		if (!nbd_is_valid_blksize(arg))
+		if (!blk_is_valid_logical_block_size(arg))
 			return -EINVAL;
 		nbd_size_set(nbd, arg,
 			     div_s64(config->bytesize, arg));
@@ -1811,7 +1803,7 @@ static int nbd_genl_size_set(struct genl_info *info, struct nbd_device *nbd)
 		bsize = nla_get_u64(info->attrs[NBD_ATTR_BLOCK_SIZE_BYTES]);
 		if (!bsize)
 			bsize = NBD_DEF_BLKSIZE;
-		if (!nbd_is_valid_blksize(bsize)) {
+		if (!blk_is_valid_logical_block_size(bsize)) {
 			printk(KERN_ERR "Invalid block size %llu\n", bsize);
 			return -EINVAL;
 		}
-- 
2.26.2


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 05/10] block: null: use blk_is_valid_logical_block_size
  2020-07-21 10:52 [PATCH 00/10] RFC: move logical block size checking to the block core Maxim Levitsky
                   ` (3 preceding siblings ...)
  2020-07-21 10:52 ` [PATCH 04/10] block: nbd: " Maxim Levitsky
@ 2020-07-21 10:52 ` Maxim Levitsky
  2020-07-21 11:15   ` Damien Le Moal
  2020-07-21 10:52 ` [PATCH 06/10] block: ms_block: " Maxim Levitsky
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 33+ messages in thread
From: Maxim Levitsky @ 2020-07-21 10:52 UTC (permalink / raw)
  To: linux-kernel
  Cc: Keith Busch, Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg,
	Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Damien Le Moal, Jason Wang, Maxim Levitsky,
	Stefan Hajnoczi, Colin Ian King, Michael S. Tsirkin,
	Paolo Bonzini, Ulf Hansson, Ajay Joshi, Ming Lei,
	open list:SONY MEMORYSTICK SUBSYSTEM, Christoph Hellwig,
	Satya Tangirala, open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov, Maxim Levitsky

This slightly changes the behavier of the driver,
when given invalid block size (non power of two, or below 512 bytes),
but shoudn't matter.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 drivers/block/null_blk_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index 87b31f9ca362e..e4df4b903b90b 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -1684,8 +1684,8 @@ static int null_init_tag_set(struct nullb *nullb, struct blk_mq_tag_set *set)
 
 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);
+	if (!blk_is_valid_logical_block_size(dev->blocksize))
+		return -ENODEV;
 
 	if (dev->queue_mode == NULL_Q_MQ && dev->use_per_node_hctx) {
 		if (dev->submit_queues != nr_online_nodes)
@@ -1865,7 +1865,7 @@ static int __init null_init(void)
 	struct nullb *nullb;
 	struct nullb_device *dev;
 
-	if (g_bs > PAGE_SIZE) {
+	if (!blk_is_valid_logical_block_size(g_bs)) {
 		pr_warn("invalid block size\n");
 		pr_warn("defaults block size to %lu\n", PAGE_SIZE);
 		g_bs = PAGE_SIZE;
-- 
2.26.2


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 06/10] block: ms_block: use blk_is_valid_logical_block_size
  2020-07-21 10:52 [PATCH 00/10] RFC: move logical block size checking to the block core Maxim Levitsky
                   ` (4 preceding siblings ...)
  2020-07-21 10:52 ` [PATCH 05/10] block: null: " Maxim Levitsky
@ 2020-07-21 10:52 ` Maxim Levitsky
  2020-07-21 11:17   ` Damien Le Moal
  2020-07-21 10:52 ` [PATCH 07/10] block: mspro_blk: " Maxim Levitsky
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 33+ messages in thread
From: Maxim Levitsky @ 2020-07-21 10:52 UTC (permalink / raw)
  To: linux-kernel
  Cc: Keith Busch, Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg,
	Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Damien Le Moal, Jason Wang, Maxim Levitsky,
	Stefan Hajnoczi, Colin Ian King, Michael S. Tsirkin,
	Paolo Bonzini, Ulf Hansson, Ajay Joshi, Ming Lei,
	open list:SONY MEMORYSTICK SUBSYSTEM, Christoph Hellwig,
	Satya Tangirala, open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov, Maxim Levitsky

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 drivers/memstick/core/ms_block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/memstick/core/ms_block.c b/drivers/memstick/core/ms_block.c
index d9ee8e3dc72da..e4df03e10fb46 100644
--- a/drivers/memstick/core/ms_block.c
+++ b/drivers/memstick/core/ms_block.c
@@ -1727,7 +1727,7 @@ static int msb_init_card(struct memstick_dev *card)
 	msb->pages_in_block = boot_block->attr.block_size * 2;
 	msb->block_size = msb->page_size * msb->pages_in_block;
 
-	if (msb->page_size > PAGE_SIZE) {
+	if (!(blk_is_valid_logical_block_size(msb->page_size))) {
 		/* this isn't supported by linux at all, anyway*/
 		dbg("device page %d size isn't supported", msb->page_size);
 		return -EINVAL;
-- 
2.26.2


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 07/10] block: mspro_blk: use blk_is_valid_logical_block_size
  2020-07-21 10:52 [PATCH 00/10] RFC: move logical block size checking to the block core Maxim Levitsky
                   ` (5 preceding siblings ...)
  2020-07-21 10:52 ` [PATCH 06/10] block: ms_block: " Maxim Levitsky
@ 2020-07-21 10:52 ` Maxim Levitsky
  2020-07-21 11:17   ` Damien Le Moal
  2020-07-21 10:52 ` [PATCH 08/10] block: nvme: " Maxim Levitsky
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 33+ messages in thread
From: Maxim Levitsky @ 2020-07-21 10:52 UTC (permalink / raw)
  To: linux-kernel
  Cc: Keith Busch, Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg,
	Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Damien Le Moal, Jason Wang, Maxim Levitsky,
	Stefan Hajnoczi, Colin Ian King, Michael S. Tsirkin,
	Paolo Bonzini, Ulf Hansson, Ajay Joshi, Ming Lei,
	open list:SONY MEMORYSTICK SUBSYSTEM, Christoph Hellwig,
	Satya Tangirala, open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov, Maxim Levitsky

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 drivers/memstick/core/mspro_block.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/memstick/core/mspro_block.c b/drivers/memstick/core/mspro_block.c
index cd6b8d4f23350..86c9eb0aef512 100644
--- a/drivers/memstick/core/mspro_block.c
+++ b/drivers/memstick/core/mspro_block.c
@@ -1199,6 +1199,12 @@ static int mspro_block_init_disk(struct memstick_dev *card)
 
 	msb->page_size = be16_to_cpu(sys_info->unit_size);
 
+	if (!(blk_is_valid_logical_block_size(msb->page_size))) {
+		dev_warn(&card->dev,
+			 "unsupported block size %d", msb->page_size);
+		return -EINVAL;
+	}
+
 	mutex_lock(&mspro_block_disk_lock);
 	disk_id = idr_alloc(&mspro_block_disk_idr, card, 0, 256, GFP_KERNEL);
 	mutex_unlock(&mspro_block_disk_lock);
-- 
2.26.2


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 08/10] block: nvme: use blk_is_valid_logical_block_size
  2020-07-21 10:52 [PATCH 00/10] RFC: move logical block size checking to the block core Maxim Levitsky
                   ` (6 preceding siblings ...)
  2020-07-21 10:52 ` [PATCH 07/10] block: mspro_blk: " Maxim Levitsky
@ 2020-07-21 10:52 ` Maxim Levitsky
  2020-07-21 11:23   ` Damien Le Moal
  2020-07-21 10:52 ` [PATCH 09/10] block: scsi: sd: " Maxim Levitsky
  2020-07-21 10:52 ` [PATCH 10/10] block: scsi: sr: " Maxim Levitsky
  9 siblings, 1 reply; 33+ messages in thread
From: Maxim Levitsky @ 2020-07-21 10:52 UTC (permalink / raw)
  To: linux-kernel
  Cc: Keith Busch, Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg,
	Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Damien Le Moal, Jason Wang, Maxim Levitsky,
	Stefan Hajnoczi, Colin Ian King, Michael S. Tsirkin,
	Paolo Bonzini, Ulf Hansson, Ajay Joshi, Ming Lei,
	open list:SONY MEMORYSTICK SUBSYSTEM, Christoph Hellwig,
	Satya Tangirala, open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov, Maxim Levitsky

This replaces manual checking in the driver

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 drivers/nvme/host/core.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index add040168e67e..8014b3046992a 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1849,10 +1849,16 @@ static void nvme_update_disk_info(struct gendisk *disk,
 	unsigned short bs = 1 << ns->lba_shift;
 	u32 atomic_bs, phys_bs, io_opt = 0;
 
-	if (ns->lba_shift > PAGE_SHIFT) {
-		/* unsupported block size, set capacity to 0 later */
+	/*
+	 * The block layer can't support LBA sizes larger than the page size
+	 * yet, so catch this early and don't allow block I/O.
+	 */
+
+	if (!blk_is_valid_logical_block_size(bs)) {
 		bs = (1 << 9);
+		capacity = 0;
 	}
+
 	blk_mq_freeze_queue(disk->queue);
 	blk_integrity_unregister(disk);
 
@@ -1887,13 +1893,6 @@ static void nvme_update_disk_info(struct gendisk *disk,
 	blk_queue_io_min(disk->queue, phys_bs);
 	blk_queue_io_opt(disk->queue, io_opt);
 
-	/*
-	 * The block layer can't support LBA sizes larger than the page size
-	 * yet, so catch this early and don't allow block I/O.
-	 */
-	if (ns->lba_shift > PAGE_SHIFT)
-		capacity = 0;
-
 	/*
 	 * Register a metadata profile for PI, or the plain non-integrity NVMe
 	 * metadata masquerading as Type 0 if supported, otherwise reject block
-- 
2.26.2


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 09/10] block: scsi: sd: use blk_is_valid_logical_block_size
  2020-07-21 10:52 [PATCH 00/10] RFC: move logical block size checking to the block core Maxim Levitsky
                   ` (7 preceding siblings ...)
  2020-07-21 10:52 ` [PATCH 08/10] block: nvme: " Maxim Levitsky
@ 2020-07-21 10:52 ` Maxim Levitsky
  2020-07-21 11:25   ` Damien Le Moal
  2020-07-21 10:52 ` [PATCH 10/10] block: scsi: sr: " Maxim Levitsky
  9 siblings, 1 reply; 33+ messages in thread
From: Maxim Levitsky @ 2020-07-21 10:52 UTC (permalink / raw)
  To: linux-kernel
  Cc: Keith Busch, Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg,
	Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Damien Le Moal, Jason Wang, Maxim Levitsky,
	Stefan Hajnoczi, Colin Ian King, Michael S. Tsirkin,
	Paolo Bonzini, Ulf Hansson, Ajay Joshi, Ming Lei,
	open list:SONY MEMORYSTICK SUBSYSTEM, Christoph Hellwig,
	Satya Tangirala, open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov, Maxim Levitsky

Use blk_is_valid_logical_block_size instead of hardcoded list

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 drivers/scsi/sd.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index d90fefffe31b7..f012e7397b058 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2520,10 +2520,7 @@ sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer)
 			  "assuming 512.\n");
 	}
 
-	if (sector_size != 512 &&
-	    sector_size != 1024 &&
-	    sector_size != 2048 &&
-	    sector_size != 4096) {
+	if (!blk_is_valid_logical_block_size(sector_size)) {
 		sd_printk(KERN_NOTICE, sdkp, "Unsupported sector size %d.\n",
 			  sector_size);
 		/*
-- 
2.26.2


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 10/10] block: scsi: sr: use blk_is_valid_logical_block_size
  2020-07-21 10:52 [PATCH 00/10] RFC: move logical block size checking to the block core Maxim Levitsky
                   ` (8 preceding siblings ...)
  2020-07-21 10:52 ` [PATCH 09/10] block: scsi: sd: " Maxim Levitsky
@ 2020-07-21 10:52 ` Maxim Levitsky
  2020-07-21 11:29   ` Damien Le Moal
  9 siblings, 1 reply; 33+ messages in thread
From: Maxim Levitsky @ 2020-07-21 10:52 UTC (permalink / raw)
  To: linux-kernel
  Cc: Keith Busch, Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg,
	Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Damien Le Moal, Jason Wang, Maxim Levitsky,
	Stefan Hajnoczi, Colin Ian King, Michael S. Tsirkin,
	Paolo Bonzini, Ulf Hansson, Ajay Joshi, Ming Lei,
	open list:SONY MEMORYSTICK SUBSYSTEM, Christoph Hellwig,
	Satya Tangirala, open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov, Maxim Levitsky

Plus some tiny refactoring.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 drivers/scsi/sr.c | 31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 0c4aa4665a2f9..0e96338029310 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -866,31 +866,26 @@ static void get_sectorsize(struct scsi_cd *cd)
 			cd->capacity = max_t(long, cd->capacity, last_written);
 
 		sector_size = get_unaligned_be32(&buffer[4]);
-		switch (sector_size) {
-			/*
-			 * HP 4020i CD-Recorder reports 2340 byte sectors
-			 * Philips CD-Writers report 2352 byte sectors
-			 *
-			 * Use 2k sectors for them..
-			 */
-		case 0:
-		case 2340:
-		case 2352:
+
+		/*
+		 * HP 4020i CD-Recorder reports 2340 byte sectors
+		 * Philips CD-Writers report 2352 byte sectors
+		 *
+		 * Use 2k sectors for them..
+		 */
+
+		if (!sector_size || sector_size == 2340 || sector_size == 2352)
 			sector_size = 2048;
-			/* fall through */
-		case 2048:
-			cd->capacity *= 4;
-			/* fall through */
-		case 512:
-			break;
-		default:
+
+		cd->capacity *= (sector_size >> SECTOR_SHIFT);
+
+		if (!blk_is_valid_logical_block_size(sector_size)) {
 			sr_printk(KERN_INFO, cd,
 				  "unsupported sector size %d.", sector_size);
 			cd->capacity = 0;
 		}
 
 		cd->device->sector_size = sector_size;
-
 		/*
 		 * Add this so that we have the ability to correctly gauge
 		 * what the device is capable of.
-- 
2.26.2


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 01/10] block: introduce blk_is_valid_logical_block_size
  2020-07-21 10:52 ` [PATCH 01/10] block: introduce blk_is_valid_logical_block_size Maxim Levitsky
@ 2020-07-21 11:05   ` Damien Le Moal
  2020-07-21 11:09     ` Maxim Levitsky
  2020-07-21 15:13   ` Christoph Hellwig
  1 sibling, 1 reply; 33+ messages in thread
From: Damien Le Moal @ 2020-07-21 11:05 UTC (permalink / raw)
  To: Maxim Levitsky, linux-kernel
  Cc: Keith Busch, Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg,
	Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Jason Wang, Maxim Levitsky, Stefan Hajnoczi,
	Colin Ian King, Michael S. Tsirkin, Paolo Bonzini, Ulf Hansson,
	Ajay Joshi, Ming Lei, open list:SONY MEMORYSTICK SUBSYSTEM,
	Christoph Hellwig, Satya Tangirala,
	open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov

On 2020/07/21 19:53, Maxim Levitsky wrote:
> Kernel block layer has never supported logical block
> sizes less that SECTOR_SIZE nor larger that PAGE_SIZE.
> 
> Some drivers have runtime configurable block size,
> so it makes sense to have common helper for that.

...common helper to check the validity of the logical block size set by the driver.

("for that" does not refer to a clear action)

> 
> Signed-off-by: Maxim Levitsky  <mlevitsk@redhat.com>
> ---
>  block/blk-settings.c   | 18 ++++++++++++++++++
>  include/linux/blkdev.h |  1 +
>  2 files changed, 19 insertions(+)
> 
> diff --git a/block/blk-settings.c b/block/blk-settings.c
> index 9a2c23cd97007..3c4ef0d00c2bc 100644
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -311,6 +311,21 @@ void blk_queue_max_segment_size(struct request_queue *q, unsigned int max_size)
>  }
>  EXPORT_SYMBOL(blk_queue_max_segment_size);
>  
> +
> +/**
> + * blk_check_logical_block_size - check if logical block size is supported
> + * by the kernel
> + * @size:  the logical block size, in bytes
> + *
> + * Description:
> + *   This function checks if the block layers supports given block size
> + **/
> +bool blk_is_valid_logical_block_size(unsigned int size)
> +{
> +	return size >= SECTOR_SIZE && size <= PAGE_SIZE && !is_power_of_2(size);
> +}
> +EXPORT_SYMBOL(blk_is_valid_logical_block_size);
> +
>  /**
>   * blk_queue_logical_block_size - set logical block size for the queue
>   * @q:  the request queue for the device
> @@ -323,6 +338,8 @@ EXPORT_SYMBOL(blk_queue_max_segment_size);
>   **/
>  void blk_queue_logical_block_size(struct request_queue *q, unsigned int size)
>  {
> +	WARN_ON(!blk_is_valid_logical_block_size(size));
> +
>  	q->limits.logical_block_size = size;
>  
>  	if (q->limits.physical_block_size < size)
> @@ -330,6 +347,7 @@ void blk_queue_logical_block_size(struct request_queue *q, unsigned int size)
>  
>  	if (q->limits.io_min < q->limits.physical_block_size)
>  		q->limits.io_min = q->limits.physical_block_size;
> +

white line change.

>  }
>  EXPORT_SYMBOL(blk_queue_logical_block_size);
>  
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 57241417ff2f8..2ed3151397e41 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -1099,6 +1099,7 @@ extern void blk_queue_max_write_same_sectors(struct request_queue *q,
>  		unsigned int max_write_same_sectors);
>  extern void blk_queue_max_write_zeroes_sectors(struct request_queue *q,
>  		unsigned int max_write_same_sectors);
> +extern bool blk_is_valid_logical_block_size(unsigned int size);
>  extern void blk_queue_logical_block_size(struct request_queue *, unsigned int);
>  extern void blk_queue_max_zone_append_sectors(struct request_queue *q,
>  		unsigned int max_zone_append_sectors);
> 


-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 02/10] block: virtio-blk: check logical block size
  2020-07-21 10:52 ` [PATCH 02/10] block: virtio-blk: check logical block size Maxim Levitsky
@ 2020-07-21 11:07   ` Damien Le Moal
  2020-07-21 15:14   ` Christoph Hellwig
  2020-07-28 15:44   ` Stefan Hajnoczi
  2 siblings, 0 replies; 33+ messages in thread
From: Damien Le Moal @ 2020-07-21 11:07 UTC (permalink / raw)
  To: Maxim Levitsky, linux-kernel
  Cc: Keith Busch, Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg,
	Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Jason Wang, Maxim Levitsky, Stefan Hajnoczi,
	Colin Ian King, Michael S. Tsirkin, Paolo Bonzini, Ulf Hansson,
	Ajay Joshi, Ming Lei, open list:SONY MEMORYSTICK SUBSYSTEM,
	Christoph Hellwig, Satya Tangirala,
	open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov

On 2020/07/21 19:54, Maxim Levitsky wrote:
> Linux kernel only supports logical block sizes which are power of two,
> at least 512 bytes and no more that PAGE_SIZE.
> 
> Check this instead of crashing later on.
> 
> Note that there is no need to check physical block size since it is
> only a hint, and virtio-blk already only supports power of two values.
> 
> Bugzilla link: https://bugzilla.redhat.com/show_bug.cgi?id=1664619
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  drivers/block/virtio_blk.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 980df853ee497..b5ee87cba00ed 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -809,10 +809,18 @@ static int virtblk_probe(struct virtio_device *vdev)
>  	err = virtio_cread_feature(vdev, VIRTIO_BLK_F_BLK_SIZE,
>  				   struct virtio_blk_config, blk_size,
>  				   &blk_size);
> -	if (!err)
> +	if (!err) {
> +		if (!blk_is_valid_logical_block_size(blk_size)) {
> +			dev_err(&vdev->dev,
> +				"%s failure: invalid logical block size %d\n",
> +				__func__, blk_size);
> +			err = -EINVAL;
> +			goto out_cleanup_queue;
> +		}
>  		blk_queue_logical_block_size(q, blk_size);
> -	else
> +	} else {
>  		blk_size = queue_logical_block_size(q);
> +	}
>  
>  	/* Use topology information if available */
>  	err = virtio_cread_feature(vdev, VIRTIO_BLK_F_TOPOLOGY,
> @@ -872,6 +880,9 @@ static int virtblk_probe(struct virtio_device *vdev)
>  	device_add_disk(&vdev->dev, vblk->disk, virtblk_attr_groups);
>  	return 0;
>  
> +out_cleanup_queue:
> +	blk_cleanup_queue(vblk->disk->queue);
> +	vblk->disk->queue = NULL;
>  out_free_tags:
>  	blk_mq_free_tag_set(&vblk->tag_set);
>  out_put_disk:
> 

Looks good to me.

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 03/10] block: loop: use blk_is_valid_logical_block_size
  2020-07-21 10:52 ` [PATCH 03/10] block: loop: use blk_is_valid_logical_block_size Maxim Levitsky
@ 2020-07-21 11:08   ` Damien Le Moal
  0 siblings, 0 replies; 33+ messages in thread
From: Damien Le Moal @ 2020-07-21 11:08 UTC (permalink / raw)
  To: Maxim Levitsky, linux-kernel
  Cc: Keith Busch, Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg,
	Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Jason Wang, Maxim Levitsky, Stefan Hajnoczi,
	Colin Ian King, Michael S. Tsirkin, Paolo Bonzini, Ulf Hansson,
	Ajay Joshi, Ming Lei, open list:SONY MEMORYSTICK SUBSYSTEM,
	Christoph Hellwig, Satya Tangirala,
	open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov

On 2020/07/21 19:54, Maxim Levitsky wrote:
> This allows to remove loop's own check for supported block size
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  drivers/block/loop.c | 23 +++++------------------
>  1 file changed, 5 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 475e1a738560d..9984c8f824271 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -228,19 +228,6 @@ static void __loop_update_dio(struct loop_device *lo, bool dio)
>  		blk_mq_unfreeze_queue(lo->lo_queue);
>  }
>  
> -/**
> - * loop_validate_block_size() - validates the passed in block size
> - * @bsize: size to validate
> - */
> -static int
> -loop_validate_block_size(unsigned short bsize)
> -{
> -	if (bsize < 512 || bsize > PAGE_SIZE || !is_power_of_2(bsize))
> -		return -EINVAL;
> -
> -	return 0;
> -}
> -
>  /**
>   * loop_set_size() - sets device size and notifies userspace
>   * @lo: struct loop_device to set the size for
> @@ -1119,9 +1106,10 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
>  	}
>  
>  	if (config->block_size) {
> -		error = loop_validate_block_size(config->block_size);
> -		if (error)
> +		if (!blk_is_valid_logical_block_size(config->block_size)) {
> +			error = -EINVAL;
>  			goto out_unlock;
> +		}
>  	}
>  
>  	error = loop_set_status_from_info(lo, &config->info);
> @@ -1607,9 +1595,8 @@ static int loop_set_block_size(struct loop_device *lo, unsigned long arg)
>  	if (lo->lo_state != Lo_bound)
>  		return -ENXIO;
>  
> -	err = loop_validate_block_size(arg);
> -	if (err)
> -		return err;
> +	if (!blk_is_valid_logical_block_size(arg))
> +		return -EINVAL;
>  
>  	if (lo->lo_queue->limits.logical_block_size == arg)
>  		return 0;
> 

Looks good to me.

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 04/10] block: nbd: use blk_is_valid_logical_block_size
  2020-07-21 10:52 ` [PATCH 04/10] block: nbd: " Maxim Levitsky
@ 2020-07-21 11:09   ` Damien Le Moal
  0 siblings, 0 replies; 33+ messages in thread
From: Damien Le Moal @ 2020-07-21 11:09 UTC (permalink / raw)
  To: Maxim Levitsky, linux-kernel
  Cc: Keith Busch, Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg,
	Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Jason Wang, Maxim Levitsky, Stefan Hajnoczi,
	Colin Ian King, Michael S. Tsirkin, Paolo Bonzini, Ulf Hansson,
	Ajay Joshi, Ming Lei, open list:SONY MEMORYSTICK SUBSYSTEM,
	Christoph Hellwig, Satya Tangirala,
	open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov

On 2020/07/21 19:54, Maxim Levitsky wrote:
> This allows to remove nbd's own check for valid block size
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  drivers/block/nbd.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index ce7e9f223b20b..2cd9c4e824f8b 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -1347,14 +1347,6 @@ static void nbd_clear_sock_ioctl(struct nbd_device *nbd,
>  		nbd_config_put(nbd);
>  }
>  
> -static bool nbd_is_valid_blksize(unsigned long blksize)
> -{
> -	if (!blksize || !is_power_of_2(blksize) || blksize < 512 ||
> -	    blksize > PAGE_SIZE)
> -		return false;
> -	return true;
> -}
> -
>  static void nbd_set_cmd_timeout(struct nbd_device *nbd, u64 timeout)
>  {
>  	nbd->tag_set.timeout = timeout * HZ;
> @@ -1379,7 +1371,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>  	case NBD_SET_BLKSIZE:
>  		if (!arg)
>  			arg = NBD_DEF_BLKSIZE;
> -		if (!nbd_is_valid_blksize(arg))
> +		if (!blk_is_valid_logical_block_size(arg))
>  			return -EINVAL;
>  		nbd_size_set(nbd, arg,
>  			     div_s64(config->bytesize, arg));
> @@ -1811,7 +1803,7 @@ static int nbd_genl_size_set(struct genl_info *info, struct nbd_device *nbd)
>  		bsize = nla_get_u64(info->attrs[NBD_ATTR_BLOCK_SIZE_BYTES]);
>  		if (!bsize)
>  			bsize = NBD_DEF_BLKSIZE;
> -		if (!nbd_is_valid_blksize(bsize)) {
> +		if (!blk_is_valid_logical_block_size(bsize)) {
>  			printk(KERN_ERR "Invalid block size %llu\n", bsize);
>  			return -EINVAL;
>  		}
> 

Looks good to me.

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 01/10] block: introduce blk_is_valid_logical_block_size
  2020-07-21 11:05   ` Damien Le Moal
@ 2020-07-21 11:09     ` Maxim Levitsky
  2020-07-21 11:31       ` Damien Le Moal
  0 siblings, 1 reply; 33+ messages in thread
From: Maxim Levitsky @ 2020-07-21 11:09 UTC (permalink / raw)
  To: Damien Le Moal, linux-kernel
  Cc: Keith Busch, Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg,
	Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Jason Wang, Maxim Levitsky, Stefan Hajnoczi,
	Colin Ian King, Michael S. Tsirkin, Paolo Bonzini, Ulf Hansson,
	Ajay Joshi, Ming Lei, open list:SONY MEMORYSTICK SUBSYSTEM,
	Christoph Hellwig, Satya Tangirala,
	open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov

On Tue, 2020-07-21 at 11:05 +0000, Damien Le Moal wrote:
> On 2020/07/21 19:53, Maxim Levitsky wrote:
> > Kernel block layer has never supported logical block
> > sizes less that SECTOR_SIZE nor larger that PAGE_SIZE.
> > 
> > Some drivers have runtime configurable block size,
> > so it makes sense to have common helper for that.
> 
> ...common helper to check the validity of the logical block size set by the driver.
Agree, will fix.
> 
> ("for that" does not refer to a clear action)
> 
> > Signed-off-by: Maxim Levitsky  <mlevitsk@redhat.com>
> > ---
> >  block/blk-settings.c   | 18 ++++++++++++++++++
> >  include/linux/blkdev.h |  1 +
> >  2 files changed, 19 insertions(+)
> > 
> > diff --git a/block/blk-settings.c b/block/blk-settings.c
> > index 9a2c23cd97007..3c4ef0d00c2bc 100644
> > --- a/block/blk-settings.c
> > +++ b/block/blk-settings.c
> > @@ -311,6 +311,21 @@ void blk_queue_max_segment_size(struct request_queue *q, unsigned int max_size)
> >  }
> >  EXPORT_SYMBOL(blk_queue_max_segment_size);
> >  
> > +
> > +/**
> > + * blk_check_logical_block_size - check if logical block size is supported
> > + * by the kernel
> > + * @size:  the logical block size, in bytes
> > + *
> > + * Description:
> > + *   This function checks if the block layers supports given block size
> > + **/
> > +bool blk_is_valid_logical_block_size(unsigned int size)
> > +{
> > +	return size >= SECTOR_SIZE && size <= PAGE_SIZE && !is_power_of_2(size);
Note here a typo, made in last minute change which I didn't test.
It should be without '!'

Best regards,
	Maxim Levitsky

> > +}
> > +EXPORT_SYMBOL(blk_is_valid_logical_block_size);
> > +
> >  /**
> >   * blk_queue_logical_block_size - set logical block size for the queue
> >   * @q:  the request queue for the device
> > @@ -323,6 +338,8 @@ EXPORT_SYMBOL(blk_queue_max_segment_size);
> >   **/
> >  void blk_queue_logical_block_size(struct request_queue *q, unsigned int size)
> >  {
> > +	WARN_ON(!blk_is_valid_logical_block_size(size));
> > +
> >  	q->limits.logical_block_size = size;
> >  
> >  	if (q->limits.physical_block_size < size)
> > @@ -330,6 +347,7 @@ void blk_queue_logical_block_size(struct request_queue *q, unsigned int size)
> >  
> >  	if (q->limits.io_min < q->limits.physical_block_size)
> >  		q->limits.io_min = q->limits.physical_block_size;
> > +
> 
> white line change.
> 
> >  }
> >  EXPORT_SYMBOL(blk_queue_logical_block_size);
> >  
> > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > index 57241417ff2f8..2ed3151397e41 100644
> > --- a/include/linux/blkdev.h
> > +++ b/include/linux/blkdev.h
> > @@ -1099,6 +1099,7 @@ extern void blk_queue_max_write_same_sectors(struct request_queue *q,
> >  		unsigned int max_write_same_sectors);
> >  extern void blk_queue_max_write_zeroes_sectors(struct request_queue *q,
> >  		unsigned int max_write_same_sectors);
> > +extern bool blk_is_valid_logical_block_size(unsigned int size);
> >  extern void blk_queue_logical_block_size(struct request_queue *, unsigned int);
> >  extern void blk_queue_max_zone_append_sectors(struct request_queue *q,
> >  		unsigned int max_zone_append_sectors);
> > 
> 
> 



^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 05/10] block: null: use blk_is_valid_logical_block_size
  2020-07-21 10:52 ` [PATCH 05/10] block: null: " Maxim Levitsky
@ 2020-07-21 11:15   ` Damien Le Moal
  2020-07-21 12:42     ` Maxim Levitsky
  0 siblings, 1 reply; 33+ messages in thread
From: Damien Le Moal @ 2020-07-21 11:15 UTC (permalink / raw)
  To: Maxim Levitsky, linux-kernel
  Cc: Keith Busch, Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg,
	Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Jason Wang, Maxim Levitsky, Stefan Hajnoczi,
	Colin Ian King, Michael S. Tsirkin, Paolo Bonzini, Ulf Hansson,
	Ajay Joshi, Ming Lei, open list:SONY MEMORYSTICK SUBSYSTEM,
	Christoph Hellwig, Satya Tangirala,
	open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov

On 2020/07/21 19:54, Maxim Levitsky wrote:
> This slightly changes the behavier of the driver,

s/behavier/behavior

> when given invalid block size (non power of two, or below 512 bytes),
> but shoudn't matter.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  drivers/block/null_blk_main.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
> index 87b31f9ca362e..e4df4b903b90b 100644
> --- a/drivers/block/null_blk_main.c
> +++ b/drivers/block/null_blk_main.c
> @@ -1684,8 +1684,8 @@ static int null_init_tag_set(struct nullb *nullb, struct blk_mq_tag_set *set)
>  
>  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);
> +	if (!blk_is_valid_logical_block_size(dev->blocksize))
> +		return -ENODEV;
>  
>  	if (dev->queue_mode == NULL_Q_MQ && dev->use_per_node_hctx) {
>  		if (dev->submit_queues != nr_online_nodes)
> @@ -1865,7 +1865,7 @@ static int __init null_init(void)
>  	struct nullb *nullb;
>  	struct nullb_device *dev;
>  
> -	if (g_bs > PAGE_SIZE) {
> +	if (!blk_is_valid_logical_block_size(g_bs)) {
>  		pr_warn("invalid block size\n");
>  		pr_warn("defaults block size to %lu\n", PAGE_SIZE);
>  		g_bs = PAGE_SIZE;

Not sure if this change is OK. Shouldn't the if here be kept as is and
blk_is_valid_logical_block_size() called after it to check values lower than 4K ?


-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 06/10] block: ms_block: use blk_is_valid_logical_block_size
  2020-07-21 10:52 ` [PATCH 06/10] block: ms_block: " Maxim Levitsky
@ 2020-07-21 11:17   ` Damien Le Moal
  0 siblings, 0 replies; 33+ messages in thread
From: Damien Le Moal @ 2020-07-21 11:17 UTC (permalink / raw)
  To: Maxim Levitsky, linux-kernel
  Cc: Keith Busch, Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg,
	Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Jason Wang, Maxim Levitsky, Stefan Hajnoczi,
	Colin Ian King, Michael S. Tsirkin, Paolo Bonzini, Ulf Hansson,
	Ajay Joshi, Ming Lei, open list:SONY MEMORYSTICK SUBSYSTEM,
	Christoph Hellwig, Satya Tangirala,
	open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov

On 2020/07/21 19:54, Maxim Levitsky wrote:
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  drivers/memstick/core/ms_block.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/memstick/core/ms_block.c b/drivers/memstick/core/ms_block.c
> index d9ee8e3dc72da..e4df03e10fb46 100644
> --- a/drivers/memstick/core/ms_block.c
> +++ b/drivers/memstick/core/ms_block.c
> @@ -1727,7 +1727,7 @@ static int msb_init_card(struct memstick_dev *card)
>  	msb->pages_in_block = boot_block->attr.block_size * 2;
>  	msb->block_size = msb->page_size * msb->pages_in_block;
>  
> -	if (msb->page_size > PAGE_SIZE) {
> +	if (!(blk_is_valid_logical_block_size(msb->page_size))) {
>  		/* this isn't supported by linux at all, anyway*/
>  		dbg("device page %d size isn't supported", msb->page_size);
>  		return -EINVAL;
> 

Looks good to me.

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 07/10] block: mspro_blk: use blk_is_valid_logical_block_size
  2020-07-21 10:52 ` [PATCH 07/10] block: mspro_blk: " Maxim Levitsky
@ 2020-07-21 11:17   ` Damien Le Moal
  0 siblings, 0 replies; 33+ messages in thread
From: Damien Le Moal @ 2020-07-21 11:17 UTC (permalink / raw)
  To: Maxim Levitsky, linux-kernel
  Cc: Keith Busch, Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg,
	Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Jason Wang, Maxim Levitsky, Stefan Hajnoczi,
	Colin Ian King, Michael S. Tsirkin, Paolo Bonzini, Ulf Hansson,
	Ajay Joshi, Ming Lei, open list:SONY MEMORYSTICK SUBSYSTEM,
	Christoph Hellwig, Satya Tangirala,
	open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov

On 2020/07/21 19:55, Maxim Levitsky wrote:
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  drivers/memstick/core/mspro_block.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/memstick/core/mspro_block.c b/drivers/memstick/core/mspro_block.c
> index cd6b8d4f23350..86c9eb0aef512 100644
> --- a/drivers/memstick/core/mspro_block.c
> +++ b/drivers/memstick/core/mspro_block.c
> @@ -1199,6 +1199,12 @@ static int mspro_block_init_disk(struct memstick_dev *card)
>  
>  	msb->page_size = be16_to_cpu(sys_info->unit_size);
>  
> +	if (!(blk_is_valid_logical_block_size(msb->page_size))) {
> +		dev_warn(&card->dev,
> +			 "unsupported block size %d", msb->page_size);
> +		return -EINVAL;
> +	}
> +
>  	mutex_lock(&mspro_block_disk_lock);
>  	disk_id = idr_alloc(&mspro_block_disk_idr, card, 0, 256, GFP_KERNEL);
>  	mutex_unlock(&mspro_block_disk_lock);
> 

Looks good to me.

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 08/10] block: nvme: use blk_is_valid_logical_block_size
  2020-07-21 10:52 ` [PATCH 08/10] block: nvme: " Maxim Levitsky
@ 2020-07-21 11:23   ` Damien Le Moal
  0 siblings, 0 replies; 33+ messages in thread
From: Damien Le Moal @ 2020-07-21 11:23 UTC (permalink / raw)
  To: Maxim Levitsky, linux-kernel
  Cc: Keith Busch, Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg,
	Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Jason Wang, Maxim Levitsky, Stefan Hajnoczi,
	Colin Ian King, Michael S. Tsirkin, Paolo Bonzini, Ulf Hansson,
	Ajay Joshi, Ming Lei, open list:SONY MEMORYSTICK SUBSYSTEM,
	Christoph Hellwig, Satya Tangirala,
	open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov

On 2020/07/21 19:55, Maxim Levitsky wrote:
> This replaces manual checking in the driver
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  drivers/nvme/host/core.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index add040168e67e..8014b3046992a 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1849,10 +1849,16 @@ static void nvme_update_disk_info(struct gendisk *disk,
>  	unsigned short bs = 1 << ns->lba_shift;
>  	u32 atomic_bs, phys_bs, io_opt = 0;
>  
> -	if (ns->lba_shift > PAGE_SHIFT) {
> -		/* unsupported block size, set capacity to 0 later */
> +	/*
> +	 * The block layer can't support LBA sizes larger than the page size
> +	 * yet, so catch this early and don't allow block I/O.
> +	 */
> +

No need for the blank line here.

> +	if (!blk_is_valid_logical_block_size(bs)) {
>  		bs = (1 << 9);
> +		capacity = 0;>  	}
> +
>  	blk_mq_freeze_queue(disk->queue);
>  	blk_integrity_unregister(disk);
>  
> @@ -1887,13 +1893,6 @@ static void nvme_update_disk_info(struct gendisk *disk,
>  	blk_queue_io_min(disk->queue, phys_bs);
>  	blk_queue_io_opt(disk->queue, io_opt);
>  
> -	/*
> -	 * The block layer can't support LBA sizes larger than the page size
> -	 * yet, so catch this early and don't allow block I/O.
> -	 */
> -	if (ns->lba_shift > PAGE_SHIFT)
> -		capacity = 0;
> -
>  	/*
>  	 * Register a metadata profile for PI, or the plain non-integrity NVMe
>  	 * metadata masquerading as Type 0 if supported, otherwise reject block
> 

Apart from the nit above, this looks OK to me.

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 09/10] block: scsi: sd: use blk_is_valid_logical_block_size
  2020-07-21 10:52 ` [PATCH 09/10] block: scsi: sd: " Maxim Levitsky
@ 2020-07-21 11:25   ` Damien Le Moal
  2020-07-21 12:55     ` Maxim Levitsky
  0 siblings, 1 reply; 33+ messages in thread
From: Damien Le Moal @ 2020-07-21 11:25 UTC (permalink / raw)
  To: Maxim Levitsky, linux-kernel
  Cc: Keith Busch, Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg,
	Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Jason Wang, Maxim Levitsky, Stefan Hajnoczi,
	Colin Ian King, Michael S. Tsirkin, Paolo Bonzini, Ulf Hansson,
	Ajay Joshi, Ming Lei, open list:SONY MEMORYSTICK SUBSYSTEM,
	Christoph Hellwig, Satya Tangirala,
	open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov

On 2020/07/21 19:55, Maxim Levitsky wrote:
> Use blk_is_valid_logical_block_size instead of hardcoded list

s/hardcoded list/hardcoded checks./

> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  drivers/scsi/sd.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index d90fefffe31b7..f012e7397b058 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2520,10 +2520,7 @@ sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer)
>  			  "assuming 512.\n");
>  	}
>  
> -	if (sector_size != 512 &&
> -	    sector_size != 1024 &&
> -	    sector_size != 2048 &&
> -	    sector_size != 4096) {
> +	if (!blk_is_valid_logical_block_size(sector_size)) {
>  		sd_printk(KERN_NOTICE, sdkp, "Unsupported sector size %d.\n",
>  			  sector_size);
>  		/*
> 

With the commit message fixed, looks OK.

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 10/10] block: scsi: sr: use blk_is_valid_logical_block_size
  2020-07-21 10:52 ` [PATCH 10/10] block: scsi: sr: " Maxim Levitsky
@ 2020-07-21 11:29   ` Damien Le Moal
  0 siblings, 0 replies; 33+ messages in thread
From: Damien Le Moal @ 2020-07-21 11:29 UTC (permalink / raw)
  To: Maxim Levitsky, linux-kernel
  Cc: Keith Busch, Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg,
	Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Jason Wang, Maxim Levitsky, Stefan Hajnoczi,
	Colin Ian King, Michael S. Tsirkin, Paolo Bonzini, Ulf Hansson,
	Ajay Joshi, Ming Lei, open list:SONY MEMORYSTICK SUBSYSTEM,
	Christoph Hellwig, Satya Tangirala,
	open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov

On 2020/07/21 19:55, Maxim Levitsky wrote:
> Plus some tiny refactoring.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  drivers/scsi/sr.c | 31 +++++++++++++------------------
>  1 file changed, 13 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 0c4aa4665a2f9..0e96338029310 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -866,31 +866,26 @@ static void get_sectorsize(struct scsi_cd *cd)
>  			cd->capacity = max_t(long, cd->capacity, last_written);
>  
>  		sector_size = get_unaligned_be32(&buffer[4]);
> -		switch (sector_size) {
> -			/*
> -			 * HP 4020i CD-Recorder reports 2340 byte sectors
> -			 * Philips CD-Writers report 2352 byte sectors
> -			 *
> -			 * Use 2k sectors for them..
> -			 */
> -		case 0:
> -		case 2340:
> -		case 2352:
> +
> +		/*
> +		 * HP 4020i CD-Recorder reports 2340 byte sectors
> +		 * Philips CD-Writers report 2352 byte sectors
> +		 *
> +		 * Use 2k sectors for them..
> +		 */
> +

No need for the blank line here.

> +		if (!sector_size || sector_size == 2340 || sector_size == 2352)
>  			sector_size = 2048;
> -			/* fall through */
> -		case 2048:
> -			cd->capacity *= 4;
> -			/* fall through */
> -		case 512:
> -			break;
> -		default:
> +
> +		cd->capacity *= (sector_size >> SECTOR_SHIFT);

Where does this come from ? There is no such code in sr get_sectorsize()...

> +
> +		if (!blk_is_valid_logical_block_size(sector_size)) {
>  			sr_printk(KERN_INFO, cd,
>  				  "unsupported sector size %d.", sector_size);
>  			cd->capacity = 0;
>  		}
>  
>  		cd->device->sector_size = sector_size;
> -

White line change.

>  		/*
>  		 * Add this so that we have the ability to correctly gauge
>  		 * what the device is capable of.
> 


-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 01/10] block: introduce blk_is_valid_logical_block_size
  2020-07-21 11:09     ` Maxim Levitsky
@ 2020-07-21 11:31       ` Damien Le Moal
  0 siblings, 0 replies; 33+ messages in thread
From: Damien Le Moal @ 2020-07-21 11:31 UTC (permalink / raw)
  To: Maxim Levitsky, linux-kernel
  Cc: Keith Busch, Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg,
	Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Jason Wang, Maxim Levitsky, Stefan Hajnoczi,
	Colin Ian King, Michael S. Tsirkin, Paolo Bonzini, Ulf Hansson,
	Ajay Joshi, Ming Lei, open list:SONY MEMORYSTICK SUBSYSTEM,
	Christoph Hellwig, Satya Tangirala,
	open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov

On 2020/07/21 20:09, Maxim Levitsky wrote:
> On Tue, 2020-07-21 at 11:05 +0000, Damien Le Moal wrote:
>> On 2020/07/21 19:53, Maxim Levitsky wrote:
>>> Kernel block layer has never supported logical block
>>> sizes less that SECTOR_SIZE nor larger that PAGE_SIZE.
>>>
>>> Some drivers have runtime configurable block size,
>>> so it makes sense to have common helper for that.
>>
>> ...common helper to check the validity of the logical block size set by the driver.
> Agree, will fix.
>>
>> ("for that" does not refer to a clear action)
>>
>>> Signed-off-by: Maxim Levitsky  <mlevitsk@redhat.com>
>>> ---
>>>  block/blk-settings.c   | 18 ++++++++++++++++++
>>>  include/linux/blkdev.h |  1 +
>>>  2 files changed, 19 insertions(+)
>>>
>>> diff --git a/block/blk-settings.c b/block/blk-settings.c
>>> index 9a2c23cd97007..3c4ef0d00c2bc 100644
>>> --- a/block/blk-settings.c
>>> +++ b/block/blk-settings.c
>>> @@ -311,6 +311,21 @@ void blk_queue_max_segment_size(struct request_queue *q, unsigned int max_size)
>>>  }
>>>  EXPORT_SYMBOL(blk_queue_max_segment_size);
>>>  
>>> +
>>> +/**
>>> + * blk_check_logical_block_size - check if logical block size is supported
>>> + * by the kernel
>>> + * @size:  the logical block size, in bytes
>>> + *
>>> + * Description:
>>> + *   This function checks if the block layers supports given block size
>>> + **/
>>> +bool blk_is_valid_logical_block_size(unsigned int size)
>>> +{
>>> +	return size >= SECTOR_SIZE && size <= PAGE_SIZE && !is_power_of_2(size);
> Note here a typo, made in last minute change which I didn't test.
> It should be without '!'

Oh ! Indeed. I missed that.

> 
> Best regards,
> 	Maxim Levitsky
> 
>>> +}
>>> +EXPORT_SYMBOL(blk_is_valid_logical_block_size);
>>> +
>>>  /**
>>>   * blk_queue_logical_block_size - set logical block size for the queue
>>>   * @q:  the request queue for the device
>>> @@ -323,6 +338,8 @@ EXPORT_SYMBOL(blk_queue_max_segment_size);
>>>   **/
>>>  void blk_queue_logical_block_size(struct request_queue *q, unsigned int size)
>>>  {
>>> +	WARN_ON(!blk_is_valid_logical_block_size(size));
>>> +
>>>  	q->limits.logical_block_size = size;
>>>  
>>>  	if (q->limits.physical_block_size < size)
>>> @@ -330,6 +347,7 @@ void blk_queue_logical_block_size(struct request_queue *q, unsigned int size)
>>>  
>>>  	if (q->limits.io_min < q->limits.physical_block_size)
>>>  		q->limits.io_min = q->limits.physical_block_size;
>>> +
>>
>> white line change.
>>
>>>  }
>>>  EXPORT_SYMBOL(blk_queue_logical_block_size);
>>>  
>>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>>> index 57241417ff2f8..2ed3151397e41 100644
>>> --- a/include/linux/blkdev.h
>>> +++ b/include/linux/blkdev.h
>>> @@ -1099,6 +1099,7 @@ extern void blk_queue_max_write_same_sectors(struct request_queue *q,
>>>  		unsigned int max_write_same_sectors);
>>>  extern void blk_queue_max_write_zeroes_sectors(struct request_queue *q,
>>>  		unsigned int max_write_same_sectors);
>>> +extern bool blk_is_valid_logical_block_size(unsigned int size);
>>>  extern void blk_queue_logical_block_size(struct request_queue *, unsigned int);
>>>  extern void blk_queue_max_zone_append_sectors(struct request_queue *q,
>>>  		unsigned int max_zone_append_sectors);
>>>
>>
>>
> 
> 
> 


-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 05/10] block: null: use blk_is_valid_logical_block_size
  2020-07-21 11:15   ` Damien Le Moal
@ 2020-07-21 12:42     ` Maxim Levitsky
  0 siblings, 0 replies; 33+ messages in thread
From: Maxim Levitsky @ 2020-07-21 12:42 UTC (permalink / raw)
  To: Damien Le Moal, linux-kernel
  Cc: Keith Busch, Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg,
	Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Jason Wang, Maxim Levitsky, Stefan Hajnoczi,
	Colin Ian King, Michael S. Tsirkin, Paolo Bonzini, Ulf Hansson,
	Ajay Joshi, Ming Lei, open list:SONY MEMORYSTICK SUBSYSTEM,
	Christoph Hellwig, Satya Tangirala,
	open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov

On Tue, 2020-07-21 at 11:15 +0000, Damien Le Moal wrote:
> On 2020/07/21 19:54, Maxim Levitsky wrote:
> > This slightly changes the behavier of the driver,
> 
> s/behavier/behavior
Thanks. This is one of the typos I make very consistently.

> 
> > when given invalid block size (non power of two, or below 512 bytes),
> > but shoudn't matter.
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  drivers/block/null_blk_main.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
> > index 87b31f9ca362e..e4df4b903b90b 100644
> > --- a/drivers/block/null_blk_main.c
> > +++ b/drivers/block/null_blk_main.c
> > @@ -1684,8 +1684,8 @@ static int null_init_tag_set(struct nullb *nullb, struct blk_mq_tag_set *set)
> >  
> >  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);
> > +	if (!blk_is_valid_logical_block_size(dev->blocksize))
> > +		return -ENODEV;
> >  
> >  	if (dev->queue_mode == NULL_Q_MQ && dev->use_per_node_hctx) {
> >  		if (dev->submit_queues != nr_online_nodes)
> > @@ -1865,7 +1865,7 @@ static int __init null_init(void)
> >  	struct nullb *nullb;
> >  	struct nullb_device *dev;
> >  
> > -	if (g_bs > PAGE_SIZE) {
> > +	if (!blk_is_valid_logical_block_size(g_bs)) {
> >  		pr_warn("invalid block size\n");
> >  		pr_warn("defaults block size to %lu\n", PAGE_SIZE);
> >  		g_bs = PAGE_SIZE;
> 
> Not sure if this change is OK. Shouldn't the if here be kept as is and
> blk_is_valid_logical_block_size() called after it to check values lower than 4K ?

The thing is that this driver supports two ways of creating the block devices.
On module load it creates by default a single block device and it uses g_bs
as its block size, but then it also has configfs based interface that allows
to create more block devices.

The default is taken also from g_bs but then user can change it (
via those NULLB_DEVICE_ATTR wrappers)
I changed the behavior slightly, that now if user supplies bad value,
it will be rejected instead of finding closest valid value.

In addition to that there is very small bug that didn't bother to fix in
this series (but I will in next one). The bug is that when null_validate_conf
fails, it return value is overriden with -ENOMEM, which gives the user a wrong
error message.

Thanks for the review!

Best regards,
	Maxim Levitsky

> 
> 



^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 09/10] block: scsi: sd: use blk_is_valid_logical_block_size
  2020-07-21 11:25   ` Damien Le Moal
@ 2020-07-21 12:55     ` Maxim Levitsky
  0 siblings, 0 replies; 33+ messages in thread
From: Maxim Levitsky @ 2020-07-21 12:55 UTC (permalink / raw)
  To: Damien Le Moal, linux-kernel
  Cc: Keith Busch, Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg,
	Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Jason Wang, Maxim Levitsky, Stefan Hajnoczi,
	Colin Ian King, Michael S. Tsirkin, Paolo Bonzini, Ulf Hansson,
	Ajay Joshi, Ming Lei, open list:SONY MEMORYSTICK SUBSYSTEM,
	Christoph Hellwig, Satya Tangirala,
	open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov

On Tue, 2020-07-21 at 11:25 +0000, Damien Le Moal wrote:
> On 2020/07/21 19:55, Maxim Levitsky wrote:
> > Use blk_is_valid_logical_block_size instead of hardcoded list
> 
> s/hardcoded list/hardcoded checks./
Done, thanks!

Best regards,
	Maxim Levitsky
> 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  drivers/scsi/sd.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index d90fefffe31b7..f012e7397b058 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -2520,10 +2520,7 @@ sd_read_capacity(struct scsi_disk *sdkp,
> > unsigned char *buffer)
> >  			  "assuming 512.\n");
> >  	}
> >  
> > -	if (sector_size != 512 &&
> > -	    sector_size != 1024 &&
> > -	    sector_size != 2048 &&
> > -	    sector_size != 4096) {
> > +	if (!blk_is_valid_logical_block_size(sector_size)) {
> >  		sd_printk(KERN_NOTICE, sdkp, "Unsupported sector size
> > %d.\n",
> >  			  sector_size);
> >  		/*
> > 
> 
> With the commit message fixed, looks OK.
> 
> Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
> 


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 01/10] block: introduce blk_is_valid_logical_block_size
  2020-07-21 10:52 ` [PATCH 01/10] block: introduce blk_is_valid_logical_block_size Maxim Levitsky
  2020-07-21 11:05   ` Damien Le Moal
@ 2020-07-21 15:13   ` Christoph Hellwig
  2020-07-22  8:50     ` Maxim Levitsky
  1 sibling, 1 reply; 33+ messages in thread
From: Christoph Hellwig @ 2020-07-21 15:13 UTC (permalink / raw)
  To: Maxim Levitsky
  Cc: linux-kernel, Keith Busch, Josef Bacik, open list:BLOCK LAYER,
	Sagi Grimberg, Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Damien Le Moal, Jason Wang, Maxim Levitsky,
	Stefan Hajnoczi, Colin Ian King, Michael S. Tsirkin,
	Paolo Bonzini, Ulf Hansson, Ajay Joshi, Ming Lei,
	open list:SONY MEMORYSTICK SUBSYSTEM, Christoph Hellwig,
	Satya Tangirala, open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov

> +/**
> + * blk_check_logical_block_size - check if logical block size is supported
> + * by the kernel
> + * @size:  the logical block size, in bytes
> + *
> + * Description:
> + *   This function checks if the block layers supports given block size
> + **/
> +bool blk_is_valid_logical_block_size(unsigned int size)
> +{
> +	return size >= SECTOR_SIZE && size <= PAGE_SIZE && !is_power_of_2(size);

Shouldn't this be a ... && is_power_of_2(size)?

>  	if (q->limits.io_min < q->limits.physical_block_size)
>  		q->limits.io_min = q->limits.physical_block_size;
> +
>  }

This adds a pointless empty line.

> +extern bool blk_is_valid_logical_block_size(unsigned int size);

No need for externs on function declarations.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 02/10] block: virtio-blk: check logical block size
  2020-07-21 10:52 ` [PATCH 02/10] block: virtio-blk: check logical block size Maxim Levitsky
  2020-07-21 11:07   ` Damien Le Moal
@ 2020-07-21 15:14   ` Christoph Hellwig
  2020-07-22  2:55     ` Martin K. Petersen
  2020-07-28 15:44   ` Stefan Hajnoczi
  2 siblings, 1 reply; 33+ messages in thread
From: Christoph Hellwig @ 2020-07-21 15:14 UTC (permalink / raw)
  To: Maxim Levitsky
  Cc: linux-kernel, Keith Busch, Josef Bacik, open list:BLOCK LAYER,
	Sagi Grimberg, Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Damien Le Moal, Jason Wang, Maxim Levitsky,
	Stefan Hajnoczi, Colin Ian King, Michael S. Tsirkin,
	Paolo Bonzini, Ulf Hansson, Ajay Joshi, Ming Lei,
	open list:SONY MEMORYSTICK SUBSYSTEM, Christoph Hellwig,
	Satya Tangirala, open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov

On Tue, Jul 21, 2020 at 01:52:31PM +0300, Maxim Levitsky wrote:
> Linux kernel only supports logical block sizes which are power of two,
> at least 512 bytes and no more that PAGE_SIZE.
> 
> Check this instead of crashing later on.
> 
> Note that there is no need to check physical block size since it is
> only a hint, and virtio-blk already only supports power of two values.
> 
> Bugzilla link: https://bugzilla.redhat.com/show_bug.cgi?id=1664619
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  drivers/block/virtio_blk.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 980df853ee497..b5ee87cba00ed 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -809,10 +809,18 @@ static int virtblk_probe(struct virtio_device *vdev)
>  	err = virtio_cread_feature(vdev, VIRTIO_BLK_F_BLK_SIZE,
>  				   struct virtio_blk_config, blk_size,
>  				   &blk_size);
> -	if (!err)
> +	if (!err) {
> +		if (!blk_is_valid_logical_block_size(blk_size)) {
> +			dev_err(&vdev->dev,
> +				"%s failure: invalid logical block size %d\n",
> +				__func__, blk_size);
> +			err = -EINVAL;
> +			goto out_cleanup_queue;
> +		}
>  		blk_queue_logical_block_size(q, blk_size);

Hmm, I wonder if we should simply add the check and warning to
blk_queue_logical_block_size and add an error in that case.  Then
drivers only have to check the error return, which might add a lot
less boiler plate code.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 02/10] block: virtio-blk: check logical block size
  2020-07-21 15:14   ` Christoph Hellwig
@ 2020-07-22  2:55     ` Martin K. Petersen
  2020-07-22  9:11       ` Maxim Levitsky
  0 siblings, 1 reply; 33+ messages in thread
From: Martin K. Petersen @ 2020-07-22  2:55 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Maxim Levitsky, linux-kernel, Keith Busch, Josef Bacik,
	open list:BLOCK LAYER, Sagi Grimberg, Jens Axboe,
	open list:NVM EXPRESS DRIVER, open list:SCSI CDROM DRIVER,
	Tejun Heo, Bart Van Assche, Martin K. Petersen, Damien Le Moal,
	Jason Wang, Maxim Levitsky, Stefan Hajnoczi, Colin Ian King,
	Michael S. Tsirkin, Paolo Bonzini, Ulf Hansson, Ajay Joshi,
	Ming Lei, open list:SONY MEMORYSTICK SUBSYSTEM, Satya Tangirala,
	open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov


Christoph,

> Hmm, I wonder if we should simply add the check and warning to
> blk_queue_logical_block_size and add an error in that case.  Then
> drivers only have to check the error return, which might add a lot
> less boiler plate code.

Yep, I agree.

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 01/10] block: introduce blk_is_valid_logical_block_size
  2020-07-21 15:13   ` Christoph Hellwig
@ 2020-07-22  8:50     ` Maxim Levitsky
  0 siblings, 0 replies; 33+ messages in thread
From: Maxim Levitsky @ 2020-07-22  8:50 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-kernel, Keith Busch, Josef Bacik, open list:BLOCK LAYER,
	Sagi Grimberg, Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Damien Le Moal, Jason Wang, Maxim Levitsky,
	Stefan Hajnoczi, Colin Ian King, Michael S. Tsirkin,
	Paolo Bonzini, Ulf Hansson, Ajay Joshi, Ming Lei,
	open list:SONY MEMORYSTICK SUBSYSTEM, Satya Tangirala,
	open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov

On Tue, 2020-07-21 at 17:13 +0200, Christoph Hellwig wrote:
> > +/**
> > + * blk_check_logical_block_size - check if logical block size is
> > supported
> > + * by the kernel
> > + * @size:  the logical block size, in bytes
> > + *
> > + * Description:
> > + *   This function checks if the block layers supports given block
> > size
> > + **/
> > +bool blk_is_valid_logical_block_size(unsigned int size)
> > +{
> > +	return size >= SECTOR_SIZE && size <= PAGE_SIZE &&
> > !is_power_of_2(size);
> 
> Shouldn't this be a ... && is_power_of_2(size)?
Yep. I noticed that few minutes after I sent the patches.

> 
> >  	if (q->limits.io_min < q->limits.physical_block_size)
> >  		q->limits.io_min = q->limits.physical_block_size;
> > +
> >  }
> 
> This adds a pointless empty line.
Will fix.
> 
> > +extern bool blk_is_valid_logical_block_size(unsigned int size);
> 
> No need for externs on function declarations.
I also think so, but I followed the style of all existing function
prototypes in this file. Most of them have 'extern'.

Thanks for the review!

Best regards,
	maxim Levitsky


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 02/10] block: virtio-blk: check logical block size
  2020-07-22  2:55     ` Martin K. Petersen
@ 2020-07-22  9:11       ` Maxim Levitsky
  2020-07-28 14:27         ` Maxim Levitsky
  2020-07-29  4:32         ` Martin K. Petersen
  0 siblings, 2 replies; 33+ messages in thread
From: Maxim Levitsky @ 2020-07-22  9:11 UTC (permalink / raw)
  To: Martin K. Petersen, Christoph Hellwig
  Cc: linux-kernel, Keith Busch, Josef Bacik, open list:BLOCK LAYER,
	Sagi Grimberg, Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Damien Le Moal, Jason Wang, Maxim Levitsky, Stefan Hajnoczi,
	Colin Ian King, Michael S. Tsirkin, Paolo Bonzini, Ulf Hansson,
	Ajay Joshi, Ming Lei, open list:SONY MEMORYSTICK SUBSYSTEM,
	Satya Tangirala, open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov

On Tue, 2020-07-21 at 22:55 -0400, Martin K. Petersen wrote:
> Christoph,
> 
> > Hmm, I wonder if we should simply add the check and warning to
> > blk_queue_logical_block_size and add an error in that case.  Then
> > drivers only have to check the error return, which might add a lot
> > less boiler plate code.
> 
> Yep, I agree.
> 

I also agree that this would be cleaner (I actually tried to implement
this the way you suggest), but let me explain my reasoning for doing it
this way.

The problem is that most current users of blk_queue_logical_block_size
(43 uses in the tree, out of which only 9 use constant block size) check
for the block size relatively early, often store it in some internal
struct etc, prior to calling blk_queue_logical_block_size thus making
them only to rely on blk_queue_logical_block_size as the check for 
block size validity will need non-trivial changes in their code.

Instead of this adding blk_is_valid_logical_block_size allowed me
to trivially convert most of the uses.

For RFC I converted only some drivers that I am more familiar with
and/or can test but I can remove the driver's own checks in most other
drivers with low chance of introducing a bug, even if I can't test the
driver.

What do you think?

I can also both make blk_queue_logical_block_size return an error value,
and have blk_is_valid_logical_block_size and use either of these checks,
depending on the driver with eventual goal of un-exporting
blk_is_valid_logical_block_size.

Also note that I did add WARN_ON to blk_queue_logical_block_size.

Best regards,
	Maxim Levitsky


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 02/10] block: virtio-blk: check logical block size
  2020-07-22  9:11       ` Maxim Levitsky
@ 2020-07-28 14:27         ` Maxim Levitsky
  2020-07-29  4:32         ` Martin K. Petersen
  1 sibling, 0 replies; 33+ messages in thread
From: Maxim Levitsky @ 2020-07-28 14:27 UTC (permalink / raw)
  To: Martin K. Petersen, Christoph Hellwig
  Cc: linux-kernel, Keith Busch, Josef Bacik, open list:BLOCK LAYER,
	Sagi Grimberg, Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Damien Le Moal, Jason Wang, Maxim Levitsky, Stefan Hajnoczi,
	Colin Ian King, Michael S. Tsirkin, Paolo Bonzini, Ulf Hansson,
	Ajay Joshi, Ming Lei, open list:SONY MEMORYSTICK SUBSYSTEM,
	Satya Tangirala, open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov

On Wed, 2020-07-22 at 12:11 +0300, Maxim Levitsky wrote:
> On Tue, 2020-07-21 at 22:55 -0400, Martin K. Petersen wrote:
> > Christoph,
> > 
> > > Hmm, I wonder if we should simply add the check and warning to
> > > blk_queue_logical_block_size and add an error in that case.  Then
> > > drivers only have to check the error return, which might add a lot
> > > less boiler plate code.
> > 
> > Yep, I agree.
> > 
> 
> I also agree that this would be cleaner (I actually tried to implement
> this the way you suggest), but let me explain my reasoning for doing
> it
> this way.
> 
> The problem is that most current users of blk_queue_logical_block_size
> (43 uses in the tree, out of which only 9 use constant block size)
> check
> for the block size relatively early, often store it in some internal
> struct etc, prior to calling blk_queue_logical_block_size thus making
> them only to rely on blk_queue_logical_block_size as the check for 
> block size validity will need non-trivial changes in their code.
> 
> Instead of this adding blk_is_valid_logical_block_size allowed me
> to trivially convert most of the uses.
> 
> For RFC I converted only some drivers that I am more familiar with
> and/or can test but I can remove the driver's own checks in most other
> drivers with low chance of introducing a bug, even if I can't test the
> driver.
> 
> What do you think?
> 
> I can also both make blk_queue_logical_block_size return an error
> value,
> and have blk_is_valid_logical_block_size and use either of these
> checks,
> depending on the driver with eventual goal of un-exporting
> blk_is_valid_logical_block_size.
> 
> Also note that I did add WARN_ON to blk_queue_logical_block_size.

Any update on this?

Best regards,
	Maxim Levitsky

> 
> Best regards,
> 	Maxim Levitsky


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 02/10] block: virtio-blk: check logical block size
  2020-07-21 10:52 ` [PATCH 02/10] block: virtio-blk: check logical block size Maxim Levitsky
  2020-07-21 11:07   ` Damien Le Moal
  2020-07-21 15:14   ` Christoph Hellwig
@ 2020-07-28 15:44   ` Stefan Hajnoczi
  2 siblings, 0 replies; 33+ messages in thread
From: Stefan Hajnoczi @ 2020-07-28 15:44 UTC (permalink / raw)
  To: Maxim Levitsky
  Cc: linux-kernel, Keith Busch, Josef Bacik, open list:BLOCK LAYER,
	Sagi Grimberg, Jens Axboe, open list:NVM EXPRESS DRIVER,
	open list:SCSI CDROM DRIVER, Tejun Heo, Bart Van Assche,
	Martin K. Petersen, Damien Le Moal, Jason Wang, Maxim Levitsky,
	Colin Ian King, Michael S. Tsirkin, Paolo Bonzini, Ulf Hansson,
	Ajay Joshi, Ming Lei, open list:SONY MEMORYSTICK SUBSYSTEM,
	Christoph Hellwig, Satya Tangirala,
	open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov


[-- Attachment #1: Type: text/plain, Size: 685 bytes --]

On Tue, Jul 21, 2020 at 01:52:31PM +0300, Maxim Levitsky wrote:
> Linux kernel only supports logical block sizes which are power of two,
> at least 512 bytes and no more that PAGE_SIZE.
> 
> Check this instead of crashing later on.
> 
> Note that there is no need to check physical block size since it is
> only a hint, and virtio-blk already only supports power of two values.
> 
> Bugzilla link: https://bugzilla.redhat.com/show_bug.cgi?id=1664619
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  drivers/block/virtio_blk.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 02/10] block: virtio-blk: check logical block size
  2020-07-22  9:11       ` Maxim Levitsky
  2020-07-28 14:27         ` Maxim Levitsky
@ 2020-07-29  4:32         ` Martin K. Petersen
  1 sibling, 0 replies; 33+ messages in thread
From: Martin K. Petersen @ 2020-07-29  4:32 UTC (permalink / raw)
  To: Maxim Levitsky
  Cc: Martin K. Petersen, Christoph Hellwig, linux-kernel, Keith Busch,
	Josef Bacik, open list:BLOCK LAYER, Sagi Grimberg, Jens Axboe,
	open list:NVM EXPRESS DRIVER, open list:SCSI CDROM DRIVER,
	Tejun Heo, Bart Van Assche, Damien Le Moal, Jason Wang,
	Maxim Levitsky, Stefan Hajnoczi, Colin Ian King,
	Michael S. Tsirkin, Paolo Bonzini, Ulf Hansson, Ajay Joshi,
	Ming Lei, open list:SONY MEMORYSTICK SUBSYSTEM, Satya Tangirala,
	open list:NETWORK BLOCK DEVICE (NBD),
	Hou Tao, Jens Axboe, open list:VIRTIO CORE AND NET DRIVERS,
	James E.J. Bottomley, Alex Dubov


Hi Maxim,

> Instead of this adding blk_is_valid_logical_block_size allowed me to
> trivially convert most of the uses.
>
> For RFC I converted only some drivers that I am more familiar with
> and/or can test but I can remove the driver's own checks in most other
> drivers with low chance of introducing a bug, even if I can't test the
> driver.
>
> What do you think?
>
> I can also both make blk_queue_logical_block_size return an error
> value, and have blk_is_valid_logical_block_size and use either of
> these checks, depending on the driver with eventual goal of
> un-exporting blk_is_valid_logical_block_size.

My concern is that blk_is_valid_logical_block_size() deviates from the
regular block layer convention where the function to set a given queue
limit will either adjust the passed value or print a warning.

I guess I won't entirely object to having the actual check in a helper
function that drivers with a peculiar initialization pattern can
use. And then make blk_queue_logical_block_size() call that helper as
well to validate the lbs.

But I do think that blk_queue_logical_block_size() should be the
preferred interface and that, where possible, drivers should be updated
to check the return value of that.

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 33+ messages in thread

end of thread, back to index

Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-21 10:52 [PATCH 00/10] RFC: move logical block size checking to the block core Maxim Levitsky
2020-07-21 10:52 ` [PATCH 01/10] block: introduce blk_is_valid_logical_block_size Maxim Levitsky
2020-07-21 11:05   ` Damien Le Moal
2020-07-21 11:09     ` Maxim Levitsky
2020-07-21 11:31       ` Damien Le Moal
2020-07-21 15:13   ` Christoph Hellwig
2020-07-22  8:50     ` Maxim Levitsky
2020-07-21 10:52 ` [PATCH 02/10] block: virtio-blk: check logical block size Maxim Levitsky
2020-07-21 11:07   ` Damien Le Moal
2020-07-21 15:14   ` Christoph Hellwig
2020-07-22  2:55     ` Martin K. Petersen
2020-07-22  9:11       ` Maxim Levitsky
2020-07-28 14:27         ` Maxim Levitsky
2020-07-29  4:32         ` Martin K. Petersen
2020-07-28 15:44   ` Stefan Hajnoczi
2020-07-21 10:52 ` [PATCH 03/10] block: loop: use blk_is_valid_logical_block_size Maxim Levitsky
2020-07-21 11:08   ` Damien Le Moal
2020-07-21 10:52 ` [PATCH 04/10] block: nbd: " Maxim Levitsky
2020-07-21 11:09   ` Damien Le Moal
2020-07-21 10:52 ` [PATCH 05/10] block: null: " Maxim Levitsky
2020-07-21 11:15   ` Damien Le Moal
2020-07-21 12:42     ` Maxim Levitsky
2020-07-21 10:52 ` [PATCH 06/10] block: ms_block: " Maxim Levitsky
2020-07-21 11:17   ` Damien Le Moal
2020-07-21 10:52 ` [PATCH 07/10] block: mspro_blk: " Maxim Levitsky
2020-07-21 11:17   ` Damien Le Moal
2020-07-21 10:52 ` [PATCH 08/10] block: nvme: " Maxim Levitsky
2020-07-21 11:23   ` Damien Le Moal
2020-07-21 10:52 ` [PATCH 09/10] block: scsi: sd: " Maxim Levitsky
2020-07-21 11:25   ` Damien Le Moal
2020-07-21 12:55     ` Maxim Levitsky
2020-07-21 10:52 ` [PATCH 10/10] block: scsi: sr: " Maxim Levitsky
2020-07-21 11:29   ` Damien Le Moal

Linux-mmc Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mmc/0 linux-mmc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mmc linux-mmc/ https://lore.kernel.org/linux-mmc \
		linux-mmc@vger.kernel.org
	public-inbox-index linux-mmc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-mmc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git