This patch series aims to move the logical block size checking to the
block code.
This was inspired by missing check for valid logical block size in
virtio-blk which causes the kernel to crash in a weird way later on
when it is invalid.
I added blk_is_valid_logical_block_size which returns true iff the
block size is one of supported sizes.
I added this check to virtio-blk, and also converted few block drivers
that I am familiar with to use this interface instead of their
own implementation.
Best regards,
Maxim Levitsky
Maxim Levitsky (10):
block: introduce blk_is_valid_logical_block_size
block: virtio-blk: check logical block size
block: loop: use blk_is_valid_logical_block_size
block: nbd: use blk_is_valid_logical_block_size
block: null: use blk_is_valid_logical_block_size
block: ms_block: use blk_is_valid_logical_block_size
block: mspro_blk: use blk_is_valid_logical_block_size
block: nvme: use blk_is_valid_logical_block_size
block: scsi: sd: use blk_is_valid_logical_block_size
block: scsi: sr: use blk_is_valid_logical_block_size
block/blk-settings.c | 18 +++++++++++++++++
drivers/block/loop.c | 23 +++++----------------
drivers/block/nbd.c | 12 ++---------
drivers/block/null_blk_main.c | 6 +++---
drivers/block/virtio_blk.c | 15 ++++++++++++--
drivers/memstick/core/ms_block.c | 2 +-
drivers/memstick/core/mspro_block.c | 6 ++++++
drivers/nvme/host/core.c | 17 ++++++++--------
drivers/scsi/sd.c | 5 +----
drivers/scsi/sr.c | 31 ++++++++++++-----------------
include/linux/blkdev.h | 1 +
11 files changed, 71 insertions(+), 65 deletions(-)
--
2.26.2
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 <[email protected]>
---
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
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 <[email protected]>
---
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
This allows to remove loop's own check for supported block size
Signed-off-by: Maxim Levitsky <[email protected]>
---
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
This allows to remove nbd's own check for valid block size
Signed-off-by: Maxim Levitsky <[email protected]>
---
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
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 <[email protected]>
---
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
Signed-off-by: Maxim Levitsky <[email protected]>
---
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
Plus some tiny refactoring.
Signed-off-by: Maxim Levitsky <[email protected]>
---
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
Signed-off-by: Maxim Levitsky <[email protected]>
---
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
This replaces manual checking in the driver
Signed-off-by: Maxim Levitsky <[email protected]>
---
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
Use blk_is_valid_logical_block_size instead of hardcoded list
Signed-off-by: Maxim Levitsky <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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 <[email protected]>
--
Damien Le Moal
Western Digital Research
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 <[email protected]>
> ---
> 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 <[email protected]>
--
Damien Le Moal
Western Digital Research
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 <[email protected]>
> ---
> 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 <[email protected]>
--
Damien Le Moal
Western Digital Research
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 <[email protected]>
> > ---
> > 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);
> >
>
>
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 <[email protected]>
> ---
> 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
On 2020/07/21 19:54, Maxim Levitsky wrote:
> Signed-off-by: Maxim Levitsky <[email protected]>
> ---
> 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 <[email protected]>
--
Damien Le Moal
Western Digital Research
On 2020/07/21 19:55, Maxim Levitsky wrote:
> Signed-off-by: Maxim Levitsky <[email protected]>
> ---
> 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 <[email protected]>
--
Damien Le Moal
Western Digital Research
On 2020/07/21 19:55, Maxim Levitsky wrote:
> This replaces manual checking in the driver
>
> Signed-off-by: Maxim Levitsky <[email protected]>
> ---
> 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 <[email protected]>
--
Damien Le Moal
Western Digital Research
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 <[email protected]>
> ---
> 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 <[email protected]>
--
Damien Le Moal
Western Digital Research
On 2020/07/21 19:55, Maxim Levitsky wrote:
> Plus some tiny refactoring.
>
> Signed-off-by: Maxim Levitsky <[email protected]>
> ---
> 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
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 <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
> > ---
> > 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
>
>
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 <[email protected]>
> > ---
> > 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 <[email protected]>
>
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 <[email protected]>
> ---
> 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.
> +/**
> + * 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.
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
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
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
Greeting,
FYI, we noticed the following commit (built with gcc-9):
commit: 4c873fbf079b55e03beaee34e922d7c1e4090a07 ("[PATCH 01/10] block: introduce blk_is_valid_logical_block_size")
url: https://github.com/0day-ci/linux/commits/Maxim-Levitsky/RFC-move-logical-block-size-checking-to-the-block-core/20200721-185651
base: https://git.kernel.org/cgit/linux/kernel/git/axboe/linux-block.git for-next
in testcase: boot
on test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 8 -m 16G
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
+---------------------------------------------------------------+------------+------------+
| | d9ad700633 | 4c873fbf07 |
+---------------------------------------------------------------+------------+------------+
| boot_successes | 16 | 0 |
| boot_failures | 0 | 18 |
| WARNING:at_block/blk-settings.c:#blk_queue_logical_block_size | 0 | 17 |
| RIP:blk_queue_logical_block_size | 0 | 17 |
| BUG:kernel_hang_in_boot_stage | 0 | 1 |
| calltrace:__do_softirq | 0 | 2 |
| calltrace:asm_call_on_stack | 0 | 3 |
+---------------------------------------------------------------+------------+------------+
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
[ 46.395587] WARNING: CPU: 3 PID: 52 at block/blk-settings.c:341 blk_queue_logical_block_size+0x12/0x50
[ 46.396681] Modules linked in: sd_mod t10_pi sg ata_generic pata_acpi intel_rapl_msr intel_rapl_common ppdev crct10dif_pclmul crc32_pclmul snd_pcm crc32c_intel parport_pc ghash_clmulni_intel bochs_drm snd_timer aesni_intel crypto_simd snd ata_piix drm_vram_helper drm_ttm_helper cryptd glue_helper joydev parport floppy soundcore i2c_piix4 libata ttm virtio_scsi serio_raw pcspkr ip_tables
[ 46.396831] sd 0:0:1:10: Power-on or device reset occurred
[ 46.396960] sd 0:0:1:0: Power-on or device reset occurred
[ 46.396982] sd 0:0:1:7: Power-on or device reset occurred
[ 46.404324] sd 0:0:1:8: Power-on or device reset occurred
[ 46.407516] sd 0:0:1:5: Power-on or device reset occurred
[ 46.407595] ------------[ cut here ]------------
[ 46.407608] WARNING: CPU: 5 PID: 53 at block/blk-settings.c:341 blk_queue_logical_block_size+0x12/0x50
[ 46.407610] Modules linked in: sd_mod t10_pi sg ata_generic pata_acpi intel_rapl_msr intel_rapl_common ppdev crct10dif_pclmul crc32_pclmul snd_pcm crc32c_intel parport_pc ghash_clmulni_intel bochs_drm snd_timer aesni_intel crypto_simd snd ata_piix drm_vram_helper drm_ttm_helper cryptd glue_helper joydev parport floppy soundcore i2c_piix4 libata ttm virtio_scsi serio_raw pcspkr ip_tables
[ 46.407648] CPU: 5 PID: 53 Comm: kworker/u16:4 Not tainted 5.8.0-rc6-00292-g4c873fbf079b5 #1
[ 46.407650] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
[ 46.407656] Workqueue: events_unbound async_run_entry_fn
[ 46.407661] RIP: 0010:blk_queue_logical_block_size+0x12/0x50
[ 46.407664] Code: 0b 89 b3 a8 04 00 00 5b c3 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 66 66 66 66 90 8d 86 00 fe ff ff 3d 00 0e 00 00 76 2a <0f> 0b 8b 87 ac 04 00 00 89 b7 b0 04 00 00 39 f0 73 08 89 b7 ac 04
[ 46.407666] RSP: 0018:ffffa13b80207cd8 EFLAGS: 00010246
[ 46.407670] RAX: 0000000000000200 RBX: ffff8ba269b08000 RCX: 0000000000000000
[ 46.407672] RDX: 00000000000001ff RSI: 0000000000000200 RDI: ffff8ba2813ab560
[ 46.407674] RBP: ffff8ba28175a000 R08: 0000000000000000 R09: 0000000000000000
[ 46.407677] R10: 0000000000000001 R11: 0000000000000003 R12: 0000000000000200
[ 46.407679] R13: 0000000000000200 R14: 0000000000000000 R15: 0000000000000000
[ 46.407682] FS: 0000000000000000(0000) GS:ffff8ba36fd40000(0000) knlGS:0000000000000000
[ 46.407684] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 46.407687] CR2: 00007faea48ed4a0 CR3: 0000000338a68000 CR4: 00000000000406e0
[ 46.407693] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 46.407695] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 46.407697] Call Trace:
[ 46.407708] sd_read_capacity+0x12b/0x370 [sd_mod]
[ 46.407718] sd_revalidate_disk+0x305/0x1030 [sd_mod]
[ 46.407727] ? device_add+0x17b/0x790
[ 46.407738] sd_probe+0x2ce/0x460 [sd_mod]
[ 46.407747] really_probe+0x298/0x3c0
[ 46.407754] driver_probe_device+0xe1/0x150
[ 46.407760] __driver_attach_async_helper+0x83/0x90
[ 46.407765] async_run_entry_fn+0x39/0x160
[ 46.407771] process_one_work+0x23c/0x5a0
[ 46.407784] worker_thread+0x50/0x3b0
[ 46.407792] ? process_one_work+0x5a0/0x5a0
[ 46.407796] kthread+0x134/0x180
[ 46.407800] ? kthread_park+0x90/0x90
[ 46.407808] ret_from_fork+0x22/0x30
[ 46.407827] irq event stamp: 789
[ 46.407832] hardirqs last enabled at (795): [<ffffffff95dcc9cf>] vprintk_emit+0x35f/0x370
[ 46.407835] hardirqs last disabled at (800): [<ffffffff95dcc872>] vprintk_emit+0x202/0x370
[ 46.407840] softirqs last enabled at (384): [<ffffffff95cd8dff>] fpu__copy+0x9f/0x310
[ 46.407842] softirqs last disabled at (382): [<ffffffff95cd8da3>] fpu__copy+0x43/0x310
[ 46.407844] ---[ end trace d9e8c103d8a266ee ]---
To reproduce:
# build kernel
cd linux
cp config-5.8.0-rc6-00292-g4c873fbf079b5 .config
make HOSTCC=gcc-9 CC=gcc-9 ARCH=x86_64 olddefconfig prepare modules_prepare bzImage
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k <bzImage> job-script # job-script is attached in this email
Thanks,
lkp
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
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 <[email protected]>
> ---
> drivers/block/virtio_blk.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
Reviewed-by: Stefan Hajnoczi <[email protected]>
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