[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 2/2] virtio-scsi/virtio-blk: Disable poll handle
From: |
Peter Xu |
Subject: |
Re: [Qemu-block] [PATCH 2/2] virtio-scsi/virtio-blk: Disable poll handlers when stopping vq handler |
Date: |
Fri, 14 Sep 2018 10:45:53 +0800 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
On Thu, Sep 13, 2018 at 10:00:43AM -0600, Alex Williamson wrote:
> On Thu, 13 Sep 2018 12:04:34 +0200
> Paolo Bonzini <address@hidden> wrote:
>
> > On 13/09/2018 11:11, Paolo Bonzini wrote:
> > > On 13/09/2018 08:03, Fam Zheng wrote:
> > >> On Wed, 09/12 14:42, Paolo Bonzini wrote:
> > >>> On 12/09/2018 13:50, Fam Zheng wrote:
> > >>>>> I think it's okay if it is invoked. The sequence is first you stop
> > >>>>> the
> > >>>>> vq, then you drain the BlockBackends, then you switch AioContext. All
> > >>>>> that matters is the outcome when virtio_scsi_dataplane_stop returns.
> > >>>> Yes, but together with vIOMMU, it also effectively leads to a
> > >>>> virtio_error(),
> > >>>> which is not clean. QEMU stderr when this call happens (with patch 1
> > >>>> but not
> > >>>> this patch):
> > >>>>
> > >>>> 2018-09-12T11:48:10.193023Z qemu-system-x86_64: vtd_iommu_translate:
> > >>>> detected translation failure (dev=02:00:00, iova=0x0)
> > >>>> 2018-09-12T11:48:10.193044Z qemu-system-x86_64: New fault is not
> > >>>> recorded due to compression of faults
> > >>>> 2018-09-12T11:48:10.193061Z qemu-system-x86_64: virtio: zero sized
> > >>>> buffers are not allowed
> > >>>
> > >>> But with iothread, virtio_scsi_dataplane_stop runs in another thread
> > >>> than the iothread; in that case you still have a race where the iothread
> > >>> can process the vq before aio_disable_external and print the error.
> > >>>
> > >>> IIUC the guest has cleared the IOMMU page tables _before_ clearing the
> > >>> DRIVER_OK bit in the status field. Could this be a guest bug?
> > >>
> > >> I'm not sure if it is a bug or not. I think what happens is the device
> > >> is left
> > >> enabled by Seabios, and then reset by kernel.
> > >
> > > That makes sense, though I'm not sure why QEMU needs to process a
> > > request long after SeaBIOS has left control to Linux. Maybe it's just
> > > that the messages should not go on QEMU stderr, and rather trace-point
> > > should be enough.
> >
> > Aha, it's not that QEMU needs to poll, it's just that polling mode is
> > enabled, and it decides to do one last iteration. In general the virtio
> > spec allows the hardware to poll whenever it wants, hence:
> >
> > 1) I'm not sure that translation failures should mark the device as
> > broken---definitely not when doing polling, possibly not even in
> > response to the guest "kicking" the virtqueue. Alex, does the PCI spec
> > say anything about this?
>
> AFAIK the PCI spec doesn't define anything about the IOMMU or response
> to translation failures. Depending on whether it's a read or write,
> the device might see an unsupported request or not even be aware of the
> error. It's really a platform RAS question whether to have any more
> significant response, most don't, but at least one tends to consider
> IOMMU faults to be a data integrity issue worth bring the system down.
> We've struggled with handling ongoing DMA generating IOMMU faults
> during kexec for a long time, so any sort of marking a device broken
> for a fault should be thoroughly considered, especially when a device
> could be assigned to a user who can trivially trigger a fault.
>
> > 2) translation faliures should definitely not print messages to stderr.
>
> Yep, easy DoS vector for a malicious guest, or malicious userspace
> driver within the guest. Thanks,
Note that it's using error_report_once() upstream so it'll only print
once for the whole lifecycle of QEMU process, and it's still a
tracepoint downstream so no error will be dumped by default. So AFAIU
it's not a DoS target for either.
I would consider it a good hint for strange bugs since AFAIU DMA error
should never exist on well-behaved guests. However I'll be fine too
to post a patch to make it an explicit tracepoint again if any of us
would still like it to go away.
Thanks,
--
Peter Xu
- Re: [Qemu-block] [PATCH 2/2] virtio-scsi/virtio-blk: Disable poll handlers when stopping vq handler, (continued)
- Re: [Qemu-block] [PATCH 2/2] virtio-scsi/virtio-blk: Disable poll handlers when stopping vq handler, Fam Zheng, 2018/09/11
- Re: [Qemu-block] [PATCH 2/2] virtio-scsi/virtio-blk: Disable poll handlers when stopping vq handler, Paolo Bonzini, 2018/09/11
- Re: [Qemu-block] [PATCH 2/2] virtio-scsi/virtio-blk: Disable poll handlers when stopping vq handler, Fam Zheng, 2018/09/11
- Re: [Qemu-block] [PATCH 2/2] virtio-scsi/virtio-blk: Disable poll handlers when stopping vq handler, Paolo Bonzini, 2018/09/12
- Re: [Qemu-block] [PATCH 2/2] virtio-scsi/virtio-blk: Disable poll handlers when stopping vq handler, Fam Zheng, 2018/09/12
- Re: [Qemu-block] [PATCH 2/2] virtio-scsi/virtio-blk: Disable poll handlers when stopping vq handler, Paolo Bonzini, 2018/09/12
- Re: [Qemu-block] [PATCH 2/2] virtio-scsi/virtio-blk: Disable poll handlers when stopping vq handler, Fam Zheng, 2018/09/13
- Re: [Qemu-block] [PATCH 2/2] virtio-scsi/virtio-blk: Disable poll handlers when stopping vq handler, Paolo Bonzini, 2018/09/13
- Re: [Qemu-block] [PATCH 2/2] virtio-scsi/virtio-blk: Disable poll handlers when stopping vq handler, Paolo Bonzini, 2018/09/13
- Re: [Qemu-block] [PATCH 2/2] virtio-scsi/virtio-blk: Disable poll handlers when stopping vq handler, Alex Williamson, 2018/09/13
- Re: [Qemu-block] [PATCH 2/2] virtio-scsi/virtio-blk: Disable poll handlers when stopping vq handler,
Peter Xu <=