Re: [PATCH] virtio: reset device on bad guest index in virtio_load()
On Fri, Nov 20, 2020 at 06:51:07PM +, 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 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(>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(>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(>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(>vq[i]); > vdev->vq[i].shadow_avail_idx = vring_avail_idx(>vq[i]); > -- > 2.22.3 >
Re: [PATCH] virtio: reset device on bad guest index in virtio_load()
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 +, John Levon wrote: > On Fri, Nov 20, 2020 at 06:51:07PM +, 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 > > --- > > 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(>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(>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(>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(>vq[i]); > > vdev->vq[i].shadow_avail_idx = vring_avail_idx(>vq[i]);
Re: [PATCH] virtio: reset device on bad guest index in virtio_load()
On Fri, Nov 20, 2020 at 06:51:07PM +, 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 > --- > 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(>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(>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(>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(>vq[i]); > vdev->vq[i].shadow_avail_idx = vring_avail_idx(>vq[i]);
[PATCH] virtio: reset device on bad guest index in virtio_load()
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 --- 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(>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(>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(>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(>vq[i]); vdev->vq[i].shadow_avail_idx = vring_avail_idx(>vq[i]); -- 2.22.3
[PATCH] virtio: reset device on bad guest index in virtio_load()
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 --- 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(>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(>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(>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(>vq[i]); vdev->vq[i].shadow_avail_idx = vring_avail_idx(>vq[i]); -- 2.22.3