Message-ID: <20211014015029-mutt-send-email-mst@kernel.org>
Date:   Thu, 14 Oct 2021 01:55:39 -0400
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Jason Wang <jasowang@...hat.com>
Cc:     linux-kernel <linux-kernel@...r.kernel.org>,
        stable@...r.kernel.org, Xie Yongji <xieyongji@...edance.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Stefan Hajnoczi <stefanha@...hat.com>,
        Jens Axboe <axboe@...nel.dk>,
        virtualization <virtualization@...ts.linux-foundation.org>,
        linux-block@...r.kernel.org
Subject: Re: [PATCH] Revert "virtio-blk: Add validation for block size in
 config space"

On Thu, Oct 14, 2021 at 10:45:25AM +0800, Jason Wang wrote:
> On Wed, Oct 13, 2021 at 8:46 PM Michael S. Tsirkin <mst@...hat.com> wrote:
> >
> > It turns out that access to config space before completing the feature
> > negotiation is broken for big endian guests at least with QEMU hosts up
> > to 6.1 inclusive.  This affects any device that accesses config space in
> > the validate callback: at the moment that is virtio-net with
> > VIRTIO_NET_F_MTU but since 82e89ea077b9 ("virtio-blk: Add validation for
> > block size in config space") that also started affecting virtio-blk with
> > VIRTIO_BLK_F_BLK_SIZE.
> 
> Ok, so it looks like I need to do something similar in my series to
> validate max_nr_ports in probe() instead of validate()? (multi port is
> not enabled by default).

I think what we should do is fix upstream QEMU first. With that, plus
Halil's work-around writing out the version 1 feature, we should be
good reading config in validate.

> 
> I wonder if we need to document this and rename the validate to
> validate_features() (since we can do very little thing if we can't
> read config in .validate()).

That's not a bad idea anyway - any validation that does not
need to clear features can be done in probe in any case.


> > Further, unlike VIRTIO_NET_F_MTU which is off by
> > default on QEMU, VIRTIO_BLK_F_BLK_SIZE is on by default, which resulted
> > in lots of people not being able to boot VMs on BE.
> >
> > The spec is very clear that what we are doing is legal so QEMU needs to
> > be fixed, but given it's been broken for so many years and no one
> > noticed, we need to give QEMU a bit more time before applying this.
> >
> > Further, this patch is incomplete (does not check blk size is a power
> > of two) and it duplicates the logic from nbd.
> >
> > Revert for now, and we'll reapply a cleaner logic in the next release.
> >
> > Cc: stable@...r.kernel.org
> > Fixes: 82e89ea077b9 ("virtio-blk: Add validation for block size in config space")
> > Cc: Xie Yongji <xieyongji@...edance.com>
> > Signed-off-by: Michael S. Tsirkin <mst@...hat.com>
> 
> Acked-by: jason Wang <jasowang@...hat.com>
> 
> > ---
> >  drivers/block/virtio_blk.c | 37 ++++++-------------------------------
> >  1 file changed, 6 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > index 9b3bd083b411..303caf2d17d0 100644
> > --- a/drivers/block/virtio_blk.c
> > +++ b/drivers/block/virtio_blk.c
> > @@ -689,28 +689,6 @@ 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 int virtblk_validate(struct virtio_device *vdev)
> > -{
> > -       u32 blk_size;
> > -
> > -       if (!vdev->config->get) {
> > -               dev_err(&vdev->dev, "%s failure: config access disabled\n",
> > -                       __func__);
> > -               return -EINVAL;
> > -       }
> > -
> > -       if (!virtio_has_feature(vdev, VIRTIO_BLK_F_BLK_SIZE))
> > -               return 0;
> > -
> > -       blk_size = virtio_cread32(vdev,
> > -                       offsetof(struct virtio_blk_config, blk_size));
> > -
> > -       if (blk_size < SECTOR_SIZE || blk_size > PAGE_SIZE)
> > -               __virtio_clear_bit(vdev, VIRTIO_BLK_F_BLK_SIZE);
> > -
> > -       return 0;
> > -}
> > -
> >  static int virtblk_probe(struct virtio_device *vdev)
> >  {
> >         struct virtio_blk *vblk;
> > @@ -722,6 +700,12 @@ static int virtblk_probe(struct virtio_device *vdev)
> >         u8 physical_block_exp, alignment_offset;
> >         unsigned int queue_depth;
> >
> > +       if (!vdev->config->get) {
> > +               dev_err(&vdev->dev, "%s failure: config access disabled\n",
> > +                       __func__);
> > +               return -EINVAL;
> > +       }
> > +
> >         err = ida_simple_get(&vd_index_ida, 0, minor_to_index(1 << MINORBITS),
> >                              GFP_KERNEL);
> >         if (err < 0)
> > @@ -836,14 +820,6 @@ static int virtblk_probe(struct virtio_device *vdev)
> >         else
> >                 blk_size = queue_logical_block_size(q);
> >
> > -       if (blk_size < SECTOR_SIZE || blk_size > PAGE_SIZE) {
> > -               dev_err(&vdev->dev,
> > -                       "block size is changed unexpectedly, now is %u\n",
> > -                       blk_size);
> > -               err = -EINVAL;
> > -               goto out_cleanup_disk;
> > -       }
> > -
> >         /* Use topology information if available */
> >         err = virtio_cread_feature(vdev, VIRTIO_BLK_F_TOPOLOGY,
> >                                    struct virtio_blk_config, physical_block_exp,
> > @@ -1009,7 +985,6 @@ static struct virtio_driver virtio_blk = {
> >         .driver.name                    = KBUILD_MODNAME,
> >         .driver.owner                   = THIS_MODULE,
> >         .id_table                       = id_table,
> > -       .validate                       = virtblk_validate,
> >         .probe                          = virtblk_probe,
> >         .remove                         = virtblk_remove,
> >         .config_changed                 = virtblk_config_changed,
> > --
> > MST
> >

Powered by blists - more mailing lists