* [PATCH] Revert "virtio-blk: Add validation for block size in config space"
@ 2021-10-13 12:46 ` Michael S. Tsirkin
0 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2021-10-13 12:46 UTC (permalink / raw)
To: linux-kernel
Cc: stable, Xie Yongji, Jason Wang, Paolo Bonzini, Stefan Hajnoczi,
Jens Axboe, virtualization, linux-block
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. 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>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] Revert "virtio-blk: Add validation for block size in config space"
@ 2021-10-13 12:46 ` Michael S. Tsirkin
0 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2021-10-13 12:46 UTC (permalink / raw)
To: linux-kernel
Cc: Jens Axboe, stable, virtualization, linux-block, Xie Yongji,
Stefan Hajnoczi, Paolo Bonzini
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. 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>
---
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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "virtio-blk: Add validation for block size in config space"
2021-10-13 12:46 ` Michael S. Tsirkin
@ 2021-10-14 2:45 ` Jason Wang
-1 siblings, 0 replies; 6+ messages in thread
From: Jason Wang @ 2021-10-14 2:45 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-kernel, stable, Xie Yongji, Paolo Bonzini, Stefan Hajnoczi,
Jens Axboe, virtualization, linux-block
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "virtio-blk: Add validation for block size in config space"
@ 2021-10-14 2:45 ` Jason Wang
0 siblings, 0 replies; 6+ messages in thread
From: Jason Wang @ 2021-10-14 2:45 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jens Axboe, linux-kernel, stable, virtualization, linux-block,
Xie Yongji, Stefan Hajnoczi, Paolo Bonzini
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
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "virtio-blk: Add validation for block size in config space"
2021-10-14 2:45 ` Jason Wang
@ 2021-10-14 5:55 ` Michael S. Tsirkin
-1 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2021-10-14 5:55 UTC (permalink / raw)
To: Jason Wang
Cc: linux-kernel, stable, Xie Yongji, Paolo Bonzini, Stefan Hajnoczi,
Jens Axboe, virtualization, linux-block
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@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 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@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
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "virtio-blk: Add validation for block size in config space"
@ 2021-10-14 5:55 ` Michael S. Tsirkin
0 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2021-10-14 5:55 UTC (permalink / raw)
To: Jason Wang
Cc: Jens Axboe, linux-kernel, stable, virtualization, linux-block,
Xie Yongji, Stefan Hajnoczi, Paolo Bonzini
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@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 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@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
> >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-14 5:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-13 12:46 [PATCH] Revert "virtio-blk: Add validation for block size in config space" Michael S. Tsirkin
2021-10-13 12:46 ` Michael S. Tsirkin
2021-10-14 2:45 ` Jason Wang
2021-10-14 2:45 ` Jason Wang
2021-10-14 5:55 ` Michael S. Tsirkin
2021-10-14 5:55 ` Michael S. Tsirkin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.