[PATCH] virtio: skip guest index check on device load

[PATCH] virtio: skip guest index check on device load

[Felipe Franciosi]

QEMU must be careful when loading device state off migration streams to
prevent a malicious source from exploiting the emulator. Overdoing these
checks has the side effect of allowing a guest to "pin itself" in cloud
environments by messing with state which is entirely in its control.

Similarly to what f3081539 achieved in usb_device_post_load(), this
commit removes such a check from virtio_load(). Worth noting, the result
of a load without this check is the same as if a guest enables a VQ with
invalid indexes to begin with. That is, the virtual device is set in a
broken state (by the datapath handler) and must be reset.

Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
---
 hw/virtio/virtio.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 6f8f865aff..0561bdb857 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3136,8 +3136,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
     RCU_READ_LOCK_GUARD();
     for (i = 0; i < num; i++) {
         if (vdev->vq[i].vring.desc) {
-            uint16_t nheads;
-
             /*
              * VIRTIO-1 devices migrate desc, used, and avail ring addresses so
              * only the region cache needs to be set up.  Legacy devices need
@@ -3157,16 +3155,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
                 continue;
             }
 
-            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) {
-                error_report("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);
-                return -1;
-            }
             vdev->vq[i].used_idx = vring_used_idx(&vdev->vq[i]);
             vdev->vq[i].shadow_avail_idx = vring_avail_idx(&vdev->vq[i]);
 
-- 
2.18.4

Re: [PATCH] virtio: skip guest index check on device load

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2852 bytes --]

On Mon, Oct 26, 2020 at 03:13:32PM +0000, Felipe Franciosi wrote:
> QEMU must be careful when loading device state off migration streams to
> prevent a malicious source from exploiting the emulator. Overdoing these
> checks has the side effect of allowing a guest to "pin itself" in cloud
> environments by messing with state which is entirely in its control.
> 
> Similarly to what f3081539 achieved in usb_device_post_load(), this
> commit removes such a check from virtio_load(). Worth noting, the result
> of a load without this check is the same as if a guest enables a VQ with
> invalid indexes to begin with. That is, the virtual device is set in a
> broken state (by the datapath handler) and must be reset.
> 
> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
> ---
>  hw/virtio/virtio.c | 12 ------------
>  1 file changed, 12 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 6f8f865aff..0561bdb857 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3136,8 +3136,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>      RCU_READ_LOCK_GUARD();
>      for (i = 0; i < num; i++) {
>          if (vdev->vq[i].vring.desc) {
> -            uint16_t nheads;
> -
>              /*
>               * VIRTIO-1 devices migrate desc, used, and avail ring addresses so
>               * only the region cache needs to be set up.  Legacy devices need
> @@ -3157,16 +3155,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>                  continue;
>              }
>  
> -            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) {
> -                error_report("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);
> -                return -1;
> -            }

Michael, the commit that introduced this check seems to have been for
debugging rather than to prevent a QEMU crash, so this removing the
check may be safe:

  commit 258dc7c96bb4b7ca71d5bee811e73933310e168c
  Author: Michael S. Tsirkin <mst@redhat.com>
  Date:   Sun Oct 17 20:23:48 2010 +0200

      virtio: sanity-check available index

      Checking available index upon load instead of
      only when vm is running makes is easier to
      debug failures.

Felipe: Did you audit the code to make sure the invalid avail_idx value
and the fields it is propagated to (e.g. shadow_avail_idx) are always
used in a safe way?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] virtio: skip guest index check on device load

[Michael S. Tsirkin]

On Tue, Oct 27, 2020 at 11:30:49AM +0000, Stefan Hajnoczi wrote:
> On Mon, Oct 26, 2020 at 03:13:32PM +0000, Felipe Franciosi wrote:
> > QEMU must be careful when loading device state off migration streams to
> > prevent a malicious source from exploiting the emulator. Overdoing these
> > checks has the side effect of allowing a guest to "pin itself" in cloud
> > environments by messing with state which is entirely in its control.
> > 
> > Similarly to what f3081539 achieved in usb_device_post_load(), this
> > commit removes such a check from virtio_load(). Worth noting, the result
> > of a load without this check is the same as if a guest enables a VQ with
> > invalid indexes to begin with. That is, the virtual device is set in a
> > broken state (by the datapath handler) and must be reset.
> > 
> > Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
> > ---
> >  hw/virtio/virtio.c | 12 ------------
> >  1 file changed, 12 deletions(-)
> > 
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 6f8f865aff..0561bdb857 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -3136,8 +3136,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
> >      RCU_READ_LOCK_GUARD();
> >      for (i = 0; i < num; i++) {
> >          if (vdev->vq[i].vring.desc) {
> > -            uint16_t nheads;
> > -
> >              /*
> >               * VIRTIO-1 devices migrate desc, used, and avail ring addresses so
> >               * only the region cache needs to be set up.  Legacy devices need
> > @@ -3157,16 +3155,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
> >                  continue;
> >              }
> >  
> > -            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) {
> > -                error_report("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);
> > -                return -1;
> > -            }
> 
> Michael, the commit that introduced this check seems to have been for
> debugging rather than to prevent a QEMU crash, so this removing the
> check may be safe:
> 
>   commit 258dc7c96bb4b7ca71d5bee811e73933310e168c
>   Author: Michael S. Tsirkin <mst@redhat.com>
>   Date:   Sun Oct 17 20:23:48 2010 +0200
> 
>       virtio: sanity-check available index
> 
>       Checking available index upon load instead of
>       only when vm is running makes is easier to
>       debug failures.

Agreed. Given this, let's keep the message around, just with
LOG_GUEST_ERROR ?


> Felipe: Did you audit the code to make sure the invalid avail_idx value
> and the fields it is propagated to (e.g. shadow_avail_idx) are always
> used in a safe way?

Re: [PATCH] virtio: skip guest index check on device load

[Felipe Franciosi]

> On Oct 27, 2020, at 12:25 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Tue, Oct 27, 2020 at 11:30:49AM +0000, Stefan Hajnoczi wrote:
>> On Mon, Oct 26, 2020 at 03:13:32PM +0000, Felipe Franciosi wrote:
>>> QEMU must be careful when loading device state off migration streams to
>>> prevent a malicious source from exploiting the emulator. Overdoing these
>>> checks has the side effect of allowing a guest to "pin itself" in cloud
>>> environments by messing with state which is entirely in its control.
>>> 
>>> Similarly to what f3081539 achieved in usb_device_post_load(), this
>>> commit removes such a check from virtio_load(). Worth noting, the result
>>> of a load without this check is the same as if a guest enables a VQ with
>>> invalid indexes to begin with. That is, the virtual device is set in a
>>> broken state (by the datapath handler) and must be reset.
>>> 
>>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
>>> ---
>>> hw/virtio/virtio.c | 12 ------------
>>> 1 file changed, 12 deletions(-)
>>> 
>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>> index 6f8f865aff..0561bdb857 100644
>>> --- a/hw/virtio/virtio.c
>>> +++ b/hw/virtio/virtio.c
>>> @@ -3136,8 +3136,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>>>     RCU_READ_LOCK_GUARD();
>>>     for (i = 0; i < num; i++) {
>>>         if (vdev->vq[i].vring.desc) {
>>> -            uint16_t nheads;
>>> -
>>>             /*
>>>              * VIRTIO-1 devices migrate desc, used, and avail ring addresses so
>>>              * only the region cache needs to be set up.  Legacy devices need
>>> @@ -3157,16 +3155,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>>>                 continue;
>>>             }
>>> 
>>> -            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) {
>>> -                error_report("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);
>>> -                return -1;
>>> -            }
>> 
>> Michael, the commit that introduced this check seems to have been for
>> debugging rather than to prevent a QEMU crash, so this removing the
>> check may be safe:
>> 
>>  commit 258dc7c96bb4b7ca71d5bee811e73933310e168c
>>  Author: Michael S. Tsirkin <mst@redhat.com>
>>  Date:   Sun Oct 17 20:23:48 2010 +0200
>> 
>>      virtio: sanity-check available index
>> 
>>      Checking available index upon load instead of
>>      only when vm is running makes is easier to
>>      debug failures.
> 
> Agreed. Given this, let's keep the message around, just with
> LOG_GUEST_ERROR ?

I thought about it. Happy to send a v2 which keeps the check and logs
without throwing an error.

Separately, I'm thinking of hooking up QEMU with VRING_ERR so datapath
handlers can notify QEMU that something went broken and NEEDS_RESET
can be flipped on the status register, possibly along a configuration
interrupt. I can see libvhost-user supports that, but are there any
reasons QEMU doesn't do this already?

> 
>> Felipe: Did you audit the code to make sure the invalid avail_idx value
>> and the fields it is propagated to (e.g. shadow_avail_idx) are always
>> used in a safe way?
> 

I did it briefly. I also wrote a mock userspace driver that creates
this condition in a very controlled way (so I can step half-way
through setting up VQs and trigger a migration, for example). But you
know how manual tests are... I may have missed something.
Your expert eyes are most welcome. :)

F.

Re: [PATCH] virtio: skip guest index check on device load

[Michael S. Tsirkin]

On Tue, Oct 27, 2020 at 12:53:29PM +0000, Felipe Franciosi wrote:
> 
> 
> > On Oct 27, 2020, at 12:25 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 
> > On Tue, Oct 27, 2020 at 11:30:49AM +0000, Stefan Hajnoczi wrote:
> >> On Mon, Oct 26, 2020 at 03:13:32PM +0000, Felipe Franciosi wrote:
> >>> QEMU must be careful when loading device state off migration streams to
> >>> prevent a malicious source from exploiting the emulator. Overdoing these
> >>> checks has the side effect of allowing a guest to "pin itself" in cloud
> >>> environments by messing with state which is entirely in its control.
> >>> 
> >>> Similarly to what f3081539 achieved in usb_device_post_load(), this
> >>> commit removes such a check from virtio_load(). Worth noting, the result
> >>> of a load without this check is the same as if a guest enables a VQ with
> >>> invalid indexes to begin with. That is, the virtual device is set in a
> >>> broken state (by the datapath handler) and must be reset.
> >>> 
> >>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
> >>> ---
> >>> hw/virtio/virtio.c | 12 ------------
> >>> 1 file changed, 12 deletions(-)
> >>> 
> >>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >>> index 6f8f865aff..0561bdb857 100644
> >>> --- a/hw/virtio/virtio.c
> >>> +++ b/hw/virtio/virtio.c
> >>> @@ -3136,8 +3136,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
> >>>     RCU_READ_LOCK_GUARD();
> >>>     for (i = 0; i < num; i++) {
> >>>         if (vdev->vq[i].vring.desc) {
> >>> -            uint16_t nheads;
> >>> -
> >>>             /*
> >>>              * VIRTIO-1 devices migrate desc, used, and avail ring addresses so
> >>>              * only the region cache needs to be set up.  Legacy devices need
> >>> @@ -3157,16 +3155,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
> >>>                 continue;
> >>>             }
> >>> 
> >>> -            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) {
> >>> -                error_report("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);
> >>> -                return -1;
> >>> -            }
> >> 
> >> Michael, the commit that introduced this check seems to have been for
> >> debugging rather than to prevent a QEMU crash, so this removing the
> >> check may be safe:
> >> 
> >>  commit 258dc7c96bb4b7ca71d5bee811e73933310e168c
> >>  Author: Michael S. Tsirkin <mst@redhat.com>
> >>  Date:   Sun Oct 17 20:23:48 2010 +0200
> >> 
> >>      virtio: sanity-check available index
> >> 
> >>      Checking available index upon load instead of
> >>      only when vm is running makes is easier to
> >>      debug failures.
> > 
> > Agreed. Given this, let's keep the message around, just with
> > LOG_GUEST_ERROR ?
> 
> I thought about it. Happy to send a v2 which keeps the check and logs
> without throwing an error.
> 
> Separately, I'm thinking of hooking up QEMU with VRING_ERR so datapath
> handlers can notify QEMU that something went broken and NEEDS_RESET
> can be flipped on the status register, possibly along a configuration
> interrupt. I can see libvhost-user supports that, but are there any
> reasons QEMU doesn't do this already?

Mostly because guest support isn't there. That in turn isn't easy,
lots of synchronization is needed within guests.


> > 
> >> Felipe: Did you audit the code to make sure the invalid avail_idx value
> >> and the fields it is propagated to (e.g. shadow_avail_idx) are always
> >> used in a safe way?
> > 
> 
> I did it briefly. I also wrote a mock userspace driver that creates
> this condition in a very controlled way (so I can step half-way
> through setting up VQs and trigger a migration, for example). But you
> know how manual tests are... I may have missed something.
> Your expert eyes are most welcome. :)
> 
> F.

Re: [PATCH] virtio: skip guest index check on device load

[Felipe Franciosi]

> On Oct 27, 2020, at 12:56 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Tue, Oct 27, 2020 at 12:53:29PM +0000, Felipe Franciosi wrote:
>> 
>> 
>>> On Oct 27, 2020, at 12:25 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> 
>>> On Tue, Oct 27, 2020 at 11:30:49AM +0000, Stefan Hajnoczi wrote:
>>>> On Mon, Oct 26, 2020 at 03:13:32PM +0000, Felipe Franciosi wrote:
>>>>> QEMU must be careful when loading device state off migration streams to
>>>>> prevent a malicious source from exploiting the emulator. Overdoing these
>>>>> checks has the side effect of allowing a guest to "pin itself" in cloud
>>>>> environments by messing with state which is entirely in its control.
>>>>> 
>>>>> Similarly to what f3081539 achieved in usb_device_post_load(), this
>>>>> commit removes such a check from virtio_load(). Worth noting, the result
>>>>> of a load without this check is the same as if a guest enables a VQ with
>>>>> invalid indexes to begin with. That is, the virtual device is set in a
>>>>> broken state (by the datapath handler) and must be reset.
>>>>> 
>>>>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
>>>>> ---
>>>>> hw/virtio/virtio.c | 12 ------------
>>>>> 1 file changed, 12 deletions(-)
>>>>> 
>>>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>>>> index 6f8f865aff..0561bdb857 100644
>>>>> --- a/hw/virtio/virtio.c
>>>>> +++ b/hw/virtio/virtio.c
>>>>> @@ -3136,8 +3136,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>>>>>    RCU_READ_LOCK_GUARD();
>>>>>    for (i = 0; i < num; i++) {
>>>>>        if (vdev->vq[i].vring.desc) {
>>>>> -            uint16_t nheads;
>>>>> -
>>>>>            /*
>>>>>             * VIRTIO-1 devices migrate desc, used, and avail ring addresses so
>>>>>             * only the region cache needs to be set up.  Legacy devices need
>>>>> @@ -3157,16 +3155,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>>>>>                continue;
>>>>>            }
>>>>> 
>>>>> -            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) {
>>>>> -                error_report("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);
>>>>> -                return -1;
>>>>> -            }
>>>> 
>>>> Michael, the commit that introduced this check seems to have been for
>>>> debugging rather than to prevent a QEMU crash, so this removing the
>>>> check may be safe:
>>>> 
>>>> commit 258dc7c96bb4b7ca71d5bee811e73933310e168c
>>>> Author: Michael S. Tsirkin <mst@redhat.com>
>>>> Date:   Sun Oct 17 20:23:48 2010 +0200
>>>> 
>>>>     virtio: sanity-check available index
>>>> 
>>>>     Checking available index upon load instead of
>>>>     only when vm is running makes is easier to
>>>>     debug failures.
>>> 
>>> Agreed. Given this, let's keep the message around, just with
>>> LOG_GUEST_ERROR ?
>> 
>> I thought about it. Happy to send a v2 which keeps the check and logs
>> without throwing an error.
>> 
>> Separately, I'm thinking of hooking up QEMU with VRING_ERR so datapath
>> handlers can notify QEMU that something went broken and NEEDS_RESET
>> can be flipped on the status register, possibly along a configuration
>> interrupt. I can see libvhost-user supports that, but are there any
>> reasons QEMU doesn't do this already?
> 
> Mostly because guest support isn't there. That in turn isn't easy,
> lots of synchronization is needed within guests.

Do you mean guest support to reset when seeing that bit in status
following the configuration interrupt?

It should be safe, though. I can have a stab to see if it breaks
Windows/Linux at least, and share an RFC if I get anywhere.

Unless you think it's a waste of time. Ideally guests shouldn't find
themselves in this situation to begin with, and if they did, resetting
would arguably just lead them into corruption again (for example). But
it does provide a mechanism for QEMU to find out that the vhost
backend stopped. That would help in the context of this patch.

F.

> 
> 
>>> 
>>>> Felipe: Did you audit the code to make sure the invalid avail_idx value
>>>> and the fields it is propagated to (e.g. shadow_avail_idx) are always
>>>> used in a safe way?
>>> 
>> 
>> I did it briefly. I also wrote a mock userspace driver that creates
>> this condition in a very controlled way (so I can step half-way
>> through setting up VQs and trigger a migration, for example). But you
>> know how manual tests are... I may have missed something.
>> Your expert eyes are most welcome. :)
>> 
>> F.
> 

Re: [PATCH] virtio: skip guest index check on device load

[Michael S. Tsirkin]

On Tue, Oct 27, 2020 at 01:02:59PM +0000, Felipe Franciosi wrote:
> 
> 
> > On Oct 27, 2020, at 12:56 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 
> > On Tue, Oct 27, 2020 at 12:53:29PM +0000, Felipe Franciosi wrote:
> >> 
> >> 
> >>> On Oct 27, 2020, at 12:25 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>> 
> >>> On Tue, Oct 27, 2020 at 11:30:49AM +0000, Stefan Hajnoczi wrote:
> >>>> On Mon, Oct 26, 2020 at 03:13:32PM +0000, Felipe Franciosi wrote:
> >>>>> QEMU must be careful when loading device state off migration streams to
> >>>>> prevent a malicious source from exploiting the emulator. Overdoing these
> >>>>> checks has the side effect of allowing a guest to "pin itself" in cloud
> >>>>> environments by messing with state which is entirely in its control.
> >>>>> 
> >>>>> Similarly to what f3081539 achieved in usb_device_post_load(), this
> >>>>> commit removes such a check from virtio_load(). Worth noting, the result
> >>>>> of a load without this check is the same as if a guest enables a VQ with
> >>>>> invalid indexes to begin with. That is, the virtual device is set in a
> >>>>> broken state (by the datapath handler) and must be reset.
> >>>>> 
> >>>>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
> >>>>> ---
> >>>>> hw/virtio/virtio.c | 12 ------------
> >>>>> 1 file changed, 12 deletions(-)
> >>>>> 
> >>>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >>>>> index 6f8f865aff..0561bdb857 100644
> >>>>> --- a/hw/virtio/virtio.c
> >>>>> +++ b/hw/virtio/virtio.c
> >>>>> @@ -3136,8 +3136,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
> >>>>>    RCU_READ_LOCK_GUARD();
> >>>>>    for (i = 0; i < num; i++) {
> >>>>>        if (vdev->vq[i].vring.desc) {
> >>>>> -            uint16_t nheads;
> >>>>> -
> >>>>>            /*
> >>>>>             * VIRTIO-1 devices migrate desc, used, and avail ring addresses so
> >>>>>             * only the region cache needs to be set up.  Legacy devices need
> >>>>> @@ -3157,16 +3155,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
> >>>>>                continue;
> >>>>>            }
> >>>>> 
> >>>>> -            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) {
> >>>>> -                error_report("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);
> >>>>> -                return -1;
> >>>>> -            }
> >>>> 
> >>>> Michael, the commit that introduced this check seems to have been for
> >>>> debugging rather than to prevent a QEMU crash, so this removing the
> >>>> check may be safe:
> >>>> 
> >>>> commit 258dc7c96bb4b7ca71d5bee811e73933310e168c
> >>>> Author: Michael S. Tsirkin <mst@redhat.com>
> >>>> Date:   Sun Oct 17 20:23:48 2010 +0200
> >>>> 
> >>>>     virtio: sanity-check available index
> >>>> 
> >>>>     Checking available index upon load instead of
> >>>>     only when vm is running makes is easier to
> >>>>     debug failures.
> >>> 
> >>> Agreed. Given this, let's keep the message around, just with
> >>> LOG_GUEST_ERROR ?
> >> 
> >> I thought about it. Happy to send a v2 which keeps the check and logs
> >> without throwing an error.
> >> 
> >> Separately, I'm thinking of hooking up QEMU with VRING_ERR so datapath
> >> handlers can notify QEMU that something went broken and NEEDS_RESET
> >> can be flipped on the status register, possibly along a configuration
> >> interrupt. I can see libvhost-user supports that, but are there any
> >> reasons QEMU doesn't do this already?
> > 
> > Mostly because guest support isn't there. That in turn isn't easy,
> > lots of synchronization is needed within guests.
> 
> Do you mean guest support to reset when seeing that bit in status
> following the configuration interrupt?
> 
> It should be safe, though. I can have a stab to see if it breaks
> Windows/Linux at least, and share an RFC if I get anywhere.
> 
> Unless you think it's a waste of time. Ideally guests shouldn't find
> themselves in this situation to begin with, and if they did, resetting
> would arguably just lead them into corruption again (for example). But
> it does provide a mechanism for QEMU to find out that the vhost
> backend stopped. That would help in the context of this patch.
> 
> F.


It's not a waste of time, it's just a lot of work
within guests.

> > 
> > 
> >>> 
> >>>> Felipe: Did you audit the code to make sure the invalid avail_idx value
> >>>> and the fields it is propagated to (e.g. shadow_avail_idx) are always
> >>>> used in a safe way?
> >>> 
> >> 
> >> I did it briefly. I also wrote a mock userspace driver that creates
> >> this condition in a very controlled way (so I can step half-way
> >> through setting up VQs and trigger a migration, for example). But you
> >> know how manual tests are... I may have missed something.
> >> Your expert eyes are most welcome. :)
> >> 
> >> F.
> > 

Re: [PATCH] virtio: skip guest index check on device load

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 939 bytes --]

On Tue, Oct 27, 2020 at 09:04:46AM -0400, Michael S. Tsirkin wrote:
> It's not a waste of time, it's just a lot of work
> within guests.

Luckily it does no harm to set the NEEDS_RESET bit even if the guest
doesn't handle it.

If the guest driver is unaware it may continue to submit requests to the
device for a while. The device emulation code stops accepting new
requests though. This means the device will become unresponsive until
reset, which is not ideal but okay in the case where the device was put
into an invalid state.

I agree that supporting NEEDS_RESET transparently inside guests is
difficult. The driver needs to reset and resume the device without
reporting errors to applications. In some cases drivers may not have
enough state in order to do that. It's also tricky to test all code
paths. I guess this is why no one has done it: drivers shouldn't enter
the NEEDS_RESET state anyway and handling it is complex.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] virtio: skip guest index check on device load

[Michael S. Tsirkin]

On Wed, Oct 28, 2020 at 11:00:38AM +0000, Stefan Hajnoczi wrote:
> On Tue, Oct 27, 2020 at 09:04:46AM -0400, Michael S. Tsirkin wrote:
> > It's not a waste of time, it's just a lot of work
> > within guests.
> 
> Luckily it does no harm to set the NEEDS_RESET bit even if the guest
> doesn't handle it.
> 
> If the guest driver is unaware it may continue to submit requests to the
> device for a while. The device emulation code stops accepting new
> requests though. This means the device will become unresponsive until
> reset, which is not ideal but okay in the case where the device was put
> into an invalid state.

There is no actual rule that device must stop processing requests.
Driver can only assume that is the case after it started the
actual reset.


> I agree that supporting NEEDS_RESET transparently inside guests is
> difficult. The driver needs to reset and resume the device without
> reporting errors to applications. In some cases drivers may not have
> enough state in order to do that. It's also tricky to test all code
> paths. I guess this is why no one has done it: drivers shouldn't enter
> the NEEDS_RESET state anyway and handling it is complex.
> 
> Stefan

Re: [PATCH] virtio: skip guest index check on device load

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1127 bytes --]

On Wed, Oct 28, 2020 at 07:30:55AM -0400, Michael S. Tsirkin wrote:
> On Wed, Oct 28, 2020 at 11:00:38AM +0000, Stefan Hajnoczi wrote:
> > On Tue, Oct 27, 2020 at 09:04:46AM -0400, Michael S. Tsirkin wrote:
> > > It's not a waste of time, it's just a lot of work
> > > within guests.
> > 
> > Luckily it does no harm to set the NEEDS_RESET bit even if the guest
> > doesn't handle it.
> > 
> > If the guest driver is unaware it may continue to submit requests to the
> > device for a while. The device emulation code stops accepting new
> > requests though. This means the device will become unresponsive until
> > reset, which is not ideal but okay in the case where the device was put
> > into an invalid state.
> 
> There is no actual rule that device must stop processing requests.
> Driver can only assume that is the case after it started the
> actual reset.

Right, I mean QEMU's implementation stops processing new requests once
the device has been marked "broken":

  void *virtqueue_pop(VirtQueue *vq, size_t sz)
  {
      if (virtio_device_disabled(vq->vdev)) { <---
          return NULL;

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] virtio: skip guest index check on device load

[Dr. David Alan Gilbert]

* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> On Tue, Oct 27, 2020 at 09:04:46AM -0400, Michael S. Tsirkin wrote:
> > It's not a waste of time, it's just a lot of work
> > within guests.
> 
> Luckily it does no harm to set the NEEDS_RESET bit even if the guest
> doesn't handle it.
> 
> If the guest driver is unaware it may continue to submit requests to the
> device for a while. The device emulation code stops accepting new
> requests though. This means the device will become unresponsive until
> reset, which is not ideal but okay in the case where the device was put
> into an invalid state.
> 
> I agree that supporting NEEDS_RESET transparently inside guests is
> difficult. The driver needs to reset and resume the device without
> reporting errors to applications.

Is that required?  I mean, what are the semantics of NEEDS_RESET - is
it assuming that you must be able to do a silent recovery?

Dave


> In some cases drivers may not have
> enough state in order to do that. It's also tricky to test all code
> paths. I guess this is why no one has done it: drivers shouldn't enter
> the NEEDS_RESET state anyway and handling it is complex.
> 
> Stefan


-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK