From nobody Fri Oct 18 08:47:11 2024 Delivered-To: importer@patchew.org Received-SPF: pass (zohomail.com: domain of lists.libvirt.org designates 8.43.85.245 as permitted sender) client-ip=8.43.85.245; envelope-from=devel-bounces@lists.libvirt.org; helo=lists.libvirt.org; Authentication-Results: mx.zohomail.com; dkim=fail; spf=pass (zohomail.com: domain of lists.libvirt.org designates 8.43.85.245 as permitted sender) smtp.mailfrom=devel-bounces@lists.libvirt.org; dmarc=pass(p=reject dis=none) header.from=lists.libvirt.org ARC-Seal: i=1; a=rsa-sha256; t=1720171367; cv=none; d=zohomail.com; s=zohoarc; b=AC1J8RJXy8emh5Ujp40isxIMHOZrePqHw8M74s6B8chC4PSH6SWQx10g5e1iCho0djJsdJSyjXp8yVpztVd/AnxJxOPW0EIuMwqVm6LzrWTw7/n2qgOH0FOapeiNDxw3ilkAOw5blRKzsPJ39n5/SPb9zwkMOcAyDZrWKIKunj8= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1720171367; h=Content-Transfer-Encoding:Cc:Cc:Date:Date:From:From:In-Reply-To:List-Subscribe:List-Post:List-Id:List-Archive:List-Help:List-Unsubscribe:MIME-Version:Message-ID:Reply-To:Reply-To:References:Subject:Subject:To:To:Message-Id; bh=Qe5Im72jFn329nXphqyqovGCjh3DXzfa6sUYw+gw3Fc=; b=EfD0HAFzaWxjyFZ61gmyB3JC/9deO9uIMmV5sLfB1M5MT+d/xxJMdL7Jk8Zv1XDnMpffSKeO1X5M1Qed1RGvB/NF9BZ1MTJODP7e3htVXU+IuHC1o/ZS65Wx6Nzn+Zuz2SX3QdJaMnCtxwcacsed/VEAQyAEs7vbcjeu1dxLKGY= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=fail; spf=pass (zohomail.com: domain of lists.libvirt.org designates 8.43.85.245 as permitted sender) smtp.mailfrom=devel-bounces@lists.libvirt.org; dmarc=pass header.from= (p=reject dis=none) Return-Path: Received: from lists.libvirt.org (lists.libvirt.org [8.43.85.245]) by mx.zohomail.com with SMTPS id 1720171367121431.16443209144563; Fri, 5 Jul 2024 02:22:47 -0700 (PDT) Received: by lists.libvirt.org (Postfix, from userid 996) id 0E128123E; Fri, 5 Jul 2024 05:22:46 -0400 (EDT) Received: from lists.libvirt.org (localhost [IPv6:::1]) by lists.libvirt.org (Postfix) with ESMTP id AD09C1512; Fri, 5 Jul 2024 05:21:56 -0400 (EDT) Received: by lists.libvirt.org (Postfix, from userid 996) id 70AF21203; Fri, 5 Jul 2024 04:02:19 -0400 (EDT) Received: from mail-oo1-f47.google.com (mail-oo1-f47.google.com [209.85.161.47]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.libvirt.org (Postfix) with ESMTPS id 9D45E1202 for ; Fri, 5 Jul 2024 04:02:18 -0400 (EDT) Received: by mail-oo1-f47.google.com with SMTP id 006d021491bc7-5c219c54800so656157eaf.0 for ; Fri, 05 Jul 2024 01:02:18 -0700 (PDT) Received: from D491Q0MXRG.bytedance.net ([139.177.225.246]) by smtp.gmail.com with ESMTPSA id 41be03b00d2f7-72c6c8ebf1asm10793757a12.74.2024.07.05.01.02.13 (version=TLS1_3 cipher=TLS_CHACHA20_POLY1305_SHA256 bits=256/256); Fri, 05 Jul 2024 01:02:15 -0700 (PDT) X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on lists.libvirt.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_INVALID,DKIM_SIGNED, MAILING_LIST_MULTI,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL,SPF_HELO_NONE autolearn=unavailable autolearn_force=no version=3.4.4 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bytedance.com; s=google; t=1720166536; x=1720771336; darn=lists.libvirt.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=7fZX4tOQdDmez3JL1kuGiFNv6Livm6SAkDvf3menUas=; b=MvEgOCFRCPlup1o0dlPchTNhja/aJj2I8nROxABFBJQGWBjsO38p2iK2MqFn3iofPM 7r8tUwaQl2wFiJGpgzn2rSlOsTPL501A2t/+K6YAFJxFtfNWJCsDYvpxw6qBZXIXCATT 9wV6vFg9f0Ot+Kb5QO9ay9p+UbOyfp3KwM/RKThWHFHplKUoVj7/+mEscJp7hoZkiN9S Vk9r/24Yz+P7m7I4IRWybLxNtntLbXWhcf/urUI28WBPP3ggr1o+TITb2DQn4jsv4ba1 9oDaviMcnk241mZrjLbVUCGv1VqWybjUUU4kmRUuefbIQ7aXvWS8kBS4kRzhWAsSSAKn t+lQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1720166536; x=1720771336; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=7fZX4tOQdDmez3JL1kuGiFNv6Livm6SAkDvf3menUas=; b=m8bP9DR+w4nQMivDhE/MWvqRsYADg25rFPhedl/8E7wb8eDQHzYmsfZu6gX4dUg3v/ Me17ig4BEPux3JB35qNmjCm3Ym6ZDccYgG8KlxkLj8lUP+zMBAWSCVX1BJsaHx60igMV Bw97P7kvUXFULYkoXAGabX5Oh/zxXrexFcZtT1I0f5T0IJFICyEv5SEnHIEB7HDOfthJ hrV2usj+0AB9dxmQXCteV1w7EK2nq4zbJDZTW6nXA8qHfPatechh93AcmsetueSWjf/6 qah4hpUGHSR3p7wpwOIoEpccg5eFZL3+pCqbu+tbWJO/LVnZXNOtjtlMZaGlX0GmGZXW R4NA== X-Gm-Message-State: AOJu0Yybqf7tiJQgNfjU3wCQ38ale4UAJNq8jI+Xhd+ZQNYjBoobaYF7 ndpz2BQw/kmn3/qNTRHUrxk18GMTp8ip53Ze1WmJKOTHU93DoUsBHpAyWsMktvhMKShrQH2FJka V X-Google-Smtp-Source: AGHT+IHZDR+Kxzab/a0HEuBk9HxXqBQD5M6LjGQtYhCopzg7UbxZTdlrhsRN7rHiHURkivgNiRlgFQ== X-Received: by 2002:a05:6871:24cb:b0:25d:905:dcc4 with SMTP id 586e51a60fabf-25e2beb0051mr3251494fac.47.1720166536337; Fri, 05 Jul 2024 01:02:16 -0700 (PDT) To: devel@lists.libvirt.org, mprivozn@redhat.com Subject: [PATCH v3 1/2] security_manager: Ensure top lock is acquired before nested locks Date: Fri, 5 Jul 2024 16:01:57 +0800 Message-Id: <20240705080158.92603-2-hongmianquan@bytedance.com> X-Mailer: git-send-email 2.32.1 (Apple Git-133) In-Reply-To: <20240705080158.92603-1-hongmianquan@bytedance.com> References: <20240705080158.92603-1-hongmianquan@bytedance.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-MailFrom: hongmianquan@bytedance.com X-Mailman-Rule-Hits: nonmember-moderation X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-config-1; header-match-config-2; header-match-config-3; header-match-devel.lists.libvirt.org-0 Message-ID-Hash: P4K5INLYQVJVWSRDR7LY4QECHEZGATZC X-Message-ID-Hash: P4K5INLYQVJVWSRDR7LY4QECHEZGATZC X-Mailman-Approved-At: Fri, 05 Jul 2024 09:21:53 -0400 CC: xieyongji@bytedance.com, yinyipeng@bytedance.com, hongmianquan X-Mailman-Version: 3.2.2 Precedence: list List-Id: Development discussions about the libvirt library & tools Archived-At: List-Archive: List-Help: List-Post: List-Subscribe: List-Unsubscribe: From: hongmianquan via Devel Reply-To: hongmianquan X-ZohoMail-DKIM: fail (Header signature does not verify) X-ZM-MESSAGEID: 1720171367867100001 Content-Type: text/plain; charset="utf-8" Fix libvirtd hang since fork() was called while another thread had security manager locked. We have the stack security driver, which internally manages other security = drivers, just call them "top" and "nested". We call virSecurityStackPreFork() to lock the top one, and it also locks and then unlocks the nested drivers prior to fork. Then in qemuSecurityPost= Fork(), it unlocks the top one, but not the nested ones. Thus, if one of the nested drivers ("dac" or "selinux") is still locked, it will cause a deadlock. If = we always surround nested locks with top lock, it is always secure. Because we have g= ot top lock before fork child libvirtd. However, it is not always the case in the current code, We discovered this = case: the nested list obtained through the qemuSecurityGetNested() will be locked= directly for subsequent use, such as in virQEMUDriverCreateCapabilities(), where the= nested list is locked using qemuSecurityGetDOI, but the top one is not locked beforehan= d. The problem stack is as follows: libvirtd thread1 libvirtd thread2 child libvirtd | | | | | | virsh capabilities qemuProcessLanuch | | | | | lock top | | | | lock nested | | | | | | fork------------------->|(nested lock h= eld by thread1) | | | | | | unlock nested unlock top unlock top | | qemuSecuritySetSocketLabel | | lock nested (deadlock) In this commit, we ensure that the top lock is acquired before the nested l= ock, so during fork, it's not possible for another task to acquire the nested lo= ck. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=3D1303031 Signed-off-by: hongmianquan --- src/libvirt_private.syms | 2 ++ src/qemu/qemu_conf.c | 13 +++++++++++-- src/qemu/qemu_driver.c | 13 +++++++++++-- src/qemu/qemu_security.h | 2 ++ src/security/security_manager.c | 22 ++++++++++++++++++++++ src/security/security_manager.h | 2 ++ 6 files changed, 50 insertions(+), 4 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9dc0d6e007..ec626fc735 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1773,6 +1773,8 @@ virSecurityManagerSetSocketLabel; virSecurityManagerSetTapFDLabel; virSecurityManagerSetTPMLabels; virSecurityManagerStackAddNested; +virSecurityManagerStackLock; +virSecurityManagerStackUnlock; virSecurityManagerTransactionAbort; virSecurityManagerTransactionCommit; virSecurityManagerTransactionStart; diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 95c70457fc..efca446637 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1595,9 +1595,14 @@ virCaps *virQEMUDriverCreateCapabilities(virQEMUDriv= er *driver) return NULL; } =20 + /* Ensure top lock is acquired before nested locks */ + qemuSecurityStackLock(driver->securityManager); + /* access sec drivers and create a sec model for each one */ - if (!(sec_managers =3D qemuSecurityGetNested(driver->securityManager))) + if (!(sec_managers =3D qemuSecurityGetNested(driver->securityManager))= ) { + qemuSecurityStackUnlock(driver->securityManager); return NULL; + } =20 /* calculate length */ for (i =3D 0; sec_managers[i]; i++) @@ -1617,14 +1622,18 @@ virCaps *virQEMUDriverCreateCapabilities(virQEMUDri= ver *driver) lbl =3D qemuSecurityGetBaseLabel(sec_managers[i], virtTypes[j]= ); type =3D virDomainVirtTypeToString(virtTypes[j]); if (lbl && - virCapabilitiesHostSecModelAddBaseLabel(sm, type, lbl) < 0) + virCapabilitiesHostSecModelAddBaseLabel(sm, type, lbl) < 0= ) { + qemuSecurityStackUnlock(driver->securityManager); return NULL; + } } =20 VIR_DEBUG("Initialized caps for security driver \"%s\" with " "DOI \"%s\"", model, doi); } =20 + qemuSecurityStackUnlock(driver->securityManager); + caps->host.numa =3D virCapabilitiesHostNUMANewHost(); caps->host.cpu =3D virQEMUDriverGetHostCPU(driver); return g_steal_pointer(&caps); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 124128b3c4..9d4958789c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6216,9 +6216,16 @@ static int qemuDomainGetSecurityLabelList(virDomainP= tr dom, ret =3D 0; } else { int len =3D 0; - virSecurityManager ** mgrs =3D qemuSecurityGetNested(driver->secur= ityManager); - if (!mgrs) + virSecurityManager ** mgrs =3D NULL; + + /* Ensure top lock is acquired before nested locks */ + qemuSecurityStackLock(driver->securityManager); + + mgrs =3D qemuSecurityGetNested(driver->securityManager); + if (!mgrs) { + qemuSecurityStackUnlock(driver->securityManager); goto cleanup; + } =20 /* Allocate seclabels array */ for (i =3D 0; mgrs[i]; i++) @@ -6233,11 +6240,13 @@ static int qemuDomainGetSecurityLabelList(virDomain= Ptr dom, &(*seclabels)[i]) < 0) { VIR_FREE(mgrs); VIR_FREE(*seclabels); + qemuSecurityStackUnlock(driver->securityManager); goto cleanup; } } ret =3D len; VIR_FREE(mgrs); + qemuSecurityStackUnlock(driver->securityManager); } =20 cleanup: diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h index 10f11771b4..69f5c7aa97 100644 --- a/src/qemu/qemu_security.h +++ b/src/qemu/qemu_security.h @@ -146,3 +146,5 @@ int qemuSecurityCommandRun(virQEMUDriver *driver, #define qemuSecuritySetTapFDLabel virSecurityManagerSetTapFDLabel #define qemuSecurityStackAddNested virSecurityManagerStackAddNested #define qemuSecurityVerify virSecurityManagerVerify +#define qemuSecurityStackLock virSecurityManagerStackLock +#define qemuSecurityStackUnlock virSecurityManagerStackUnlock diff --git a/src/security/security_manager.c b/src/security/security_manage= r.c index 5df0cd3419..d3133a76ff 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -989,6 +989,28 @@ virSecurityManagerGetNested(virSecurityManager *mgr) return list; } =20 +/* + * Usually called before virSecurityManagerGetNested(). + * We need to ensure locking the stack security manager before + * locking the nested security manager to maintain the correct + * synchronization state. + * It must be followed by a call virSecurityManagerStackUnlock(). + */ +void +virSecurityManagerStackLock(virSecurityManager *mgr) +{ + if (STREQ("stack", mgr->drv->name)) + virObjectLock(mgr); +} + + +void +virSecurityManagerStackUnlock(virSecurityManager *mgr) +{ + if (STREQ("stack", mgr->drv->name)) + virObjectUnlock(mgr); +} + =20 /** * virSecurityManagerDomainSetPathLabel: diff --git a/src/security/security_manager.h b/src/security/security_manage= r.h index 97add3294d..559a5c6da1 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -158,6 +158,8 @@ int virSecurityManagerSetTapFDLabel(virSecurityManager = *mgr, char *virSecurityManagerGetMountOptions(virSecurityManager *mgr, virDomainDef *vm); virSecurityManager ** virSecurityManagerGetNested(virSecurityManager *mgr); +void virSecurityManagerStackLock(virSecurityManager *mgr); +void virSecurityManagerStackUnlock(virSecurityManager *mgr); =20 typedef enum { VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN =3D 1 << 0, --=20 2.20.1