From patchwork Thu Oct 19 15:40:17 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Woodhouse X-Patchwork-Id: 13429405 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 654FFCDB488 for ; Thu, 19 Oct 2023 15:41:19 +0000 (UTC) Received: from list by lists.xenproject.org with outflank-mailman.619409.964390 (Exim 4.92) (envelope-from ) id 1qtV8j-0005vK-FJ; Thu, 19 Oct 2023 15:40:49 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 619409.964390; Thu, 19 Oct 2023 15:40:49 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1qtV8j-0005vC-B7; Thu, 19 Oct 2023 15:40:49 +0000 Received: by outflank-mailman (input) for mailman id 619409; Thu, 19 Oct 2023 15:40:48 +0000 Received: from se1-gles-sth1-in.inumbo.com ([159.253.27.254] helo=se1-gles-sth1.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1qtV8h-0005v1-Pw for xen-devel@lists.xenproject.org; Thu, 19 Oct 2023 15:40:48 +0000 Received: from casper.infradead.org (casper.infradead.org [2001:8b0:10b:1236::1]) by se1-gles-sth1.inumbo.com (Halon) with ESMTPS id de5c7d8a-6e95-11ee-98d5-6d05b1d4d9a1; Thu, 19 Oct 2023 17:40:46 +0200 (CEST) Received: from [2001:8b0:10b:1::ebe] (helo=i7.infradead.org) by casper.infradead.org with esmtpsa (Exim 4.94.2 #2 (Red Hat Linux)) id 1qtV8N-007ot1-K0; Thu, 19 Oct 2023 15:40:27 +0000 Received: from dwoodhou by i7.infradead.org with local (Exim 4.96 #2 (Red Hat Linux)) id 1qtV8N-000Pv5-0t; Thu, 19 Oct 2023 16:40:27 +0100 X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list X-Inumbo-ID: de5c7d8a-6e95-11ee-98d5-6d05b1d4d9a1 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=Sender:Content-Transfer-Encoding: MIME-Version:References:In-Reply-To:Message-Id:Date:Subject:Cc:To:From: Reply-To:Content-Type:Content-ID:Content-Description; bh=iaeDXSTaoWdCm87GDHXDzDikArDTOfszjp92r6YPQM0=; b=i+jG3TmJBmY+zkUpHBdimsG7cq 7p0LDLP5W7wK0hCX+CeYTknNp1J5rK1m0dyVJ6+T831ZnUgVu7V/UMwvxU4q9P/StzXIiLSxKYXt+ VMW8IT2l6P1NZhPDNqyPqseAATfVk971Z4Q833tnPE/Kf0gKrP1Zh7BH2U410KfSi6+In3MJ/SBHN TI/EBEeH4n9s2rgGYbRurAllX+cLnCc/AbH7JXqf+RQkFqXb5Gsj+sF+n9eRd8R3u0ifOIq4fjnwO UaJEe6dC5bT5fJJM/beY/4CRxmoL3A5t0gs90g5VnQEnfwbjcVqTva9L1r8vyFio0Kb/uknr+vsxl QdfW8f8A==; From: David Woodhouse To: qemu-devel@nongnu.org Cc: Kevin Wolf , Hanna Reitz , Stefano Stabellini , Anthony Perard , Paul Durrant , =?utf-8?q?Marc-Andr=C3=A9_Lureau?= , Paolo Bonzini , Richard Henderson , Eduardo Habkost , "Michael S. Tsirkin" , Marcel Apfelbaum , Jason Wang , Marcelo Tosatti , Cleber Rosa , =?utf-8?q?Philippe_Mathieu-Daud=C3=A9?= , Wainer dos Santos Moschetta , Beraldo Leal , qemu-block@nongnu.org, xen-devel@lists.xenproject.org, kvm@vger.kernel.org, Bernhard Beschow , Joel Upham Subject: [PATCH v2 21/24] net: do not delete nics in net_cleanup() Date: Thu, 19 Oct 2023 16:40:17 +0100 Message-Id: <20231019154020.99080-22-dwmw2@infradead.org> X-Mailer: git-send-email 2.40.1 In-Reply-To: <20231019154020.99080-1-dwmw2@infradead.org> References: <20231019154020.99080-1-dwmw2@infradead.org> MIME-Version: 1.0 Sender: David Woodhouse X-SRS-Rewrite: SMTP reverse-path rewritten from by casper.infradead.org. See http://www.infradead.org/rpr.html From: David Woodhouse In net_cleanup() we only need to delete the netdevs, as those may have state which outlives Qemu when it exits, and thus may actually need to be cleaned up on exit. The nics, on the other hand, are owned by the device which created them. Most devices don't bother to clean up on exit because they don't have any state which will outlive Qemu... but XenBus devices do need to clean up their nodes in XenStore, and do have an exit handler to delete them. When the XenBus exit handler destroys the xen-net-device, it attempts to delete its nic after net_cleanup() had already done so. And crashes. Fix this by only deleting netdevs as we walk the list. As the comment notes, we can't use QTAILQ_FOREACH_SAFE() as each deletion may remove *multiple* entries, including the "safely" saved 'next' pointer. But we can store the *previous* entry, since nics are safe. Signed-off-by: David Woodhouse --- net/net.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/net/net.c b/net/net.c index c0c0cbe99e..bbe33da176 100644 --- a/net/net.c +++ b/net/net.c @@ -1499,18 +1499,34 @@ static void net_vm_change_state_handler(void *opaque, bool running, void net_cleanup(void) { - NetClientState *nc; + NetClientState *nc, **p = &QTAILQ_FIRST(&net_clients); /*cleanup colo compare module for COLO*/ colo_compare_cleanup(); - /* We may del multiple entries during qemu_del_net_client(), - * so QTAILQ_FOREACH_SAFE() is also not safe here. + /* + * Walk the net_clients list and remove the netdevs but *not* any + * NET_CLIENT_DRIVER_NIC entries. The latter are owned by the device + * model which created them, and in some cases (e.g. xen-net-device) + * the device itself may do cleanup at exit and will be upset if we + * just delete its NIC from underneath it. + * + * Since qemu_del_net_client() may delete multiple entries, using + * QTAILQ_FOREACH_SAFE() is not safe here. The only safe pointer + * to keep as a bookmark is a NET_CLIENT_DRIVER_NIC entry, so keep + * 'p' pointing to either the head of the list, or the 'next' field + * of the latest NET_CLIENT_DRIVER_NIC, and operate on *p as we walk + * the list. + * + * The 'nc' variable isn't part of the list traversal; it's purely + * for convenience as too much '(*p)->' has a tendency to make the + * readers' eyes bleed. */ - while (!QTAILQ_EMPTY(&net_clients)) { - nc = QTAILQ_FIRST(&net_clients); + while (*p) { + nc = *p; if (nc->info->type == NET_CLIENT_DRIVER_NIC) { - qemu_del_nic(qemu_get_nic(nc)); + /* Skip NET_CLIENT_DRIVER_NIC entries */ + p = &QTAILQ_NEXT(nc, next); } else { qemu_del_net_client(nc); }