From nobody Sat May 11 11:58:36 2024 Delivered-To: importer@patchew.org Authentication-Results: mx.zohomail.com; spf=pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org; dmarc=pass(p=none dis=none) header.from=nongnu.org ARC-Seal: i=1; a=rsa-sha256; t=1682080008; cv=none; d=zohomail.com; s=zohoarc; b=bcj8KPmtFckqBsiEbp/VzNxVvu8A1rXCDImSwoIT0izLgbr4nMl+OOFWN+4XPqzcu95cPEZJdwgHxdztQzNltMdGLqiYsEhmsDus744prQEaWso0qOUNGr391zFyJbTskD9SMT8MkoInYcxk6Jyj2uXsdaj5A30mYEtCY/f8cik= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1682080008; h=Content-Type:Content-Transfer-Encoding:Cc:Date:From:List-Subscribe:List-Post:List-Id:List-Archive:List-Help:List-Unsubscribe:MIME-Version:Message-ID:Reply-To:Sender:Subject:To; bh=f2KYOkoSLscfOv/2h1+sYipxG1OhweIyr8an0dnv8iI=; b=buDQQq0ksdyLBZFaLEwdU7jvYr4/2rq4xL2FXHCij9tQrqrVmpcC/+nUGtVK1Wk6mwo9oEcdj2fQhdUFeNNKWu5wfK3EN+wnIpz9Up+onTfX8NHcThSB3ZITnCofFsZF7bVLeqf/i+dkHtbksornmsKKAyWZwkszGNHy3mGtccQ= ARC-Authentication-Results: i=1; mx.zohomail.com; spf=pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org; dmarc=pass header.from= (p=none dis=none) Return-Path: Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) by mx.zohomail.com with SMTPS id 1682080007957640.5949114968458; Fri, 21 Apr 2023 05:26:47 -0700 (PDT) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pppq9-0008Lo-OF; Fri, 21 Apr 2023 08:26:14 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ppppt-0008LI-BI for qemu-devel@nongnu.org; Fri, 21 Apr 2023 08:25:58 -0400 Received: from frasgout.his.huawei.com ([185.176.79.56]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ppppp-0001O8-Kp for qemu-devel@nongnu.org; Fri, 21 Apr 2023 08:25:56 -0400 Received: from lhrpeml500005.china.huawei.com (unknown [172.18.147.207]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4Q2txw6QB0z67m0K; Fri, 21 Apr 2023 20:24:36 +0800 (CST) Received: from SecurePC-101-06.china.huawei.com (10.122.247.231) by lhrpeml500005.china.huawei.com (7.191.163.240) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.23; Fri, 21 Apr 2023 13:25:47 +0100 To: , Peter Maydell CC: Igor Mammedov , "Michael S . Tsirkin" , Fan Ni , , Thomas Huth , =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Subject: [RFC PATCH v2] hw/pci-bridge: Fix release ordering by embedding PCIBridgeWindows within PCIBridge Date: Fri, 21 Apr 2023 13:25:50 +0100 Message-ID: <20230421122550.28234-1-Jonathan.Cameron@huawei.com> X-Mailer: git-send-email 2.37.2 MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable X-Originating-IP: [10.122.247.231] X-ClientProxiedBy: lhrpeml100003.china.huawei.com (7.191.160.210) To lhrpeml500005.china.huawei.com (7.191.163.240) X-CFilter-Loop: Reflected Received-SPF: pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) client-ip=209.51.188.17; envelope-from=qemu-devel-bounces+importer=patchew.org@nongnu.org; helo=lists.gnu.org; Received-SPF: pass client-ip=185.176.79.56; envelope-from=jonathan.cameron@huawei.com; helo=frasgout.his.huawei.com X-Spam_score_int: -41 X-Spam_score: -4.2 X-Spam_bar: ---- X-Spam_report: (-4.2 / 5.0 requ) BAYES_00=-1.9, RCVD_IN_DNSWL_MED=-2.3, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-to: Jonathan Cameron From: Jonathan Cameron via Errors-To: qemu-devel-bounces+importer=patchew.org@nongnu.org Sender: qemu-devel-bounces+importer=patchew.org@nongnu.org X-ZM-MESSAGEID: 1682080015238100001 The lifetime of the PCIBridgeWindows instance accessed via the windows poin= ter in struct PCIBridge is managed separately from the PCIBridge itself. Triggered by ./qemu-system-x86_64 -M x-remote -display none -monitor stdio QEMU monitor: device_add cxl-downstream In some error handling paths (such as the above due to attaching a cxl-down= stream port anything other than a cxl-upstream port) the g_free() of the PCIBridge windows in pci_bridge_region_cleanup() is called before the final call of flatview_uref() in address_space_set_flatview() ultimately from drain_call_rcu() At one stage this resulted in a crash, currently can still be observed using valgrind which records a use after free. When present, only one instance is allocated. pci_bridge_update_mappings() can operate directly on an instance rather than creating a new one and swapping it in. Thus there appears to be no reason to not directly couple the lifetimes of the two structures by embedding the PCIBridgeWindows within the PCIBridge removing the need for the problematic separate free. Patch is same as was posted deep in the discussion. https://lore.kernel.org/qemu-devel/20230403171232.000020bb@huawei.com/ Posted as an RFC as only lightly tested and I'm not sure what the reasoning behind the separation of lifetimes originally was. As such perhaps this is not the best route to fixing the issue. Reported-by: Thomas Huth Signed-off-by: Jonathan Cameron -- v2: Fix spurious include of rcu.h (Philippe Mathieu-Daud=C3=A9) Reviewed-by: Thomas Huth --- hw/pci/pci_bridge.c | 19 ++++++++----------- include/hw/pci/pci_bridge.h | 2 +- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c index dd5af508f9..e7b9345615 100644 --- a/hw/pci/pci_bridge.c +++ b/hw/pci/pci_bridge.c @@ -184,11 +184,11 @@ static void pci_bridge_init_vga_aliases(PCIBridge *br= , PCIBus *parent, } } =20 -static PCIBridgeWindows *pci_bridge_region_init(PCIBridge *br) +static void pci_bridge_region_init(PCIBridge *br) { PCIDevice *pd =3D PCI_DEVICE(br); PCIBus *parent =3D pci_get_bus(pd); - PCIBridgeWindows *w =3D g_new(PCIBridgeWindows, 1); + PCIBridgeWindows *w =3D &br->windows; uint16_t cmd =3D pci_get_word(pd->config + PCI_COMMAND); =20 pci_bridge_init_alias(br, &w->alias_pref_mem, @@ -211,8 +211,6 @@ static PCIBridgeWindows *pci_bridge_region_init(PCIBrid= ge *br) cmd & PCI_COMMAND_IO); =20 pci_bridge_init_vga_aliases(br, parent, w->alias_vga); - - return w; } =20 static void pci_bridge_region_del(PCIBridge *br, PCIBridgeWindows *w) @@ -234,19 +232,18 @@ static void pci_bridge_region_cleanup(PCIBridge *br, = PCIBridgeWindows *w) object_unparent(OBJECT(&w->alias_vga[QEMU_PCI_VGA_IO_LO])); object_unparent(OBJECT(&w->alias_vga[QEMU_PCI_VGA_IO_HI])); object_unparent(OBJECT(&w->alias_vga[QEMU_PCI_VGA_MEM])); - g_free(w); } =20 void pci_bridge_update_mappings(PCIBridge *br) { - PCIBridgeWindows *w =3D br->windows; + PCIBridgeWindows *w =3D &br->windows; =20 /* Make updates atomic to: handle the case of one VCPU updating the br= idge * while another accesses an unaffected region. */ memory_region_transaction_begin(); - pci_bridge_region_del(br, br->windows); + pci_bridge_region_del(br, w); pci_bridge_region_cleanup(br, w); - br->windows =3D pci_bridge_region_init(br); + pci_bridge_region_init(br); memory_region_transaction_commit(); } =20 @@ -385,7 +382,7 @@ void pci_bridge_initfn(PCIDevice *dev, const char *type= name) sec_bus->address_space_io =3D &br->address_space_io; memory_region_init(&br->address_space_io, OBJECT(br), "pci_bridge_io", 4 * GiB); - br->windows =3D pci_bridge_region_init(br); + pci_bridge_region_init(br); QLIST_INIT(&sec_bus->child); QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling); } @@ -396,8 +393,8 @@ void pci_bridge_exitfn(PCIDevice *pci_dev) PCIBridge *s =3D PCI_BRIDGE(pci_dev); assert(QLIST_EMPTY(&s->sec_bus.child)); QLIST_REMOVE(&s->sec_bus, sibling); - pci_bridge_region_del(s, s->windows); - pci_bridge_region_cleanup(s, s->windows); + pci_bridge_region_del(s, &s->windows); + pci_bridge_region_cleanup(s, &s->windows); /* object_unparent() is called automatically during device deletion */ } =20 diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h index 01670e9e65..ea54a81a15 100644 --- a/include/hw/pci/pci_bridge.h +++ b/include/hw/pci/pci_bridge.h @@ -73,7 +73,7 @@ struct PCIBridge { MemoryRegion address_space_mem; MemoryRegion address_space_io; =20 - PCIBridgeWindows *windows; + PCIBridgeWindows windows; =20 pci_map_irq_fn map_irq; const char *bus_name; --=20 2.37.2