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 | 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
On Wed, Jul 15, 2020 at 5:55 PM Maxim Levitsky <[email protected]> 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 | 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
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Looks fine,
Reviewed-by: Ming Lei <[email protected]>
--
Ming Lei
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 <[email protected]>
> > > ---
> > > 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
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 <[email protected]>
> > > > ---
> > > > 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