From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40717) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X00fk-0005PK-E5 for qemu-devel@nongnu.org; Wed, 25 Jun 2014 23:48:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X00fd-0000s8-6A for qemu-devel@nongnu.org; Wed, 25 Jun 2014 23:48:28 -0400 Received: from mailout2.samsung.com ([203.254.224.25]:33801) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X00fc-0000re-S2 for qemu-devel@nongnu.org; Wed, 25 Jun 2014 23:48:21 -0400 Received: from epcpsbgm2.samsung.com (epcpsbgm2 [203.254.230.27]) by mailout2.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0N7R001WIBWCY090@mailout2.samsung.com> for qemu-devel@nongnu.org; Thu, 26 Jun 2014 12:48:12 +0900 (KST) From: SeokYeon Hwang References: <1403690348-19267-1-git-send-email-syeon.hwang@samsung.com> <20140625120330.GA13856@redhat.com> <1403699312.20031.46.camel@localhost.localdomain> In-reply-to: <1403699312.20031.46.camel@localhost.localdomain> Date: Thu, 26 Jun 2014 12:48:14 +0900 Message-id: <007a01cf90f1$752d87f0$5f8897d0$@samsung.com> MIME-version: 1.0 Content-type: text/plain; charset=UTF-8 Content-transfer-encoding: quoted-printable Content-language: ko Subject: Re: [Qemu-devel] [PATCH] pci: add checking whether the device is realized before unlinking the capability List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: marcel.a@redhat.com Cc: qemu-devel@nongnu.org, "'Michael S. Tsirkin'" > On Wed, 2014-06-25 at 15:03 +0300, Michael S. Tsirkin wrote: > > On Wed, Jun 25, 2014 at 06:59:08PM +0900, SeokYeon Hwang wrote: > > > In case of the unrealized "pdev", memory can be illegally accessed = and > corrupted. > > > Refer to device_unparent() in the commit > 5c21ce77d7e5643089ceec556c0408445d017f32. > Hi, > Thank you for submitting the patch. >=20 > Can you please send to the list how to reproduce the issue? > Is this a regression? Before the above commit did it work? Hi, When VirtIO device is hot-unplugged, Qemu unrealizes the device first = then child_bus by device_unparent(). (After the commit 5c21ce77d7e5643089ceec556c0408445d017f32.) For example, if I suppose to hot-unplug "virtio block" device, unparent = "virtio-blk-pci" first, then "virtio-blk". virtio_bus_device_unplugged() function in virtio-bus.c calls = "msix_uninit()" with the parameter of its parent PCI device. The problem is that it accesses already freed member of pdev, such as = "pdev->config". This illegal access causes the heap corruption, and it kills Qemu = process suddenly. You can detect this illegal access by using "electric fence" or = "valgrind". >=20 > > > > > > Change-Id: Iacb195a092c86d4c677ad0404582af104b2251ae > > > Signed-off-by: SeokYeon Hwang > > > > Another case of qemu mailing list dropping patches :( I will bounce = it > > now so people can see the original message. > > > > > > > --- > > > hw/pci/pci.c | 7 ++++++- > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 49eca95..bb7f0c5 > > > 100644 > > > --- a/hw/pci/pci.c > > > +++ b/hw/pci/pci.c > > > @@ -2056,7 +2056,12 @@ int pci_add_capability(PCIDevice *pdev, > > > uint8_t cap_id, > > > /* Unlink capability from the pci config space. */ void > > > pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size) = { > > > - uint8_t prev, offset =3D pci_find_capability_list(pdev, = cap_id, > &prev); > > > + uint8_t prev, offset; > > > + /* Check whether the device is realized or not */ > > > + if (!pdev->qdev.realized) { > The 'qdev' field and 'realize' property are private, you should use = one of > QOM's 'property_get' methods. >=20 > I am also concerned about adding the realize check here, it seems too > "deep" in implementation, seeing this checks everywhere doesn't seem a > good idea. > I think we should first understand the root cause and try making this > change in a higher level. >=20 > I hope I helped, > Marcel Ok, I see. I think you're right. If needed, I will send an another patch using = "object_property_get_bool()". Thanks. >=20 >=20 > > > + return; > > > + } > > > + offset =3D pci_find_capability_list(pdev, cap_id, &prev); > > > if (!offset) > > > return; > > > pdev->config[prev] =3D pdev->config[offset + = PCI_CAP_LIST_NEXT]; > > > -- > > > 1.9.1 > > >=20