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(a)huawei.com&gt; --- 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); }

In the case of concurrent VM operations, it is possible to have a null pointer reference in qemuMonitorOpen. In the case of concurrent VM shutdown, the priv->monconf will be changed in qemuProcessStop. qemuMonitorOpen releases the lock before calling qemuMonitorOpenUnix and then references priv->monconf. The path variable in monconf will cause a null pointer, so it's better to back up the path content in monconf before releasing the lock. Signed-off-by: Jiang Jiacheng <jiangjiacheng(a)huawei.com&gt; --- src/qemu/qemu_monitor.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index c2808c75a3..792b895570 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -679,6 +679,7 @@ qemuMonitorOpen(virDomainObj *vm, { VIR_AUTOCLOSE fd = -1; qemuMonitor *ret = NULL; + g_aurofree char *path = NULL; if (config->type != VIR_DOMAIN_CHR_TYPE_UNIX) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -687,8 +688,10 @@ qemuMonitorOpen(virDomainObj *vm, return NULL; } + path = g_strdup(config->data.nix.path); + virObjectUnlock(vm); - fd = qemuMonitorOpenUnix(config->data.nix.path); + fd = qemuMonitorOpenUnix(path); virObjectLock(vm); if (fd < 0)