Re: [PATCH] Revert "virtio-blk: Add validation for block size in config space"

[Jason Wang <jasowang@redhat.com>]

On Wed, Oct 13, 2021 at 8:46 PM Michael S. Tsirkin <mst@redhat.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 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()).

> 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@vger.kernel.org
> Fixes: 82e89ea077b9 ("virtio-blk: Add validation for block size in config space")
> Cc: Xie Yongji <xieyongji@bytedance.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Acked-by: jason Wang <jasowang@redhat.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
>