* [PATCH] virtio-blk: check host supplied logical block size
@ 2020-07-15 9:55 Maxim Levitsky
2020-07-15 10:06 ` Michael S. Tsirkin
2020-07-15 10:33 ` Ming Lei
0 siblings, 2 replies; 6+ messages in thread
From: Maxim Levitsky @ 2020-07-15 9:55 UTC (permalink / raw)
To: linux-kernel
Cc: Stefan Hajnoczi, open list:BLOCK LAYER,
open list:VIRTIO CORE AND NET DRIVERS, Paolo Bonzini,
Michael S. Tsirkin, Jason Wang, Jens Axboe, Maxim Levitsky
Linux kernel only supports logical block sizes which are power of two,
at least 512 bytes and no more that PAGE_SIZE.
Check this instead of crashing later on.
Note that there is no need to check physical block size since it is
only a hint, and virtio-blk already only supports power of two values.
Bugzilla link: https://bugzilla.redhat.com/show_bug.cgi?id=1664619
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
drivers/block/virtio_blk.c | 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,
&blk_size);
- if (!err)
+ if (!err) {
+ if (!virtblk_valid_block_size(blk_size)) {
+ dev_err(&vdev->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(&vdev->dev, vblk->disk, virtblk_attr_groups);
return 0;
+out_cleanup_queue:
+ blk_cleanup_queue(vblk->disk->queue);
+ vblk->disk->queue = NULL;
out_free_tags:
blk_mq_free_tag_set(&vblk->tag_set);
out_put_disk:
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] virtio-blk: check host supplied logical block size
2020-07-15 9:55 [PATCH] virtio-blk: check host supplied logical block size Maxim Levitsky
@ 2020-07-15 10:06 ` Michael S. Tsirkin
2020-07-15 10:19 ` Maxim Levitsky
2020-07-15 10:33 ` Ming Lei
1 sibling, 1 reply; 6+ messages in thread
From: Michael S. Tsirkin @ 2020-07-15 10:06 UTC (permalink / raw)
To: Maxim Levitsky
Cc: linux-kernel, Stefan Hajnoczi, open list:BLOCK LAYER,
open list:VIRTIO CORE AND NET DRIVERS, Paolo Bonzini, Jason Wang,
Jens Axboe
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 <mlevitsk@redhat.com>
> ---
> 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?
> 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,
> &blk_size);
> - if (!err)
> + if (!err) {
> + if (!virtblk_valid_block_size(blk_size)) {
> + dev_err(&vdev->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.
> @@ -872,6 +885,9 @@ static int virtblk_probe(struct virtio_device *vdev)
> device_add_disk(&vdev->dev, vblk->disk, virtblk_attr_groups);
> return 0;
>
> +out_cleanup_queue:
> + blk_cleanup_queue(vblk->disk->queue);
> + vblk->disk->queue = NULL;
> out_free_tags:
> blk_mq_free_tag_set(&vblk->tag_set);
> out_put_disk:
> --
> 2.26.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] virtio-blk: check host supplied logical block size
2020-07-15 10:06 ` Michael S. Tsirkin
@ 2020-07-15 10:19 ` Maxim Levitsky
2020-07-15 11:53 ` Michael S. Tsirkin
0 siblings, 1 reply; 6+ messages in thread
From: Maxim Levitsky @ 2020-07-15 10:19 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-kernel, Stefan Hajnoczi, open list:BLOCK LAYER,
open list:VIRTIO CORE AND NET DRIVERS, Paolo Bonzini, Jason Wang,
Jens Axboe
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 <mlevitsk@redhat.com>
> > ---
> > 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,
> > &blk_size);
> > - if (!err)
> > + if (!err) {
> > + if (!virtblk_valid_block_size(blk_size)) {
> > + dev_err(&vdev->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(&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
Best regards,
Maxim Levitsky
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] virtio-blk: check host supplied logical block size
2020-07-15 9:55 [PATCH] virtio-blk: check host supplied logical block size Maxim Levitsky
2020-07-15 10:06 ` Michael S. Tsirkin
@ 2020-07-15 10:33 ` Ming Lei
1 sibling, 0 replies; 6+ messages in thread
From: Ming Lei @ 2020-07-15 10:33 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Linux Kernel Mailing List, Jens Axboe, Michael S. Tsirkin,
open list:VIRTIO CORE AND NET DRIVERS, open list:BLOCK LAYER,
Stefan Hajnoczi, Paolo Bonzini
On Wed, Jul 15, 2020 at 5:55 PM Maxim Levitsky <mlevitsk@redhat.com> wrote:
>
> Linux kernel only supports logical block sizes which are power of two,
> at least 512 bytes and no more that PAGE_SIZE.
>
> Check this instead of crashing later on.
>
> Note that there is no need to check physical block size since it is
> only a hint, and virtio-blk already only supports power of two values.
>
> Bugzilla link: https://bugzilla.redhat.com/show_bug.cgi?id=1664619
>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
> drivers/block/virtio_blk.c | 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,
> &blk_size);
> - if (!err)
> + if (!err) {
> + if (!virtblk_valid_block_size(blk_size)) {
> + dev_err(&vdev->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(&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
>
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Looks fine,
Reviewed-by: Ming Lei <ming.lei@redhat.com>
--
Ming Lei
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] virtio-blk: check host supplied logical block size
2020-07-15 10:19 ` Maxim Levitsky
@ 2020-07-15 11:53 ` Michael S. Tsirkin
2020-07-15 12:10 ` Maxim Levitsky
0 siblings, 1 reply; 6+ messages in thread
From: Michael S. Tsirkin @ 2020-07-15 11:53 UTC (permalink / raw)
To: Maxim Levitsky
Cc: linux-kernel, Stefan Hajnoczi, open list:BLOCK LAYER,
open list:VIRTIO CORE AND NET DRIVERS, Paolo Bonzini, Jason Wang,
Jens Axboe
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 <mlevitsk@redhat.com>
> > > ---
> > > 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.
> >
> > > 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,
> > > &blk_size);
> > > - if (!err)
> > > + if (!err) {
> > > + if (!virtblk_valid_block_size(blk_size)) {
> > > + dev_err(&vdev->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(&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
>
>
> Best regards,
> Maxim Levitsky
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] virtio-blk: check host supplied logical block size
2020-07-15 11:53 ` Michael S. Tsirkin
@ 2020-07-15 12:10 ` Maxim Levitsky
0 siblings, 0 replies; 6+ messages in thread
From: Maxim Levitsky @ 2020-07-15 12:10 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-kernel, Stefan Hajnoczi, open list:BLOCK LAYER,
open list:VIRTIO CORE AND NET DRIVERS, Paolo Bonzini, Jason Wang,
Jens Axboe
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 <mlevitsk@redhat.com>
> > > > ---
> > > > 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,
> > > > &blk_size);
> > > > - if (!err)
> > > > + if (!err) {
> > > > + if (!virtblk_valid_block_size(blk_size)) {
> > > > + dev_err(&vdev->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(&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
> >
> > Best regards,
> > Maxim Levitsky
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-07-15 12:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-15 9:55 [PATCH] virtio-blk: check host supplied logical block size Maxim Levitsky
2020-07-15 10:06 ` Michael S. Tsirkin
2020-07-15 10:19 ` Maxim Levitsky
2020-07-15 11:53 ` Michael S. Tsirkin
2020-07-15 12:10 ` Maxim Levitsky
2020-07-15 10:33 ` Ming Lei
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).