On Mon, 19 Sep 2016 21:27:45 +0200 Laszlo Ersek <ler...@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 <stefa...@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