From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54939) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fabVJ-0005k9-2s for qemu-devel@nongnu.org; Wed, 04 Jul 2018 02:43:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fabVE-0000c4-6V for qemu-devel@nongnu.org; Wed, 04 Jul 2018 02:43:05 -0400 Received: from 3.mo179.mail-out.ovh.net ([178.33.251.175]:41351) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1fabVE-0000bM-06 for qemu-devel@nongnu.org; Wed, 04 Jul 2018 02:43:00 -0400 Received: from player738.ha.ovh.net (unknown [10.109.120.89]) by mo179.mail-out.ovh.net (Postfix) with ESMTP id 01D36D6CC8 for ; Wed, 4 Jul 2018 08:42:57 +0200 (CEST) References: <20180701171953.9921-1-clg@kaod.org> <20180702035726.GK2455@xz-mi> <20180704022648.GA2568@xz-mi> From: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= Message-ID: Date: Wed, 4 Jul 2018 08:42:47 +0200 MIME-Version: 1.0 In-Reply-To: <20180704022648.GA2568@xz-mi> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] exec.c: check RAMBlock validity before changing its flag List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Xu Cc: qemu-devel@nongnu.org, "Dr . David Alan Gilbert" , Peter Maydell , "Michael S. Tsirkin" , Alex Williamson , Paolo Bonzini , David Gibson On 07/04/2018 04:26 AM, Peter Xu wrote: > On Tue, Jul 03, 2018 at 02:45:24PM +0200, C=C3=A9dric Le Goater wrote: >> On 07/02/2018 05:57 AM, Peter Xu wrote: >>> On Sun, Jul 01, 2018 at 07:19:53PM +0200, C=C3=A9dric Le Goater wrote= : >>>> When a PCI device is unplugged, the PCI memory regions are deleted >>>> before the optional ROM RAMBlock is flagged non-migratable. But, whe= n >>>> this is done, the RAMBlock has already been cleared from the region, >>>> leading to a segv. >>>> >>>> Fix the issue by testing the RAMBlock before flagging it, as it is >>>> done in qemu_ram_unset_idstr() >>>> >>>> Signed-off-by: C=C3=A9dric Le Goater >>>> --- >>>> >>>> I caught this bug while deleting a passthrough device from a pserie= s >>>> machine. Here is the stack: >>>> =20 >>>> #0 qemu_ram_unset_migratable (rb=3D0x0) at /home/legoater/work/q= emu/qemu-xive-3.0.git/exec.c:1994 >>>> #1 0x000000010072def0 in vmstate_unregister_ram (mr=3D0x101796af= 0, dev=3D) >>>> #2 0x0000000100694e5c in pci_del_option_rom (pdev=3D0x101796330) >>>> #3 pci_qdev_unrealize (dev=3D, errp=3D) >>>> #4 0x00000001005ff910 in device_set_realized (obj=3D0x101796330,= value=3D, errp=3D0x0) >>>> #5 0x00000001007a487c in property_set_bool (obj=3D0x101796330, v= =3D, name=3D,=20 >>>> #6 0x00000001007a7878 in object_property_set (obj=3D0x101796330,= v=3D0x7fff70033110,=20 >>>> #7 0x00000001007aaf1c in object_property_set_qobject (obj=3D0x10= 1796330, value=3D,=20 >>>> #8 0x00000001007a7b90 in object_property_set_bool (obj=3D0x10179= 6330, value=3D,=20 >>>> #9 0x00000001005fcdd8 in device_unparent (obj=3D0x101796330) >>>> #10 0x00000001007a6dd0 in object_finalize_child_property (obj=3D<= optimized out>, name=3D,=20 >>>> #11 0x00000001007a50c0 in object_property_del_child (obj=3D0x1011= 1f800, child=3D0x101796330,=20 >>>> #12 0x0000000100425cc0 in spapr_phb_remove_pci_device_cb (dev=3D0= x101796330) >>>> #13 0x0000000100427974 in spapr_drc_release (drc=3D0x1017e2df0) >>>> #14 0x0000000100429098 in spapr_drc_detach (drc=3D0x1017e2df0) >>>> #15 0x00000001004294e0 in drc_isolate_physical (drc=3D0x1017e2df0= ) >>>> #16 0x000000010042a50c in rtas_set_isolation_state (state=3D0, id= x=3D) >>>> >>>> May be we should call pci_del_option_rom() before >>>> pci_unregister_io_regions() ? >>> >>> This seems to make more sense to me. >>> >>> Meanwhile I assume the name pci_del_option_rom() is a bit misleading = - >>> it's not really deleting the ROM but unregistering the ROM only. >>> Instead IIUC it's pci_unregister_io_regions() which deleted that. So >>> maybe we can either rename the function pci_del_option_rom(), or we >>> can pick the ROM destruction out of pci_unregister_io_regions() and >>> put it into pci_del_option_rom() to make sure it's done as the last >>> step? >> >> So it is a little more complex than I thought.=20 >> >> The PCI device is a vfio PCI device and the PCI ROM region is initiali= zed=20 >> in vfio_pci_size_rom() with memory_region_init_io(), which does not=20 >> allocate the RAMBlock, but has_rom is still set to true.=20 >> >> When the device is deleted, pci_del_option_rom() is called and with it= , vmstate_unregister_ram() because has_rom is set to true. Leading to the >> SEGV. >> >> I am not sure how to handle this case. It seems that the realize routi= ne=20 >> of VFIOPCIDevice is hijacking a little the PCIDevice layer. >=20 > Indeed. >=20 > Then now I'm a bit confused on who actually deleted the ROM memory > region that was created when pci_add_option_rom() was called. It > seems to be leaked. >=20 > AFAIU the rest of the memory regions of the BARs (0-5) are managed by > specific device emulation code, however this ROM memory region is > managed by PCI subsystem. Not sure whether that means we should > destroy the region in PCI subsystem too, e.g. in pci_del_option_rom(). >=20 > And now I see this patch might be a valid fix for the VFIO-specific > issue (though we might comment that a bit somewhere). yes. I will send a v2 with an updated commit log. Thanks, C.