[PATCH 2/3] qemu: get the stackManager lock before getting nested lock
Michal Prívozník
mprivozn at redhat.com
Wed Oct 12 12:49:19 UTC 2022
On 9/28/22 15:53, Jiang Jiacheng wrote:
> When creating a VM by forking, there is a logic that get the lock of
> StacksecurityManager before get other nested lock to avoid deadlock
> between child process and thread of the parent. While in
> `virQEMUDriverCreateCapabilities` and `qemuDomainGetSecurityLabelList`,
> we get nested lock without getting the StacksecurityManager lock, which
> will result in deadlock in some concurrent scenarios. It is better to keep
> the logical in these funcitons same as others.
>
> Signed-off-by: Jiang Jiacheng <jiangjiacheng at huawei.com>
> ---
> src/qemu/qemu_conf.c | 6 +++++-
> src/qemu/qemu_driver.c | 3 +++
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 3b75cdeb95..745d3e6a5e 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1331,6 +1331,7 @@ virCaps *virQEMUDriverCreateCapabilities(virQEMUDriver *driver)
>
> caps->host.secModels = g_new0(virCapsHostSecModel, caps->host.nsecModels);
>
> + virObjectLock(driver->securityManager);
> for (i = 0; sec_managers[i]; i++) {
> virCapsHostSecModel *sm = &caps->host.secModels[i];
> doi = qemuSecurityGetDOI(sec_managers[i]);
> @@ -1342,14 +1343,17 @@ 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) {
> + virObjectUnlock(driver->securityManager);
> return NULL;
> + }
> }
>
> VIR_DEBUG("Initialized caps for security driver \"%s\" with "
> "DOI \"%s\"", model, doi);
> }
>
> + virObjectUnlock(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 707f4cc1bb..9fb5f1d653 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5848,15 +5848,18 @@ static int qemuDomainGetSecurityLabelList(virDomainPtr dom,
> (*seclabels) = g_new0(virSecurityLabel, len);
> memset(*seclabels, 0, sizeof(**seclabels) * len);
>
> + virObjectLock(driver->securityManager);
> /* Fill the array */
> for (i = 0; i < len; i++) {
> if (qemuSecurityGetProcessLabel(mgrs[i], vm->def, vm->pid,
> &(*seclabels)[i]) < 0) {
> VIR_FREE(mgrs);
> VIR_FREE(*seclabels);
> + virObjectUnlock(driver->securityManager);
> goto cleanup;
> }
> }
> + virObjectUnlock(driver->securityManager);
> ret = len;
> VIR_FREE(mgrs);
> }
I think the problem here is that virSecurityManagerGetNested() does not
acquire its own lock but yet accesses its own internal data. I believe
proper fix is among these lines:
diff --git i/src/security/security_manager.c w/src/security/security_manager.c
index 572e400a48..82ca748b61 100644
--- i/src/security/security_manager.c
+++ w/src/security/security_manager.c
@@ -973,6 +973,7 @@ virSecurityManagerGetMountOptions(virSecurityManager *mgr,
virSecurityManager **
virSecurityManagerGetNested(virSecurityManager *mgr)
{
+ VIR_LOCK_GUARD lock = virObjectLockGuard(mgr);
virSecurityManager ** list = NULL;
if (STREQ("stack", mgr->drv->name))
Michal
More information about the libvir-list
mailing list