Re: [Qemu-devel] [PATCH v3 00/10] virtio: avoid exit() when device enters invalid states
On Mon, Sep 19, 2016 at 06:07:40PM +0200, Cornelia Huck wrote: > Do you have a version that fits on current master? Thanks for reminding me about this series. I will send a rebased version. Stefan signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH v3 00/10] virtio: avoid exit() when device enters invalid states
On Tue, 20 Sep 2016 11:26:57 +0200
Greg Kurz wrote:
> Stefan's series still applies on the current head, except the virtio_config.h
> patch which isn't needed anymore.
I went through the patches, series generally looks good to me.
>
> And indeed there are a bunch of places where QEMU exits:
Most of which should be converted to virtio_error(), except...
>
> [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);
...this one, which tests for a host misconfiguration, and...
> hw/scsi/virtio-scsi.c:exit(1);
> hw/scsi/virtio-scsi.c:exit(1);
...this one, which is a migration stream problem.
> 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.
Yes. Let's tackle them piece-by-piece.
>
> 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.
Cool.
Re: [Qemu-devel] [PATCH v3 00/10] virtio: avoid exit() when device enters invalid states
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
Re: [Qemu-devel] [PATCH v3 00/10] virtio: avoid exit() when device enters invalid states
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
Re: [Qemu-devel] [PATCH v3 00/10] virtio: avoid exit() when device enters invalid states
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.
Re: [Qemu-devel] [PATCH v3 00/10] virtio: avoid exit() when device enters invalid states
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?
