[PATCH 0/3] fix some issue in concurrency scenarios
Jiang Jiacheng (3): qemu: Init address before qemuProcessShutdownOrReboot during reconnect process qemu: get the stackManager lock before getting nested lock qemu: back up the path in qemuMonitorOpen src/qemu/qemu_conf.c | 6 +++++- src/qemu/qemu_driver.c | 3 +++ src/qemu/qemu_monitor.c | 5 ++++- src/qemu/qemu_process.c | 10 +++++----- 4 files changed, 17 insertions(+), 7 deletions(-) -- 2.33.0
When libvirt is restarted, the qemuProcessShutdownReboot command is executed to restore the VM that is being restarted. In this case, a coredump may occur when we hotplug a pci device since the PCI address hasn't be inited yet. Moving the initialization of address to the front of qemuProcessShutdownOrReboot to ensure that we have the address inited. Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/qemu/qemu_process.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 32f03ff79a..6c93f28a9e 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8784,6 +8784,11 @@ qemuProcessReconnect(void *opaque) goto error; } + if ((qemuDomainAssignAddresses(obj->def, priv->qemuCaps, + driver, obj, false)) < 0) { + goto error; + } + /* In case the domain shutdown or fake reboot while we were not running, * we need to finish the shutdown or fake reboot process. And we need to * do it after we have virQEMUCaps filled in. @@ -8802,11 +8807,6 @@ qemuProcessReconnect(void *opaque) if (qemuProcessBuildDestroyMemoryPaths(driver, obj, NULL, true) < 0) goto error; - if ((qemuDomainAssignAddresses(obj->def, priv->qemuCaps, - driver, obj, false)) < 0) { - goto error; - } - /* if domain requests security driver we haven't loaded, report error, but * do not kill the domain */ -- 2.33.0
On 9/28/22 15:53, Jiang Jiacheng wrote:
When libvirt is restarted, the qemuProcessShutdownReboot command is executed to restore the VM that is being restarted. In this case, a coredump may occur when we hotplug a pci device since the PCI address hasn't be inited yet. Moving the initialization of address to the front of qemuProcessShutdownOrReboot to ensure that we have the address inited.
Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/qemu/qemu_process.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> and pushed. Michal
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@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); } -- 2.33.0
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@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
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@huawei.com> --- 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) -- 2.33.0
On 9/28/22 15:53, Jiang Jiacheng wrote:
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@huawei.com> --- 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)
I'm failing to see how is this possible. Connecting to a monitor inherently has a job, qemuProcessStop also has a job, inherently. And even if there was a path, the @config is an object and all that qemuProcessStop does is just unrefs it. Hence, a path string being freed seems more like a refcounting issue to me. Thus, this looks more correct: diff --git c/src/qemu/qemu_monitor.c w/src/qemu/qemu_monitor.c index 5eba154d96..286f0ae7ff 100644 --- c/src/qemu/qemu_monitor.c +++ w/src/qemu/qemu_monitor.c @@ -687,9 +687,11 @@ qemuMonitorOpen(virDomainObj *vm, return NULL; } + virObjectRef(config); virObjectUnlock(vm); fd = qemuMonitorOpenUnix(config->data.nix.path); virObjectLock(vm); + virObjectUnref(config); if (fd < 0) return NULL; Michal
ping... Thanks. On 2022/9/28 21:53, Jiang Jiacheng wrote:
Jiang Jiacheng (3): qemu: Init address before qemuProcessShutdownOrReboot during reconnect process qemu: get the stackManager lock before getting nested lock qemu: back up the path in qemuMonitorOpen
src/qemu/qemu_conf.c | 6 +++++- src/qemu/qemu_driver.c | 3 +++ src/qemu/qemu_monitor.c | 5 ++++- src/qemu/qemu_process.c | 10 +++++----- 4 files changed, 17 insertions(+), 7 deletions(-)
participants (2)
-
Jiang Jiacheng -
Michal Prívozník