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 qemuSecurityPostFork(), 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 got 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 beforehand. The problem stack is as follows: libvirtd thread1 libvirtd thread2 child libvirtd | | | | | | virsh capabilities qemuProcessLanuch | | | | | lock top | | | | lock nested | | | | | | fork------------------->|(nested lock held 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 lock, so during fork, it's not possible for another task to acquire the nested lock. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1303031 Signed-off-by: hongmianquan <hongmianquan(a)bytedance.com&gt; --- 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(virQEMUDriver *driver) return NULL; } + /* 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 = qemuSecurityGetNested(driver->securityManager))) + if (!(sec_managers = qemuSecurityGetNested(driver->securityManager))) { + qemuSecurityStackUnlock(driver->securityManager); return NULL; + } /* calculate length */ for (i = 0; sec_managers[i]; i++) @@ -1617,14 +1622,18 @@ virCaps *virQEMUDriverCreateCapabilities(virQEMUDriver *driver) lbl = qemuSecurityGetBaseLabel(sec_managers[i], virtTypes[j]); type = virDomainVirtTypeToString(virtTypes[j]); if (lbl && - virCapabilitiesHostSecModelAddBaseLabel(sm, type, lbl) < 0) + virCapabilitiesHostSecModelAddBaseLabel(sm, type, lbl) < 0) { + qemuSecurityStackUnlock(driver->securityManager); return NULL; + } } VIR_DEBUG("Initialized caps for security driver \"%s\" with " "DOI \"%s\"", model, doi); } + qemuSecurityStackUnlock(driver->securityManager); + caps->host.numa = virCapabilitiesHostNUMANewHost(); caps->host.cpu = 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(virDomainPtr dom, ret = 0; } else { int len = 0; - virSecurityManager ** mgrs = qemuSecurityGetNested(driver->securityManager); - if (!mgrs) + virSecurityManager ** mgrs = NULL; + + /* Ensure top lock is acquired before nested locks */ + qemuSecurityStackLock(driver->securityManager); + + mgrs = qemuSecurityGetNested(driver->securityManager); + if (!mgrs) { + qemuSecurityStackUnlock(driver->securityManager); goto cleanup; + } /* Allocate seclabels array */ for (i = 0; mgrs[i]; i++) @@ -6233,11 +6240,13 @@ static int qemuDomainGetSecurityLabelList(virDomainPtr dom, &(*seclabels)[i]) < 0) { VIR_FREE(mgrs); VIR_FREE(*seclabels); + qemuSecurityStackUnlock(driver->securityManager); goto cleanup; } } ret = len; VIR_FREE(mgrs); + qemuSecurityStackUnlock(driver->securityManager); } 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_manager.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; } +/* + * 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); +} + /** * virSecurityManagerDomainSetPathLabel: diff --git a/src/security/security_manager.h b/src/security/security_manager.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); typedef enum { VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN = 1 << 0, -- 2.20.1

This commit removes the redundant call to qemuSecurityGetNested() in qemuStateInitialize(). In qemuSecurityGetModel(), the first security manager in the stack is already used by default, so this change helps to simplify the code. Signed-off-by: hongmianquan <hongmianquan(a)bytedance.com&gt; --- src/qemu/qemu_driver.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9d4958789c..4831490456 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -650,7 +650,6 @@ qemuStateInitialize(bool privileged, bool autostart = true; size_t i; const char *defsecmodel = NULL; - g_autofree virSecurityManager **sec_managers = NULL; g_autoptr(virIdentity) identity = virIdentityGetCurrent(); qemu_driver = g_new0(virQEMUDriver, 1); @@ -925,11 +924,8 @@ qemuStateInitialize(bool privileged, if (!qemu_driver->qemuCapsCache) goto error; - if (!(sec_managers = qemuSecurityGetNested(qemu_driver->securityManager))) - goto error; - - if (sec_managers[0] != NULL) - defsecmodel = qemuSecurityGetModel(sec_managers[0]); + if (qemu_driver->securityManager != NULL) + defsecmodel = qemuSecurityGetModel(qemu_driver->securityManager); if (!(qemu_driver->xmlopt = virQEMUDriverCreateXMLConf(qemu_driver, defsecmodel))) -- 2.20.1