From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex Williamson Subject: Re: [PATCH] kvm: deassign irqs in reset path Date: Fri, 30 Mar 2012 15:09:11 -0600 Message-ID: <1333141751.29235.31.camel@ul30vt> References: <201203301918.q2UJI63c005908@int-mx02.intmail.prod.int.phx2.redhat.com> <4F760993.304@web.de> <20120330201313.GB2376@redhat.com> <4F761517.6010105@web.de> <20120330203140.GC2376@redhat.com> <4F7618FC.7040807@web.de> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: Jason Baron , qemu-devel@nongnu.org, kvm@vger.kernel.org, mst@redhat.com To: Jan Kiszka Return-path: In-Reply-To: <4F7618FC.7040807@web.de> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org List-Id: kvm.vger.kernel.org On Fri, 2012-03-30 at 22:35 +0200, Jan Kiszka wrote: > On 2012-03-30 22:31, Jason Baron wrote: > > On Fri, Mar 30, 2012 at 10:18:31PM +0200, Jan Kiszka wrote: > >>>>> The root cause of the problem is that the 'reset_assigned_device()' code > >>>>> first writes a 0 to the command register. Then, when qemu subsequently does > >>>>> a kvm_deassign_irq() (called by assign_irq(), in the system_reset path), > >>>>> the kernel ends up calling '__msix_mask_irq()', which performs a write to > >>>>> the memory mapped msi vector space. Since, we've explicitly told the device > >>>>> to disallow mmio access (via the 0 write to the command register), we end > >>>>> up with the above 'Unsupported Request'. > >>>>> > >>>>> The fix here is to first call kvm_deassign_irq(), before doing the reset, > >>>> > >>>> s/fix/workaround/. This is a kernel bug if userspace can crash the > >>>> system like this, no? Let's fix the kernel first and then look at what > >>>> needs to be changed here. > >>>> > >>>> Jan > >>>> > >>> > >>> But don't I need special privalege to run the device assignment bits? > >> > >> Yes, but even that might be moderated by a management component like > >> libvirt. > >> > >>> For example, this crash is precipitated by a write of '0' to the pci > >>> device config register from userspace. Surely, not every is allowed to > >>> do that write. So it seems to me, that this patch is in keeping with the > >>> current model of how things work. > >> > >> No user should needlessly be able to crash the host by issuing valid > >> commands in a special order. > >> > >> Jan > >> > > > > Right, but as I see device-assign.c, we are essentially programming the > > pci device directly from userspace. Put another way, the kernel could > > crash the system if it programmed a pci device in the wrong order. So I > > don't see how this is different. But maybe I'm misunderstanding the > > model here? > > The model is that the KVM device assignment subsystem in the kernel (or > vfio in the future) + the IOMMU confine what userspace (not the qemu > guest) can do with the device and prevent that any harm is caused to the > system. There might be practical holes in this model, but this is still > what we are striving for. Jan, It's possible to cause this same crash w/o device assignment involved. Use setpci to zero the command register, then set smp_affinity on the device using MSI-X. It's the right thing to do for qemu reset to return device interrupts to a known state, even if it does just happen to be a workaround for this problem. Given that we're using a generic interface for config space, I'm not sure how practical it is to start arbitrarily picking certain things as bad and making them inaccessible (there are plenty of targets in PCI config space on x86). Thanks, Alex From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:51824) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SDj4Q-0001LY-Gl for qemu-devel@nongnu.org; Fri, 30 Mar 2012 17:09:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SDj4M-0008HR-Tv for qemu-devel@nongnu.org; Fri, 30 Mar 2012 17:09:18 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55589) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SDj4M-0008Gx-L7 for qemu-devel@nongnu.org; Fri, 30 Mar 2012 17:09:14 -0400 Message-ID: <1333141751.29235.31.camel@ul30vt> From: Alex Williamson Date: Fri, 30 Mar 2012 15:09:11 -0600 In-Reply-To: <4F7618FC.7040807@web.de> References: <201203301918.q2UJI63c005908@int-mx02.intmail.prod.int.phx2.redhat.com> <4F760993.304@web.de> <20120330201313.GB2376@redhat.com> <4F761517.6010105@web.de> <20120330203140.GC2376@redhat.com> <4F7618FC.7040807@web.de> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Mime-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] kvm: deassign irqs in reset path List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: Jason Baron , qemu-devel@nongnu.org, kvm@vger.kernel.org, mst@redhat.com On Fri, 2012-03-30 at 22:35 +0200, Jan Kiszka wrote: > On 2012-03-30 22:31, Jason Baron wrote: > > On Fri, Mar 30, 2012 at 10:18:31PM +0200, Jan Kiszka wrote: > >>>>> The root cause of the problem is that the 'reset_assigned_device()' code > >>>>> first writes a 0 to the command register. Then, when qemu subsequently does > >>>>> a kvm_deassign_irq() (called by assign_irq(), in the system_reset path), > >>>>> the kernel ends up calling '__msix_mask_irq()', which performs a write to > >>>>> the memory mapped msi vector space. Since, we've explicitly told the device > >>>>> to disallow mmio access (via the 0 write to the command register), we end > >>>>> up with the above 'Unsupported Request'. > >>>>> > >>>>> The fix here is to first call kvm_deassign_irq(), before doing the reset, > >>>> > >>>> s/fix/workaround/. This is a kernel bug if userspace can crash the > >>>> system like this, no? Let's fix the kernel first and then look at what > >>>> needs to be changed here. > >>>> > >>>> Jan > >>>> > >>> > >>> But don't I need special privalege to run the device assignment bits? > >> > >> Yes, but even that might be moderated by a management component like > >> libvirt. > >> > >>> For example, this crash is precipitated by a write of '0' to the pci > >>> device config register from userspace. Surely, not every is allowed to > >>> do that write. So it seems to me, that this patch is in keeping with the > >>> current model of how things work. > >> > >> No user should needlessly be able to crash the host by issuing valid > >> commands in a special order. > >> > >> Jan > >> > > > > Right, but as I see device-assign.c, we are essentially programming the > > pci device directly from userspace. Put another way, the kernel could > > crash the system if it programmed a pci device in the wrong order. So I > > don't see how this is different. But maybe I'm misunderstanding the > > model here? > > The model is that the KVM device assignment subsystem in the kernel (or > vfio in the future) + the IOMMU confine what userspace (not the qemu > guest) can do with the device and prevent that any harm is caused to the > system. There might be practical holes in this model, but this is still > what we are striving for. Jan, It's possible to cause this same crash w/o device assignment involved. Use setpci to zero the command register, then set smp_affinity on the device using MSI-X. It's the right thing to do for qemu reset to return device interrupts to a known state, even if it does just happen to be a workaround for this problem. Given that we're using a generic interface for config space, I'm not sure how practical it is to start arbitrarily picking certain things as bad and making them inaccessible (there are plenty of targets in PCI config space on x86). Thanks, Alex