From: Peter Krempa Date: Mon, 10 Jun 2024 16:12:16 +0000 (+0200) Subject: qemuProcessStop: Prevent crash when qemuDomainObjStopWorker() unlocks the VM X-Git-Url: http://xenbits.xensource.com/gitweb?a=commitdiff_plain;h=d29e0f3d4a5362d7b33261df1e55896396707de3;p=libvirt.git qemuProcessStop: Prevent crash when qemuDomainObjStopWorker() unlocks the VM 'qemuDomainObjStopWorker()' which is meant to dispose of the event loop thread for the monitor unlocks the VM object while disposing the thread to prevent possible deadlocks with events waiting on the monitor thread. Unfortunately 'qemuDomainObjStopWorker()' is called *before* the VM is marked as inactive by clearing 'vm->def->id', but at the same time it's no longer marked as 'beingDestroyed' when we're inside 'qemuProcessStop()'. If 'vm' would be kept locked this wouldn't be a problem. Same way it's not a problem for anything that uses non-ASYNC VM jobs, or when the monitor is accessed in an async job, as the 'destroy' job interlocks with those. It is a problem for code inside an async job which uses 'qemuDomainObjWait()' though. The API contract of qemuDomainObjWait() ensures the caller that the VM on successful return from it, but in this specific reason it's not the case, as both 'beingDestroyed' is already false, and 'vm->def->id' is not yet cleared. To fix the issue move the 'qemuDomainObjStopWorker()' call *after* clearing 'vm->def->id' and also add a note stating what the function is doing. Fixes: 860a999802d3c82538373bb3f314f92a2e258754 Closes: https://gitlab.com/libvirt/libvirt/-/issues/640 Reported-by: luzhipeng Signed-off-by: Peter Krempa Reviewed-by: Michal Privoznik --- diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 9de01b1a0d..d3163a8605 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8533,8 +8533,6 @@ void qemuProcessStop(virQEMUDriver *driver, g_clear_pointer(&priv->monConfig, virObjectUnref); } - qemuDomainObjStopWorker(vm); - /* Remove the master key */ qemuDomainMasterKeyRemove(priv); @@ -8568,6 +8566,11 @@ void qemuProcessStop(virQEMUDriver *driver, /* Wake up anything waiting on domain condition */ virDomainObjBroadcast(vm); + /* IMPORTANT: qemuDomainObjStopWorker() unlocks @vm in order to prevent + * deadlocks with the per-VM event loop thread. This MUST be done after + * marking the VM as dead */ + qemuDomainObjStopWorker(vm); + virFileDeleteTree(priv->libDir); virFileDeleteTree(priv->channelTargetDir);