Re: [Qemu-devel] [PATCH v3 00/10] virtio: avoid exit() when device enters invalid states

[Greg Kurz <groug@kaod.org>]

On Mon, 19 Sep 2016 21:27:45 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 09/19/16 19:51, Michael S. Tsirkin wrote:
> > On Mon, Sep 19, 2016 at 06:07:40PM +0200, Cornelia Huck wrote:  
> >> On Tue, 12 Apr 2016 14:25:24 +0100
> >> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >>  
> >>> v3:
> >>>  * Patch 1: Fix typo and clarify commit description [Markus]
> >>>  * Use virtio_set_status() instead of open coding assignment [Cornelia]
> >>>  * Add live migration
> >>>
> >>> v2:
> >>>  * Add VIRTIO_CONFIG_S_NEEDS_RESET notification for VIRTIO 1.0 [Cornelia]
> >>>    (Note I've sent a Linux virtio_config.h patch to get the constant added to
> >>>    the headers.)
> >>>  * Split int -> unsigned int change into separate commit [Fam]
> >>>  * Fix double "index" typo in commit description [Fam]
> >>>
> >>> The virtio code calls exit() when the device enters an invalid state.  This
> >>> means invalid vring indices and descriptor chains kill the VM.  See the patch
> >>> descriptions for why this is a bad thing.
> >>>
> >>> When the virtio device is in the broken state calls to virtqueue_pop() and
> >>> friends will pretend the virtqueue is empty.  This means the device will become
> >>> isolated from guest activity until it is reset again.
> >>>
> >>> RFC because two things are missing:
> >>> 1. Live migration support (subsection for broken flag?)
> >>> 2. Auditing devices and replacing exit() calls there too
> >>>
> >>> Stefan Hajnoczi (10):
> >>>   virtio: fix stray tab character
> >>>   include: update virtio_config.h Linux header
> >>>   virtio: stop virtqueue processing if device is broken
> >>>   virtio: migrate vdev->broken flag
> >>>   virtio: handle virtqueue_map_desc() errors
> >>>   virtio: handle virtqueue_get_avail_bytes() errors
> >>>   virtio: use unsigned int for virtqueue_get_avail_bytes() index
> >>>   virtio: handle virtqueue_read_next_desc() errors
> >>>   virtio: handle virtqueue_num_heads() errors
> >>>   virtio: handle virtqueue_get_head() errors
> >>>
> >>>  hw/virtio/virtio.c                             | 223 +++++++++++++++++++------
> >>>  include/hw/virtio/virtio.h                     |   3 +
> >>>  include/standard-headers/linux/virtio_config.h |   2 +
> >>>  3 files changed, 181 insertions(+), 47 deletions(-)
> >>>  
> >>
> >> As the exit-in-virtio question has popped up several times in the
> >> recent past: I think we should go forward with this series, even if we
> >> still need to look at the individual devices. Do you have a version
> >> that fits on current master?  
> > 
> > I agree.
> >   
> 
> NB, Prasad just posted a patch (v3 being the latest) that adds another
> such exit(1), at my suggestion.
> 
> [Qemu-devel] [PATCH v3] virtio: add check for descriptor's mapped
>                         address
> 
> So a rebase of this series should likely consider that patch as well.
> (But Stefan is aware anyway.)
> 
> Thanks!
> Laszlo
> 

Stefan's series still applies on the current head, except the virtio_config.h
patch which isn't needed anymore.

And indeed there are a bunch of places where QEMU exits:

[greg@bahia qemu-virtio]$ git grep 'exit(1)' hw/virtio hw/*/virtio*
hw/block/virtio-blk.c:        exit(1);
hw/block/virtio-blk.c:        exit(1);
hw/block/virtio-blk.c:        exit(1);
hw/net/virtio-net.c:            exit(1);
hw/net/virtio-net.c:            exit(1);
hw/net/virtio-net.c:            exit(1);
hw/net/virtio-net.c:            exit(1);
hw/net/virtio-net.c:                exit(1);
hw/scsi/virtio-scsi-dataplane.c:        exit(1);
hw/scsi/virtio-scsi.c:    exit(1);
hw/scsi/virtio-scsi.c:        exit(1);
hw/scsi/virtio-scsi.c:        exit(1);
hw/virtio/virtio.c:        exit(1);
hw/virtio/virtio.c:            exit(1);
hw/virtio/virtio.c:            exit(1);
hw/virtio/virtio.c:        exit(1);

And also even more places with assert() or BUG_ON(), some of which are
guest errors actually.

For example, in virtio-9p, we have:

static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
{
...
        len = iov_to_buf(elem->out_sg, elem->out_num, 0,
                          &out, sizeof out);
        BUG_ON(len != sizeof out);
...
}

The condition may only be true if the guest sent less than the expected
9P message header which is 7-byte long.

I have a patch for this based on Stefan's series BTW.

Cheers.

--
Greg