Friendly ping. 在 2024/6/25 6:03 下午, hongmianquan 写道: > We need to ensure top lock is acquired before nested lock. Otherwise deadlock > issues may arise. We have the stack security driver, which internally manages > other security drivers, we 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. > We discovered this case: the nested list obtained through the qemuSecurityGetNested() > will be locked 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------------------->|(held nested lock) > | | | > | | | > 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 | 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; > > # 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(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))) > return NULL; > @@ -1402,8 +1405,10 @@ 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 " > @@ -1412,6 +1417,8 @@ virCaps *virQEMUDriverCreateCapabilities(virQEMUDriver *driver) > > caps->host.numa = virCapabilitiesHostNUMANewHost(); > caps->host.cpu = virQEMUDriverGetHostCPU(driver); > + > + qemuSecurityStackUnlock(driver->securityManager); > return g_steal_pointer(&caps); > } > > 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 = 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); > @@ -835,11 +834,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))) > @@ -5663,7 +5659,12 @@ static int qemuDomainGetSecurityLabelList(virDomainPtr dom, > ret = 0; > } else { > int len = 0; > - virSecurityManager ** mgrs = qemuSecurityGetNested(driver->securityManager); > + virSecurityManager ** mgrs = NULL; > + > + /* Ensure top lock is acquired before nested locks */ > + qemuSecurityStackLock(driver->securityManager); > + > + mgrs = qemuSecurityGetNested(driver->securityManager); > if (!mgrs) > goto cleanup; > > @@ -5688,6 +5689,7 @@ static int qemuDomainGetSecurityLabelList(virDomainPtr dom, > } > > 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_manager.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; > } > > +/* > + * 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 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); > > typedef enum { > VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN = 1 << 0,