On Wed, Oct 13, 2021 at 8:46 PM Michael S. Tsirkin <mst@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> Fixes: 82e89ea077b9 ("virtio-blk: Add validation for block size in config space")
> Cc: Xie Yongji <xieyongji@xxxxxxxxxxxxx>
> Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
Acked-by: jason Wang <jasowang@xxxxxxxxxx>
> ---
> 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
>
