1. Patchew
  2. QEMU
  3. View series

[PATCH] virtio: reset device on bad guest index in virtio_load()

John Levon posted 1 patch 3 years, 5 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20201120185103.GA442386@sent
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>
hw/virtio/virtio.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
[PATCH] virtio: reset device on bad guest index in virtio_load()
Posted by John Levon 3 years, 5 months ago 

If we find a queue with an inconsistent guest index value, explicitly mark the
device as needing a reset - and broken - via virtio_error().

There's at least one driver implementation - the virtio-win NetKVM driver - that
is able to handle a VIRTIO_CONFIG_S_NEEDS_RESET notification and successfully
restore the device to a working state. Other implementations do not correctly
handle this, but as the VQ is not in a functional state anyway, this is still
worth doing.

Signed-off-by: John Levon <john.levon@nutanix.com>
---
 hw/virtio/virtio.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index ceb58fda6c..eff35fab7c 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3161,12 +3161,15 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
             nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
             /* Check it isn't doing strange things with descriptor numbers. */
             if (nheads > vdev->vq[i].vring.num) {
-                qemu_log_mask(LOG_GUEST_ERROR,
-                              "VQ %d size 0x%x Guest index 0x%x "
-                              "inconsistent with Host index 0x%x: delta 0x%x",
-                              i, vdev->vq[i].vring.num,
-                              vring_avail_idx(&vdev->vq[i]),
-                              vdev->vq[i].last_avail_idx, nheads);
+                virtio_error(vdev, "VQ %d size 0x%x Guest index 0x%x "
+                             "inconsistent with Host index 0x%x: delta 0x%x",
+                             i, vdev->vq[i].vring.num,
+                             vring_avail_idx(&vdev->vq[i]),
+                             vdev->vq[i].last_avail_idx, nheads);
+                vdev->vq[i].used_idx = 0;
+                vdev->vq[i].shadow_avail_idx = 0;
+                vdev->vq[i].inuse = 0;
+                continue;
             }
             vdev->vq[i].used_idx = vring_used_idx(&vdev->vq[i]);
             vdev->vq[i].shadow_avail_idx = vring_avail_idx(&vdev->vq[i]);
-- 
2.22.3
Re: [PATCH] virtio: reset device on bad guest index in virtio_load()
Posted by John Levon 3 years, 4 months ago 
On Fri, Nov 20, 2020 at 06:51:07PM +0000, John Levon wrote:

> If we find a queue with an inconsistent guest index value, explicitly mark the
> device as needing a reset - and broken - via virtio_error().
> 
> There's at least one driver implementation - the virtio-win NetKVM driver - that
> is able to handle a VIRTIO_CONFIG_S_NEEDS_RESET notification and successfully
> restore the device to a working state. Other implementations do not correctly
> handle this, but as the VQ is not in a functional state anyway, this is still
> worth doing.

Ping, anyone have issues with doing this?

cheers
john

> Signed-off-by: John Levon <john.levon@nutanix.com>
> ---
>  hw/virtio/virtio.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index ceb58fda6c..eff35fab7c 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3161,12 +3161,15 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>              nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
>              /* Check it isn't doing strange things with descriptor numbers. */
>              if (nheads > vdev->vq[i].vring.num) {
> -                qemu_log_mask(LOG_GUEST_ERROR,
> -                              "VQ %d size 0x%x Guest index 0x%x "
> -                              "inconsistent with Host index 0x%x: delta 0x%x",
> -                              i, vdev->vq[i].vring.num,
> -                              vring_avail_idx(&vdev->vq[i]),
> -                              vdev->vq[i].last_avail_idx, nheads);
> +                virtio_error(vdev, "VQ %d size 0x%x Guest index 0x%x "
> +                             "inconsistent with Host index 0x%x: delta 0x%x",
> +                             i, vdev->vq[i].vring.num,
> +                             vring_avail_idx(&vdev->vq[i]),
> +                             vdev->vq[i].last_avail_idx, nheads);
> +                vdev->vq[i].used_idx = 0;
> +                vdev->vq[i].shadow_avail_idx = 0;
> +                vdev->vq[i].inuse = 0;
> +                continue;
>              }
>              vdev->vq[i].used_idx = vring_used_idx(&vdev->vq[i]);
>              vdev->vq[i].shadow_avail_idx = vring_avail_idx(&vdev->vq[i]);
Re: [PATCH] virtio: reset device on bad guest index in virtio_load()
Posted by Michael S. Tsirkin 3 years, 4 months ago 
No, but how about sending the patch to me and the mailing list?
I didn't get it through either channel.

