From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51124) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmHKa-0004e1-8R for qemu-devel@nongnu.org; Tue, 20 Sep 2016 05:27:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bmHKW-0002AP-0P for qemu-devel@nongnu.org; Tue, 20 Sep 2016 05:27:11 -0400 Received: from 8.mo1.mail-out.ovh.net ([178.33.110.239]:32900) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmHKV-00029D-OZ for qemu-devel@nongnu.org; Tue, 20 Sep 2016 05:27:07 -0400 Received: from player770.ha.ovh.net (b7.ovh.net [213.186.33.57]) by mo1.mail-out.ovh.net (Postfix) with ESMTP id D6F7C726B for ; Tue, 20 Sep 2016 11:27:05 +0200 (CEST) Date: Tue, 20 Sep 2016 11:26:57 +0200 From: Greg Kurz Message-ID: <20160920112657.39cf44ab@bahia> In-Reply-To: <983e6ea4-c0fd-fad1-9027-c913c2faf796@redhat.com> References: <1460467534-29147-1-git-send-email-stefanha@redhat.com> <20160919180740.6a21408e.cornelia.huck@de.ibm.com> <20160919205057-mutt-send-email-mst@kernel.org> <983e6ea4-c0fd-fad1-9027-c913c2faf796@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 00/10] virtio: avoid exit() when device enters invalid states List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laszlo Ersek Cc: "Michael S. Tsirkin" , Cornelia Huck , Fam Zheng , qemu-devel@nongnu.org, Stefan Hajnoczi , Prasad Pandit On Mon, 19 Sep 2016 21:27:45 +0200 Laszlo Ersek 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 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