From nobody Mon Sep 16 19:06:12 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=1719507007; cv=none; d=zohomail.com; s=zohoarc; b=fxM6OFguhqlFQ6T6bMhYz+n09t7SNLiYhj2PszIXxa+cxEDaP6dS6zceRnrSuJX9o72FEW++2jX3ZZ3hSItLHJR2Vucq9UcFgJynez+49DBwfHS327D5TmMJq24mgqzd6PxQsyjwwRpJ5Gr1nzWKHHT9gHUOhJmFkRE6145X1Xk= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1719507007; h=Content-Transfer-Encoding:Cc:Cc:Date:Date:From:From:List-Subscribe:List-Post:List-Id:List-Archive:List-Help:List-Unsubscribe:MIME-Version:Message-ID:Reply-To:Reply-To:Subject:Subject:To:To:Message-Id; bh=InJY30W0DtkpQorShaiz+YKxItv7Uea87f5c4neAvfU=; b=gJmG7ixDC2FWxv/hzWR02JLbQafzyBTCEIQ3wNuxg4b6b6E7OWlmV1wTpbH2lnynUU+jsOpA/VPhCU9GHHuf3TBCQuw52aV0DdmAcDVhHzDcErXclXmoS/+ig3YuuiHO9/zEajRwY0Giw5A6z/b8YrpQetk3++8aRi474pX9RuY= 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 1719507007235625.4322916692955; Thu, 27 Jun 2024 09:50:07 -0700 (PDT) Received: by lists.libvirt.org (Postfix, from userid 996) id EB75F11BA; Thu, 27 Jun 2024 12:50:05 -0400 (EDT) Received: from lists.libvirt.org (localhost [IPv6:::1]) by lists.libvirt.org (Postfix) with ESMTP id 2677613EB; Thu, 27 Jun 2024 12:49:02 -0400 (EDT) Received: by lists.libvirt.org (Postfix, from userid 996) id 5E6C0CD1; Thu, 27 Jun 2024 07:41:34 -0400 (EDT) Received: from mail-pf1-f174.google.com (mail-pf1-f174.google.com [209.85.210.174]) (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 26DE7B38 for ; Thu, 27 Jun 2024 07:41:33 -0400 (EDT) Received: by mail-pf1-f174.google.com with SMTP id d2e1a72fcca58-706627ff48dso4220062b3a.1 for ; Thu, 27 Jun 2024 04:41:33 -0700 (PDT) Received: from D491Q0MXRG.bytedance.net ([203.208.167.149]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-706b4a072e5sm1126293b3a.105.2024.06.27.04.41.29 (version=TLS1_3 cipher=TLS_CHACHA20_POLY1305_SHA256 bits=256/256); Thu, 27 Jun 2024 04:41:30 -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=1719488491; x=1720093291; darn=lists.libvirt.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=YchraZpMKI7+/BAgV1sMdfNeEz80JzgtJ3aR52ljkQQ=; b=EN731+cFjHgLepFFpvULlNignVu7iElQc1AXzttwbtNWiL4HtnzUAxROvp0r5vaWlG 36gbxVxDaX8+JeTrFWP0UR1dBRhaYghuAH5TZDclmQvXiTF8fMr1ZoN8NFf6n4mSmUPg afAfRWyMRRXuoAgsD3FpHewD+yJCc7fiV/tCNZSnQE5MYMfIXZ/kyr0Cev0ehprvURh/ HSFeocOtFw61kU0dxeMmjMMmLClkCeN01w8sOEbcvOYsjjD3LAoj2v8KmDxfguyE+6gi pLSil6COKWWgHCMgP/jmPOfOnV+STYwG/D82UivRDGs6mcQjU9puG6XKOVmRUjt/3yOs 35NA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1719488491; x=1720093291; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=YchraZpMKI7+/BAgV1sMdfNeEz80JzgtJ3aR52ljkQQ=; b=pAWRFhHta2HaYy4olSSbx5gFdzCvHzEwSRtmtJJYTY5lioVa9l+75bJAG/1EDvjK00 pkw1bmcYJafPGCiaYwDK9Y1tzxAO8r/TRvgX9jDBOBXp5PIV4V3QWKGCSSPS/fbBLbmD 94G0zrs8gygOhgEWxrddj0XsTcFjvQcHJrtMHCanT+wQCuxGQw87Apr2JhuKLzvK1OzO S+QhxgaIicBS5gIN/zr1BwxehLOeZSRisdT2OxoyfoovL9qnfLoifgSRDwnhWxjI8FLX Hy3LbenG6wRUNc+6VGmx0NGxNVVuF44fhboVgV9SFfQUwJBI74cAIO/resyRIC5reRX4 X9bw== X-Gm-Message-State: AOJu0YxbYGpJfD0W/d51f/XnRt+pd+tf5qHbyDbNo7tK3G60TVhNyCOM y5Avisz6No9rhocxBW5j4ZHOBgyMSE7DCSLmRj9b249B/EPaL34orj4XtCTTT78Sl4OzupuI/lo N X-Google-Smtp-Source: AGHT+IE5Sw0yPD/R9j333uXfCXTi2uIpY5Mbx62ryA9s+64gj4TYzC4ySDEagd3OV6yrgLbfFctkXQ== X-Received: by 2002:a05:6a00:b91:b0:705:b81b:6ee2 with SMTP id d2e1a72fcca58-706745e809bmr18147822b3a.19.1719488491396; Thu, 27 Jun 2024 04:41:31 -0700 (PDT) To: devel@lists.libvirt.org Subject: [PATCH v2] security_manager: Ensure top lock is acquired before nested locks Date: Thu, 27 Jun 2024 19:41:23 +0800 Message-Id: <20240627114123.11209-1-hongmianquan@bytedance.com> X-Mailer: git-send-email 2.32.1 (Apple Git-133) 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: 43GSEDNC67OOKL4Y377HHVIAFICNG2AX X-Message-ID-Hash: 43GSEDNC67OOKL4Y377HHVIAFICNG2AX X-Mailman-Approved-At: Thu, 27 Jun 2024 16:48:58 -0400 CC: 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: 1719507008173100001 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. We discovered this case: the nested list obtained through the qemuSecurityG= etNested() will be locked for subsequent use, such as in virQEMUDriverCreateCapabiliti= es(), where the nested list is locked using qemuSecurityGetDOI, but the top one i= s not locked beforehand. 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 | 3 ++- src/qemu/qemu_conf.c | 9 ++++++++- src/qemu/qemu_driver.c | 16 +++++++++------- src/qemu/qemu_security.h | 2 ++ src/security/security_manager.c | 22 ++++++++++++++++++++++ src/security/security_manager.h | 2 ++ 6 files changed, 45 insertions(+), 9 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bac4a8a366..39cdb90772 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1806,7 +1806,8 @@ virSecurityManagerTransactionAbort; virSecurityManagerTransactionCommit; virSecurityManagerTransactionStart; virSecurityManagerVerify; - +virSecurityManagerStackLock; +virSecurityManagerStackUnlock; =20 # security/security_util.h virSecurityXATTRNamespaceDefined; diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 4050a82341..21f0739fd5 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1380,6 +1380,9 @@ virCaps *virQEMUDriverCreateCapabilities(virQEMUDrive= r *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))) return NULL; @@ -1402,8 +1405,10 @@ virCaps *virQEMUDriverCreateCapabilities(virQEMUDriv= er *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 " @@ -1412,6 +1417,8 @@ virCaps *virQEMUDriverCreateCapabilities(virQEMUDrive= r *driver) =20 caps->host.numa =3D virCapabilitiesHostNUMANewHost(); caps->host.cpu =3D virQEMUDriverGetHostCPU(driver); + + qemuSecurityStackUnlock(driver->securityManager); return g_steal_pointer(&caps); } =20 diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fc1704f4fc..c980a0990f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -560,7 +560,6 @@ qemuStateInitialize(bool privileged, bool autostart =3D true; size_t i; const char *defsecmodel =3D NULL; - g_autofree virSecurityManager **sec_managers =3D NULL; g_autoptr(virIdentity) identity =3D virIdentityGetCurrent(); =20 qemu_driver =3D g_new0(virQEMUDriver, 1); @@ -835,11 +834,8 @@ qemuStateInitialize(bool privileged, if (!qemu_driver->qemuCapsCache) goto error; =20 - if (!(sec_managers =3D qemuSecurityGetNested(qemu_driver->securityMana= ger))) - goto error; - - if (sec_managers[0] !=3D NULL) - defsecmodel =3D qemuSecurityGetModel(sec_managers[0]); + if (qemu_driver->securityManager !=3D NULL) + defsecmodel =3D qemuSecurityGetModel(qemu_driver->securityManager); =20 if (!(qemu_driver->xmlopt =3D virQEMUDriverCreateXMLConf(qemu_driver, defsecmodel))) @@ -5663,7 +5659,12 @@ static int qemuDomainGetSecurityLabelList(virDomainP= tr dom, ret =3D 0; } else { int len =3D 0; - virSecurityManager ** mgrs =3D qemuSecurityGetNested(driver->secur= ityManager); + virSecurityManager ** mgrs =3D NULL; + + /* Ensure top lock is acquired before nested locks */ + qemuSecurityStackLock(driver->securityManager); + + mgrs =3D qemuSecurityGetNested(driver->securityManager); if (!mgrs) goto cleanup; =20 @@ -5688,6 +5689,7 @@ static int qemuDomainGetSecurityLabelList(virDomainPt= r dom, } =20 cleanup: + qemuSecurityStackUnlock(driver->securityManager); virDomainObjEndAPI(&vm); return ret; } diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h index 41da33debc..19fcb3c939 100644 --- a/src/qemu/qemu_security.h +++ b/src/qemu/qemu_security.h @@ -151,3 +151,5 @@ int qemuSecurityCommandRun(virQEMUDriver *driver, #define qemuSecuritySetTapFDLabel virSecurityManagerSetTapFDLabel #define qemuSecurityStackAddNested virSecurityManagerStackAddNested #define qemuSecurityVerify virSecurityManagerVerify +#define qemuSecurityStackLock virSecurityManagerStackLock +#define qemuSecurityStackUnlock virSecurityManagerStackUnlock \ No newline at end of file diff --git a/src/security/security_manager.c b/src/security/security_manage= r.c index 24f2f3d3dc..c49c4f708f 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 a416af3215..bb6d22bc31 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.11.0