[PATCH 1/1] scsi: virtio-scsi: handle correctly case when all LUNs were unplugged
Commit 5ff843721467 ("scsi: virtio_scsi: unplug LUNs when events missed"),
almost fixed the case of mass unpluging of LUNs, but it missed a
corner case in which all the LUNs are unplugged at the same time.
In this case INQUIRY ends with DID_BAD_TARGET.
Detect this and unplug the LUN.
Signed-off-by: Maxim Levitsky
---
drivers/scsi/virtio_scsi.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 0e0910c5b9424..c7f0c22b6f11d 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -351,6 +351,16 @@ static void virtscsi_rescan_hotunplug(struct virtio_scsi
*vscsi)
/* PQ indicates the LUN is not attached */
scsi_remove_device(sdev);
}
+
+ else if (host_byte(result) == DID_BAD_TARGET) {
+ /*
+* if all LUNs of a virtio-scsi device are unplugged,
+* it will respond with BAD TARGET on any INQUIRY
+* command.
+* Remove the device in this case as well
+*/
+ scsi_remove_device(sdev);
+ }
}
kfree(inq_result);
--
2.26.2
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 0/1] virtio-scsi: fix missing unplug events when all LUNs are unplugged at the same time
virtio-scsi currently has limit of 8 outstanding notifications so when more that
8 LUNs are unplugged, some are missed.
Commit 5ff843721467 ("scsi: virtio_scsi: unplug LUNs when events missed")
Fixed this by checking the 'event overflow' bit and manually scanned the bus
to see which LUNs are still there.
However there is a corner case when all LUNs are unplugged.
In this case (which is not fully scsi confirmant IMHO), all scsi
commands to such device respond with INVALID TARGET.
This patch proposes to detect this and remove the LUN in this case
as well.
Maxim Levitsky (1):
scsi: virtio-scsi: handle correctly case when all LUNs were unplugged
drivers/scsi/virtio_scsi.c | 10 ++
1 file changed, 10 insertions(+)
--
2.26.2
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 02/10] block: virtio-blk: check logical block size
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 02/10] block: virtio-blk: check logical block size
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 01/10] block: introduce blk_is_valid_logical_block_size
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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 09/10] block: scsi: sd: use blk_is_valid_logical_block_size
On Tue, 2020-07-21 at 11:25 +, 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
> > ---
> > 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
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 05/10] block: null: use blk_is_valid_logical_block_size
On Tue, 2020-07-21 at 11:15 +, 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
> > ---
> > 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
>
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 01/10] block: introduce blk_is_valid_logical_block_size
On Tue, 2020-07-21 at 11:05 +, 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
> > ---
> > 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);
> >
>
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 09/10] block: scsi: sd: use blk_is_valid_logical_block_size
Use blk_is_valid_logical_block_size instead of hardcoded list
Signed-off-by: Maxim Levitsky
---
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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 10/10] block: scsi: sr: use blk_is_valid_logical_block_size
Plus some tiny refactoring.
Signed-off-by: Maxim Levitsky
---
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([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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 08/10] block: nvme: use blk_is_valid_logical_block_size
This replaces manual checking in the driver
Signed-off-by: Maxim Levitsky
---
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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 07/10] block: mspro_blk: use blk_is_valid_logical_block_size
Signed-off-by: Maxim Levitsky
---
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(>dev,
+"unsupported block size %d", msb->page_size);
+ return -EINVAL;
+ }
+
mutex_lock(_block_disk_lock);
disk_id = idr_alloc(_block_disk_idr, card, 0, 256, GFP_KERNEL);
mutex_unlock(_block_disk_lock);
--
2.26.2
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 05/10] block: null: use blk_is_valid_logical_block_size
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
---
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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 06/10] block: ms_block: use blk_is_valid_logical_block_size
Signed-off-by: Maxim Levitsky
---
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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 02/10] block: virtio-blk: check logical block size
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
---
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,
_size);
- if (!err)
+ if (!err) {
+ if (!blk_is_valid_logical_block_size(blk_size)) {
+ dev_err(>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(>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(>tag_set);
out_put_disk:
--
2.26.2
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 03/10] block: loop: use blk_is_valid_logical_block_size
This allows to remove loop's own check for supported block size
Signed-off-by: Maxim Levitsky
---
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, >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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 04/10] block: nbd: use blk_is_valid_logical_block_size
This allows to remove nbd's own check for valid block size
Signed-off-by: Maxim Levitsky
---
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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 01/10] block: introduce blk_is_valid_logical_block_size
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
---
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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 00/10] RFC: move logical block size checking to the block core
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-blk: check host supplied logical block size
On Wed, 2020-07-15 at 07:53 -0400, Michael S. Tsirkin wrote:
> On Wed, Jul 15, 2020 at 01:19:55PM +0300, Maxim Levitsky wrote:
> > On Wed, 2020-07-15 at 06:06 -0400, Michael S. Tsirkin wrote:
> > > On Wed, Jul 15, 2020 at 12:55:18PM +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
> > > > ---
> > > > drivers/block/virtio_blk.c | 20 ++--
> > > > 1 file changed, 18 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > > > index 980df853ee497..36dda31cc4e96 100644
> > > > --- a/drivers/block/virtio_blk.c
> > > > +++ b/drivers/block/virtio_blk.c
> > > > @@ -681,6 +681,12 @@ static const struct blk_mq_ops virtio_mq_ops =
> > > > {
> > > > static unsigned int virtblk_queue_depth;
> > > > module_param_named(queue_depth, virtblk_queue_depth, uint, 0444);
> > > >
> > > > +
> > > > +static bool virtblk_valid_block_size(unsigned int blksize)
> > > > +{
> > > > + return blksize >= 512 && blksize <= PAGE_SIZE &&
> > > > is_power_of_2(blksize);
> > > > +}
> > > > +
> > >
> > > Is this a blk core assumption? in that case, does not this belong
> > > in blk core?
> >
> > It is a blk core assumption.
> > I had checked other drivers and these that have variable block size all
> > check this manually like that.
> >
> > I don't mind fixing all of them but I am a bit afraid to create
> > too much mess.
>
> You don't have to, start by adding this in blk core and using in virtio.
> Patches to update all drivers can then come separately.
I'll do this then.
Best regards,
Maxim Levitsky
>
> > > > static int virtblk_probe(struct virtio_device *vdev)
> > > > {
> > > > struct virtio_blk *vblk;
> > > > @@ -809,9 +815,16 @@ static int virtblk_probe(struct virtio_device
> > > > *vdev)
> > > > err = virtio_cread_feature(vdev, VIRTIO_BLK_F_BLK_SIZE,
> > > >struct virtio_blk_config, blk_size,
> > > >_size);
> > > > - if (!err)
> > > > + if (!err) {
> > > > + if (!virtblk_valid_block_size(blk_size)) {
> > > > + dev_err(>dev,
> > > > + "%s failure: unsupported 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 */
> > >
> > > OK so if we are doing this pls add {} around blk_size =
> > > queue_logical_block_size(q);
> > > too.
> > Will do.
> >
> > > > @@ -872,6 +885,9 @@ static int virtblk_probe(struct virtio_device
> > > > *vdev)
> > > > device_add_disk(>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(>tag_set);
> > > > out_put_disk:
> > > > --
> > > > 2.26.2
> >
> > Best regards,
> > Maxim Levitsky
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-blk: check host supplied logical block size
On Wed, 2020-07-15 at 06:06 -0400, Michael S. Tsirkin wrote:
> On Wed, Jul 15, 2020 at 12:55:18PM +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
> > ---
> > drivers/block/virtio_blk.c | 20 ++--
> > 1 file changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > index 980df853ee497..36dda31cc4e96 100644
> > --- a/drivers/block/virtio_blk.c
> > +++ b/drivers/block/virtio_blk.c
> > @@ -681,6 +681,12 @@ static const struct blk_mq_ops virtio_mq_ops =
> > {
> > static unsigned int virtblk_queue_depth;
> > module_param_named(queue_depth, virtblk_queue_depth, uint, 0444);
> >
> > +
> > +static bool virtblk_valid_block_size(unsigned int blksize)
> > +{
> > + return blksize >= 512 && blksize <= PAGE_SIZE &&
> > is_power_of_2(blksize);
> > +}
> > +
>
> Is this a blk core assumption? in that case, does not this belong
> in blk core?
It is a blk core assumption.
I had checked other drivers and these that have variable block size all
check this manually like that.
I don't mind fixing all of them but I am a bit afraid to create
too much mess.
>
> > static int virtblk_probe(struct virtio_device *vdev)
> > {
> > struct virtio_blk *vblk;
> > @@ -809,9 +815,16 @@ static int virtblk_probe(struct virtio_device
> > *vdev)
> > err = virtio_cread_feature(vdev, VIRTIO_BLK_F_BLK_SIZE,
> >struct virtio_blk_config, blk_size,
> >_size);
> > - if (!err)
> > + if (!err) {
> > + if (!virtblk_valid_block_size(blk_size)) {
> > + dev_err(>dev,
> > + "%s failure: unsupported 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 */
>
> OK so if we are doing this pls add {} around blk_size =
> queue_logical_block_size(q);
> too.
Will do.
>
> > @@ -872,6 +885,9 @@ static int virtblk_probe(struct virtio_device
> > *vdev)
> > device_add_disk(>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(>tag_set);
> > out_put_disk:
> > --
> > 2.26.2
Best regards,
Maxim Levitsky
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] virtio-blk: check host supplied logical block size
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
---
drivers/block/virtio_blk.c | 20 ++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 980df853ee497..36dda31cc4e96 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -681,6 +681,12 @@ static const struct blk_mq_ops virtio_mq_ops = {
static unsigned int virtblk_queue_depth;
module_param_named(queue_depth, virtblk_queue_depth, uint, 0444);
+
+static bool virtblk_valid_block_size(unsigned int blksize)
+{
+ return blksize >= 512 && blksize <= PAGE_SIZE && is_power_of_2(blksize);
+}
+
static int virtblk_probe(struct virtio_device *vdev)
{
struct virtio_blk *vblk;
@@ -809,9 +815,16 @@ static int virtblk_probe(struct virtio_device *vdev)
err = virtio_cread_feature(vdev, VIRTIO_BLK_F_BLK_SIZE,
struct virtio_blk_config, blk_size,
_size);
- if (!err)
+ if (!err) {
+ if (!virtblk_valid_block_size(blk_size)) {
+ dev_err(>dev,
+ "%s failure: unsupported 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 */
@@ -872,6 +885,9 @@ static int virtblk_probe(struct virtio_device *vdev)
device_add_disk(>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(>tag_set);
out_put_disk:
--
2.26.2
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