On Mon, Nov 30, 2020 at 03:51:13PM +0000, John Levon wrote:
> On Fri, Nov 20, 2020 at 06:51:07PM +0000, John Levon wrote:
> 
> > If we find a queue with an inconsistent guest index value, explicitly mark the
> > device as needing a reset - and broken - via virtio_error().
> > 
> > There's at least one driver implementation - the virtio-win NetKVM driver - that
> > is able to handle a VIRTIO_CONFIG_S_NEEDS_RESET notification and successfully
> > restore the device to a working state. Other implementations do not correctly
> > handle this, but as the VQ is not in a functional state anyway, this is still
> > worth doing.
> 
> Ping, anyone have issues with doing this?
> 
> cheers
> john
> 
> > Signed-off-by: John Levon <john.levon@nutanix.com>
> > ---
> >  hw/virtio/virtio.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index ceb58fda6c..eff35fab7c 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -3161,12 +3161,15 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
> >              nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
> >              /* Check it isn't doing strange things with descriptor numbers. */
> >              if (nheads > vdev->vq[i].vring.num) {
> > -                qemu_log_mask(LOG_GUEST_ERROR,
> > -                              "VQ %d size 0x%x Guest index 0x%x "
> > -                              "inconsistent with Host index 0x%x: delta 0x%x",
> > -                              i, vdev->vq[i].vring.num,
> > -                              vring_avail_idx(&vdev->vq[i]),
> > -                              vdev->vq[i].last_avail_idx, nheads);
> > +                virtio_error(vdev, "VQ %d size 0x%x Guest index 0x%x "
> > +                             "inconsistent with Host index 0x%x: delta 0x%x",
> > +                             i, vdev->vq[i].vring.num,
> > +                             vring_avail_idx(&vdev->vq[i]),
> > +                             vdev->vq[i].last_avail_idx, nheads);
> > +                vdev->vq[i].used_idx = 0;
> > +                vdev->vq[i].shadow_avail_idx = 0;
> > +                vdev->vq[i].inuse = 0;
> > +                continue;
> >              }
> >              vdev->vq[i].used_idx = vring_used_idx(&vdev->vq[i]);
> >              vdev->vq[i].shadow_avail_idx = vring_avail_idx(&vdev->vq[i]);
Re: [PATCH] virtio: reset device on bad guest index in virtio_load()
Posted by Michael S. Tsirkin 3 years, 4 months ago 
On Fri, Nov 20, 2020 at 06:51:07PM +0000, John Levon wrote:
> 
> If we find a queue with an inconsistent guest index value, explicitly mark the
> device as needing a reset - and broken - via virtio_error().
> 
> There's at least one driver implementation - the virtio-win NetKVM driver - that
> is able to handle a VIRTIO_CONFIG_S_NEEDS_RESET notification and successfully
> restore the device to a working state. Other implementations do not correctly
> handle this, but as the VQ is not in a functional state anyway, this is still
> worth doing.
> 
> Signed-off-by: John Levon <john.levon@nutanix.com>

I tagged this for after the release. pls ping me after the release
to help make sure it does not get lost.

> ---
>  hw/virtio/virtio.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index ceb58fda6c..eff35fab7c 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3161,12 +3161,15 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>              nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
>              /* Check it isn't doing strange things with descriptor numbers. */
>              if (nheads > vdev->vq[i].vring.num) {
> -                qemu_log_mask(LOG_GUEST_ERROR,
> -                              "VQ %d size 0x%x Guest index 0x%x "
> -                              "inconsistent with Host index 0x%x: delta 0x%x",
> -                              i, vdev->vq[i].vring.num,
> -                              vring_avail_idx(&vdev->vq[i]),
> -                              vdev->vq[i].last_avail_idx, nheads);
> +                virtio_error(vdev, "VQ %d size 0x%x Guest index 0x%x "
> +                             "inconsistent with Host index 0x%x: delta 0x%x",
> +                             i, vdev->vq[i].vring.num,
> +                             vring_avail_idx(&vdev->vq[i]),
> +                             vdev->vq[i].last_avail_idx, nheads);
> +                vdev->vq[i].used_idx = 0;
> +                vdev->vq[i].shadow_avail_idx = 0;
> +                vdev->vq[i].inuse = 0;
> +                continue;
>              }
>              vdev->vq[i].used_idx = vring_used_idx(&vdev->vq[i]);
>              vdev->vq[i].shadow_avail_idx = vring_avail_idx(&vdev->vq[i]);
> -- 
> 2.22.3
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.