On Fri, Jul 19, 2024 at 14:46:49 +0200, Michal Privoznik wrote:
Imagine two threads. Thread A is executing qemuProcessStop() and thread B is executing qemuDomainCreateXML(). To make things more interesting, the domain XML passed to qemuDomainCreateXML matches name + UUID of that the virDomainObj that qemuProcessStop() is currently working with. Here's what happens.
1) Thread A locks @vm, enters qemuProcessStop().
2) Thread B parses given XML, calls virDomainObjListAdd() -> virDomainObjListAdd() -> virDomainObjListAddLocked() -> virDomainObjListFindByUUIDLocked(). Since there's a match on UUID, an virDomainObj object is returned and the thread proceeds to calling virObjectLock(). NB, it's the same object as in thread A.
3) Thread A sets vm->def->id = -1; this means that from this point on, virDomainObjIsActive() will return false.
4) Thread A calls qemuDomainObjStopWorker() which unlocks the @vm.
5) Thread B acquires the @vm lock and since virDomainObjIsActive() returns false, it proceeds to calling virDomainObjAssignDef() where vm->def is replaced.
6) Thread B then calls qemuProcessBeginJob() which unlocks the @vm temporarily.
7) Thread A, still in qemuDomainObjStopWorker() acquires @vm lock and proceeds with cleanup.
8) Thread A finds different definition than the one needing cleanup.
In my testing I've seen stale pointers, e.g. vm->def->nets[0]->priv was NULL, which lead to a SIGSEGV as there's 'QEMU_DOMAIN_NETWORK_PRIVATE(net)->created' line when cleaning up nets. Your mileage may vary.
Even if we did not crash, the plain fact that vm->def is changed in the middle of qemuProcessStop() means we might be cleaning up something else than intended.
This paragraph is the important bit. The root cause of the problem here is that 'virDomainObjListAdd' inside 'qemuDomainCreateXML' can modify 'vm->def' whithout holding any _MODIFY-type JOB on the domain object which we normally require for any modification of 'vm->def' related data. This wasn't a problem until now as we've relinquished the lock on @vm only in situations when the @vm object was considered live: 1) Before the per-VM thread cleanup was added to qemuProcessStop it never unlocked 2) After the per-VM thread cleanup was added, this unlock was done prior to setting vm->def->id to '-1' 3) All other cases are done only when the VM is live.
As a fix, I'm moving all lines that obviously touch vm->def before the domain object is unlocked. That left virHookCall(VIR_HOOK_QEMU_OP_STOPPED, VIR_HOOK_SUBOP_END) nearly next to virHookCall(VIR_HOOK_QEMU_OP_RELEASE, VIR_HOOK_SUBOP_END) which I figured is not something we want. So I've shuffled things a bit more.
This feels like a fix for symptoms of 'virDomainObjListAdd' not honouring the _MODIFY-type job expectation, and we're shuffling code around so that it doesn't care about the broken expectation. Since I don't currently have a better idea of how to fix this I'm okay with this patch given the following conditions:
Fixes: 3865410e7f67ca4ec66e9a905e75f452762a97f0
Explain that the above commit inverted the order of setting the VM as inactive and unlocking thus allowing the above sequence of events to happen, and,
Resolves: https://issues.redhat.com/browse/RHEL-49607 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_process.c | 122 ++++++++++++++++++++-------------------- 1 file changed, 62 insertions(+), 60 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 25dfd04272..9ea6c678b8 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8530,6 +8530,18 @@ void qemuProcessStop(virQEMUDriver *driver, VIR_QEMU_PROCESS_KILL_FORCE| VIR_QEMU_PROCESS_KILL_NOCHECK));
+ vm->pid = 0; + + /* now that we know it's stopped call the hook if present */ + if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { + g_autofree char *xml = qemuDomainDefFormatXML(driver, NULL, vm->def, 0); + + /* we can't stop the operation even if the script raised an error */ + ignore_value(virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, + VIR_HOOK_QEMU_OP_STOPPED, VIR_HOOK_SUBOP_END, + NULL, xml, NULL)); + } + if (priv->agent) { g_clear_pointer(&priv->agent, qemuAgentClose); } @@ -8553,25 +8565,6 @@ void qemuProcessStop(virQEMUDriver *driver,
qemuDBusStop(driver, vm);
- /* Only after this point we can reset 'priv->beingDestroyed' so that - * there's no point at which the VM could be considered as alive between - * entering the destroy job and this point where the active "flag" is - * cleared. - */ - vm->def->id = -1; - priv->beingDestroyed = false; - - /* 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); - - if (!!g_atomic_int_dec_and_test(&driver->nactive) && driver->inhibitCallback) - driver->inhibitCallback(false, driver->inhibitOpaque); - /* Clear network bandwidth */ virDomainClearNetBandwidth(vm->def);
@@ -8588,18 +8581,6 @@ void qemuProcessStop(virQEMUDriver *driver, } }
- virPortAllocatorRelease(priv->nbdPort); - priv->nbdPort = 0; - - if (priv->monConfig) { - if (priv->monConfig->type == VIR_DOMAIN_CHR_TYPE_UNIX) - unlink(priv->monConfig->data.nix.path); - g_clear_pointer(&priv->monConfig, virObjectUnref); - } - - /* Remove the master key */ - qemuDomainMasterKeyRemove(priv); - ignore_value(virDomainChrDefForeach(vm->def, false, qemuProcessCleanupChardevDevice, @@ -8609,22 +8590,6 @@ void qemuProcessStop(virQEMUDriver *driver, /* Its namespace is also gone then. */ qemuDomainDestroyNamespace(driver, vm);
- virFileDeleteTree(priv->libDir); - virFileDeleteTree(priv->channelTargetDir); - - /* Stop autodestroy in case guest is restarted */ - virCloseCallbacksDomainRemove(vm, NULL, qemuProcessAutoDestroy); - - /* now that we know it's stopped call the hook if present */ - if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - g_autofree char *xml = qemuDomainDefFormatXML(driver, NULL, vm->def, 0); - - /* we can't stop the operation even if the script raised an error */ - ignore_value(virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, - VIR_HOOK_QEMU_OP_STOPPED, VIR_HOOK_SUBOP_END, - NULL, xml, NULL)); - } - /* Reset Security Labels unless caller don't want us to */ if (!(flags & VIR_QEMU_PROCESS_STOP_NO_RELABEL)) qemuSecurityRestoreAllLabel(driver, vm, @@ -8672,8 +8637,6 @@ void qemuProcessStop(virQEMUDriver *driver, virResctrlAllocRemove(vm->def->resctrls[i]->alloc); }
- qemuProcessRemoveDomainStatus(driver, vm); - /* Remove VNC and Spice ports from port reservation bitmap, but only if they were reserved by the driver (autoport=yes) */ @@ -8706,20 +8669,9 @@ void qemuProcessStop(virQEMUDriver *driver, } }
- for (i = 0; i < vm->ndeprecations; i++) - g_free(vm->deprecations[i]); - g_clear_pointer(&vm->deprecations, g_free); - vm->ndeprecations = 0; - vm->taint = 0; - vm->pid = 0; - virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason); for (i = 0; i < vm->def->niothreadids; i++) vm->def->iothreadids[i]->thread_id = 0;
- /* clean up a possible backup job */ - if (priv->backup) - qemuBackupJobTerminate(vm, VIR_DOMAIN_JOB_STATUS_CANCELED); - /* Do this explicitly after vm->pid is reset so that security drivers don't * try to enter the domain's namespace which is non-existent by now as qemu * is no longer running. */ @@ -8753,6 +8705,56 @@ void qemuProcessStop(virQEMUDriver *driver,
qemuSecurityReleaseLabel(driver->securityManager, vm->def);
+ /* Only after this point we can reset 'priv->beingDestroyed' so that + * there's no point at which the VM could be considered as alive between + * entering the destroy job and this point where the active "flag" is + * cleared. + */ + vm->def->id = -1; + priv->beingDestroyed = false; + + /* 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 */
Extend the comment here stating how virDomainObjAssignDef() in combination with vm->def->id being -1 can cause update of vm->def and thus no code below can ever access it any more.
+ qemuDomainObjStopWorker(vm); + + if (!!g_atomic_int_dec_and_test(&driver->nactive) && driver->inhibitCallback) + driver->inhibitCallback(false, driver->inhibitOpaque); + + virPortAllocatorRelease(priv->nbdPort); + priv->nbdPort = 0; + + if (priv->monConfig) { + if (priv->monConfig->type == VIR_DOMAIN_CHR_TYPE_UNIX) + unlink(priv->monConfig->data.nix.path); + g_clear_pointer(&priv->monConfig, virObjectUnref); + } + + /* Remove the master key */ + qemuDomainMasterKeyRemove(priv); + + virFileDeleteTree(priv->libDir); + virFileDeleteTree(priv->channelTargetDir); + + /* Stop autodestroy in case guest is restarted */ + virCloseCallbacksDomainRemove(vm, NULL, qemuProcessAutoDestroy); + + qemuProcessRemoveDomainStatus(driver, vm); + + for (i = 0; i < vm->ndeprecations; i++) + g_free(vm->deprecations[i]); + g_clear_pointer(&vm->deprecations, g_free); + vm->ndeprecations = 0; + vm->taint = 0; + virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason); + + /* clean up a possible backup job */ + if (priv->backup) + qemuBackupJobTerminate(vm, VIR_DOMAIN_JOB_STATUS_CANCELED); + /* clear all private data entries which are no longer needed */ qemuDomainObjPrivateDataClear(priv);
-- 2.44.2
Reviewed-by: Peter Krempa <pkrempa@redhat.com>