[PATCH 00/12] qemu: migration: Fix crashes when VM shutdowns itself during migration in active state
The daemon crashes due to unexpected cleanup happening due to bad assumptions about locking and state. See patch 5. Peter Krempa (12): qemuBlockJobProcessEventConcludedBackup: Handle potentially NULL 'job->disk' qemuDomainDiskPrivateDispose: Prevent dangling 'disk' pointer in blockjob data qemuDomainDeviceBackendChardevForeach: Fix typo in comment qemuDomainObjWait: Add documentation qemuProcessStop: Prevent crash when qemuDomainObjStopWorker() unlocks the VM qemuProcessStop: Move code not depending on 'vm->def->id' after reset of the ID qemu: process: Ensure that 'beingDestroyed' gets cleared only after VM id is reset qemu: domain: Introduce qemuDomainObjIsActive helper qemu: migration: Properly check for live VM after qemuDomainObjWait() qemu: migration: Inline 'qemuMigrationDstFinishResume()' qemuMigrationSrcRun: Re-check whether VM is active before accessing job data qemu: migration: Preserve error across qemuDomainSetMaxMemLock() on error paths src/qemu/qemu_backup.c | 6 +-- src/qemu/qemu_backup.h | 2 +- src/qemu/qemu_blockjob.c | 9 +++- src/qemu/qemu_domain.c | 40 +++++++++++++- src/qemu/qemu_domain.h | 2 + src/qemu/qemu_migration.c | 43 +++++++--------- src/qemu/qemu_process.c | 106 ++++++++++++++++++++++---------------- 7 files changed, 131 insertions(+), 77 deletions(-) -- 2.45.2
Similarly to other blockjob handlers, if there's no disk associated with the blockjob the handler needs to behave correctly. This is needed as the disk might have been de-associated on unplug or other operations. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_backup.c | 6 +++--- src/qemu/qemu_backup.h | 2 +- src/qemu/qemu_blockjob.c | 9 +++++++-- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c index 857709b17e..81391c29f7 100644 --- a/src/qemu/qemu_backup.c +++ b/src/qemu/qemu_backup.c @@ -966,7 +966,7 @@ qemuBackupGetXMLDesc(virDomainObj *vm, void qemuBackupNotifyBlockjobEnd(virDomainObj *vm, - virDomainDiskDef *disk, + const char *diskdst, qemuBlockjobState state, const char *errmsg, unsigned long long cur, @@ -983,7 +983,7 @@ qemuBackupNotifyBlockjobEnd(virDomainObj *vm, size_t i; VIR_DEBUG("vm: '%s', disk:'%s', state:'%d' errmsg:'%s'", - vm->def->name, disk->dst, state, NULLSTR(errmsg)); + vm->def->name, NULLSTR(diskdst), state, NULLSTR(errmsg)); if (!backup) return; @@ -1016,7 +1016,7 @@ qemuBackupNotifyBlockjobEnd(virDomainObj *vm, if (!backupdisk->store) continue; - if (STREQ(disk->dst, backupdisk->name)) { + if (STREQ_NULLABLE(diskdst, backupdisk->name)) { switch (state) { case QEMU_BLOCKJOB_STATE_COMPLETED: backupdisk->state = VIR_DOMAIN_BACKUP_DISK_STATE_COMPLETE; diff --git a/src/qemu/qemu_backup.h b/src/qemu/qemu_backup.h index ec0603026a..768da6cbef 100644 --- a/src/qemu/qemu_backup.h +++ b/src/qemu/qemu_backup.h @@ -36,7 +36,7 @@ qemuBackupJobCancelBlockjobs(virDomainObj *vm, void qemuBackupNotifyBlockjobEnd(virDomainObj *vm, - virDomainDiskDef *disk, + const char *diskdst, qemuBlockjobState state, const char *errmsg, unsigned long long cur, diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 4b5b63d287..42856df6d4 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -1372,8 +1372,12 @@ qemuBlockJobProcessEventConcludedBackup(virQEMUDriver *driver, unsigned long long progressTotal) { g_autoptr(qemuBlockStorageSourceAttachData) backend = NULL; + const char *diskdst = NULL; + + if (job->disk) + diskdst = job->disk->dst; - qemuBackupNotifyBlockjobEnd(vm, job->disk, newstate, job->errmsg, + qemuBackupNotifyBlockjobEnd(vm, diskdst, newstate, job->errmsg, progressCurrent, progressTotal, asyncJob); if (job->data.backup.store && @@ -1386,7 +1390,8 @@ qemuBlockJobProcessEventConcludedBackup(virQEMUDriver *driver, if (backend) qemuBlockStorageSourceAttachRollback(qemuDomainGetMonitor(vm), backend); - if (job->data.backup.bitmap) + if (job->disk && + job->data.backup.bitmap) qemuMonitorBitmapRemove(qemuDomainGetMonitor(vm), qemuBlockStorageSourceGetEffectiveNodename(job->disk->src), job->data.backup.bitmap); -- 2.45.2
Clear the 'disk' member of 'blockjob' as we're freeing the disk object at this point. While this should not normally happen it was observed when other bug allowed the VM to be cleared while other threads didn't yet finish. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7ba2ea4a5e..a39f361a64 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -798,7 +798,13 @@ qemuDomainDiskPrivateDispose(void *obj) virObjectUnref(priv->migrSource); g_free(priv->qomName); g_free(priv->nodeCopyOnRead); - virObjectUnref(priv->blockjob); + if (priv->blockjob) { + /* Prevent dangling 'disk' pointer, as the disk object will be freed + * right after this function returns if any of the blockjob instance + * outlives this for any reason. */ + priv->blockjob->disk = NULL; + virObjectUnref(priv->blockjob); + } } static virClass *qemuDomainStorageSourcePrivateClass; -- 2.45.2
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a39f361a64..9bbad887e0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -12302,7 +12302,7 @@ qemuDomainDeviceBackendChardevIter(virDomainDef *def G_GNUC_UNUSED, /** - * qemuDomainDeviceBackendChardevForeach:a + * qemuDomainDeviceBackendChardevForeach: * @def: domain definition * @cb: callback * @opqaue: data for @cb -- 2.45.2
Document why this function exists and meaning of return values. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9bbad887e0..8fe1b1924d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -12368,6 +12368,18 @@ qemuDomainRemoveLogs(virQEMUDriver *driver, } +/** + * qemuDomainObjWait: + * @vm: domain object + * + * Wait for a signal on the main domain condition. Take into account internal + * qemu state in addition to what virDomainObjWait checks. Code in the qemu + * driver must use this function exclusively instead of virDomainObjWait. + * + * Returns: + * - 0 on successful wait AND VM is guaranteed to be running + * - -1 on failure to wait or VM was terminated while waiting + */ int qemuDomainObjWait(virDomainObj *vm) { -- 2.45.2
On 6/13/24 17:11, Peter Krempa wrote:
Document why this function exists and meaning of return values.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9bbad887e0..8fe1b1924d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -12368,6 +12368,18 @@ qemuDomainRemoveLogs(virQEMUDriver *driver, }
+/** + * qemuDomainObjWait: + * @vm: domain object + * + * Wait for a signal on the main domain condition. Take into account internal + * qemu state in addition to what virDomainObjWait checks. Code in the qemu + * driver must use this function exclusively instead of virDomainObjWait. + * + * Returns: + * - 0 on successful wait AND VM is guaranteed to be running + * - -1 on failure to wait or VM was terminated while waiting
Nitpick, drop those leading '-' symbols. And possibly add a comma/full stop at EOLs.
+ */ int qemuDomainObjWait(virDomainObj *vm) {
Michal
'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: grass-lu <https://gitlab.com/grass-lu> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 7ef7040a85..aef83d2409 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8527,8 +8527,6 @@ void qemuProcessStop(virQEMUDriver *driver, g_clear_pointer(&priv->monConfig, virObjectUnref); } - qemuDomainObjStopWorker(vm); - /* Remove the master key */ qemuDomainMasterKeyRemove(priv); @@ -8562,6 +8560,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); -- 2.45.2
There are few function calls done while cleaning up a stopped VM which do require the old VM id, to e.g. clean up paths containing the 'short' domain name in the path. Anything else, which doesn't strictly require it can be moved after clearing the 'id' in order to decrease likelyhood of potential bugs. This patch moves all the code which does not require the 'id' (except for the log entry and closing the monitor socket) after the statement clearing the id and adds a comment explaining that anything in the section must not unlock the VM object. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 85 ++++++++++++++++++++++------------------- 1 file changed, 46 insertions(+), 39 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index aef83d2409..3187f0a9a2 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8482,10 +8482,11 @@ void qemuProcessStop(virQEMUDriver *driver, goto endjob; } - qemuProcessBuildDestroyMemoryPaths(driver, vm, NULL, false); - - if (!!g_atomic_int_dec_and_test(&driver->nactive) && driver->inhibitCallback) - driver->inhibitCallback(false, driver->inhibitOpaque); + /* BEWARE: At this point 'vm->def->id' is not cleared yet. Any code that + * requires the id (e.g. to call virDomainDefGetShortName()) must be placed + * between here (after the VM is killed) and the statement clearing the id. + * The code *MUST NOT* unlock vm, otherwise other code might be confused + * about the state of the VM. */ if ((timestamp = virTimeStringNow()) != NULL) { qemuDomainLogAppendMessage(driver, vm, "%s: shutting down, reason=%s\n", @@ -8493,6 +8494,47 @@ void qemuProcessStop(virQEMUDriver *driver, virDomainShutoffReasonTypeToString(reason)); } + /* shut it off for sure */ + ignore_value(qemuProcessKill(vm, + VIR_QEMU_PROCESS_KILL_FORCE| + VIR_QEMU_PROCESS_KILL_NOCHECK)); + + if (priv->agent) { + g_clear_pointer(&priv->agent, qemuAgentClose); + } + priv->agentError = false; + + if (priv->mon) { + g_clear_pointer(&priv->mon, qemuMonitorClose); + } + + qemuProcessBuildDestroyMemoryPaths(driver, vm, NULL, false); + + /* Do this before we delete the tree and remove pidfile. */ + qemuProcessKillManagedPRDaemon(vm); + + qemuDomainCleanupRun(driver, vm); + + outgoingMigration = (flags & VIR_QEMU_PROCESS_STOP_MIGRATED) && + (asyncJob == VIR_ASYNC_JOB_MIGRATION_OUT); + + qemuExtDevicesStop(driver, vm, outgoingMigration); + + qemuDBusStop(driver, vm); + + vm->def->id = -1; + + /* 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); @@ -8512,15 +8554,6 @@ void qemuProcessStop(virQEMUDriver *driver, virPortAllocatorRelease(priv->nbdPort); priv->nbdPort = 0; - if (priv->agent) { - g_clear_pointer(&priv->agent, qemuAgentClose); - } - priv->agentError = false; - - if (priv->mon) { - g_clear_pointer(&priv->mon, qemuMonitorClose); - } - if (priv->monConfig) { if (priv->monConfig->type == VIR_DOMAIN_CHR_TYPE_UNIX) unlink(priv->monConfig->data.nix.path); @@ -8530,41 +8563,15 @@ void qemuProcessStop(virQEMUDriver *driver, /* Remove the master key */ qemuDomainMasterKeyRemove(priv); - /* Do this before we delete the tree and remove pidfile. */ - qemuProcessKillManagedPRDaemon(vm); - ignore_value(virDomainChrDefForeach(vm->def, false, qemuProcessCleanupChardevDevice, NULL)); - /* shut it off for sure */ - ignore_value(qemuProcessKill(vm, - VIR_QEMU_PROCESS_KILL_FORCE| - VIR_QEMU_PROCESS_KILL_NOCHECK)); - /* Its namespace is also gone then. */ qemuDomainDestroyNamespace(driver, vm); - qemuDomainCleanupRun(driver, vm); - - outgoingMigration = (flags & VIR_QEMU_PROCESS_STOP_MIGRATED) && - (asyncJob == VIR_ASYNC_JOB_MIGRATION_OUT); - qemuExtDevicesStop(driver, vm, outgoingMigration); - - qemuDBusStop(driver, vm); - - vm->def->id = -1; - - /* 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); -- 2.45.2
Prevent the possibility that a VM could be considered as alive while inside qemuProcessStop. A recently fixed bug which unlocked the domain object while inside qemuProcessStop showed that there's possibility to confuse the state of the VM to be considered active while 'qemuProcessStop' is processing shutdown of the VM. Ensure that this doesn't happen by clearing the 'beingDestroyed' flag only after the VM id is cleared. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3187f0a9a2..8736738a5b 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8413,29 +8413,31 @@ qemuProcessBeginStopJob(virDomainObj *vm, { qemuDomainObjPrivate *priv = vm->privateData; unsigned int killFlags = forceKill ? VIR_QEMU_PROCESS_KILL_FORCE : 0; - int ret = -1; /* We need to prevent monitor EOF callback from doing our work (and * sending misleading events) while the vm is unlocked inside - * BeginJob/ProcessKill API - */ + * BeginJob/ProcessKill API or any other code path before 'vm->def->id' is + * cleared inside qemuProcessStop */ priv->beingDestroyed = true; if (qemuProcessKill(vm, killFlags) < 0) - goto cleanup; + goto error; /* Wake up anything waiting on domain condition */ VIR_DEBUG("waking up all jobs waiting on the domain condition"); virDomainObjBroadcast(vm); if (virDomainObjBeginJob(vm, job) < 0) - goto cleanup; + goto error; - ret = 0; + /* priv->beingDestroyed is deliberately left set to 'true' here. Caller + * is supposed to call qemuProcessStop, which will reset it after + * 'vm->def->id' is set to -1 */ + return 0; - cleanup: + error: priv->beingDestroyed = false; - return ret; + return -1; } @@ -8522,7 +8524,13 @@ 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); -- 2.45.2
The helper checks whether VM is active including the internal qemu state. This helper will become useful in situations when an async job is in use as VIR_JOB_DESTROY can run along async jobs thus both checks are necessary. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 18 ++++++++++++++++++ src/qemu/qemu_domain.h | 2 ++ 2 files changed, 20 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8fe1b1924d..64fe1abbc5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -12397,6 +12397,24 @@ qemuDomainObjWait(virDomainObj *vm) } +/** + * qemuDomainObjIsActive: + * @vm: domain object + * + * Return whether @vm is active. Take qemu-driver specifics into account. + */ +bool +qemuDomainObjIsActive(virDomainObj *vm) +{ + qemuDomainObjPrivate *priv = vm->privateData; + + if (priv->beingDestroyed) + return false; + + return virDomainObjIsActive(vm); +} + + /** * virDomainRefreshStatsSchema: * @driver: qemu driver data diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index a3089ea449..d777559119 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1113,6 +1113,8 @@ qemuDomainRemoveLogs(virQEMUDriver *driver, int qemuDomainObjWait(virDomainObj *vm); +bool +qemuDomainObjIsActive(virDomainObj *vm); int qemuDomainRefreshStatsSchema(virDomainObj *dom); -- 2.45.2
Similarly to the one change in commit 4d1a1fdffda19a62d62fa2457d162362 we should be checking that the VM is not being yet destroyed if we've invoked qemuDomainObjWait(). Use the new helper qemuDomainObjIsActive(). Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 1faab5dd23..0d8d3fd94f 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2058,7 +2058,6 @@ qemuMigrationSrcWaitForCompletion(virDomainObj *vm, virConnectPtr dconn, unsigned int flags) { - qemuDomainObjPrivate *priv = vm->privateData; virDomainJobData *jobData = vm->job->current; int rv; @@ -2069,7 +2068,7 @@ qemuMigrationSrcWaitForCompletion(virDomainObj *vm, return rv; if (qemuDomainObjWait(vm) < 0) { - if (virDomainObjIsActive(vm) && !priv->beingDestroyed) + if (qemuDomainObjIsActive(vm)) jobData->status = VIR_DOMAIN_JOB_STATUS_FAILED; return -2; } @@ -5055,7 +5054,7 @@ qemuMigrationSrcRun(virQEMUDriver *driver, error: virErrorPreserveLast(&orig_err); - if (virDomainObjIsActive(vm)) { + if (qemuDomainObjIsActive(vm)) { int reason; virDomainState state = virDomainObjGetState(vm, &reason); @@ -6781,7 +6780,7 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver, * overwrites it. */ virErrorPreserveLast(&orig_err); - if (virDomainObjIsActive(vm)) { + if (qemuDomainObjIsActive(vm)) { if (doKill) { qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, VIR_ASYNC_JOB_MIGRATION_IN, @@ -6805,7 +6804,7 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver, jobPriv->migParams, vm->job->apiFlags); } - if (!virDomainObjIsActive(vm)) + if (!qemuDomainObjIsActive(vm)) qemuDomainRemoveInactive(driver, vm, VIR_DOMAIN_UNDEFINE_TPM, false); virErrorRestore(&orig_err); @@ -7050,7 +7049,7 @@ qemuMigrationSrcToFile(virQEMUDriver *driver, virDomainObj *vm, virErrorPreserveLast(&orig_err); /* Restore max migration bandwidth */ - if (virDomainObjIsActive(vm)) { + if (qemuDomainObjIsActive(vm)) { if (qemuMigrationParamsSetULL(migParams, QEMU_MIGRATION_PARAM_MAX_BANDWIDTH, saveMigBandwidth * 1024 * 1024) == 0) -- 2.45.2
The function is a pointless wrapper on top of qemuMigrationDstWaitForCompletion. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 0d8d3fd94f..3524915e9d 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2098,7 +2098,7 @@ qemuMigrationDstWaitForCompletion(virDomainObj *vm, unsigned int flags = 0; int rv; - VIR_DEBUG("Waiting for incoming migration to complete"); + VIR_DEBUG("Waiting for incoming migration to complete (vm='%p')", vm); if (postcopy) flags = QEMU_MIGRATION_COMPLETED_POSTCOPY; @@ -6689,21 +6689,6 @@ qemuMigrationDstFinishFresh(virQEMUDriver *driver, } -static int -qemuMigrationDstFinishResume(virDomainObj *vm) -{ - VIR_DEBUG("vm=%p", vm); - - if (qemuMigrationDstWaitForCompletion(vm, - VIR_ASYNC_JOB_MIGRATION_IN, - false) < 0) { - return -1; - } - - return 0; -} - - static virDomainPtr qemuMigrationDstFinishActive(virQEMUDriver *driver, virConnectPtr dconn, @@ -6753,7 +6738,7 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver, } if (flags & VIR_MIGRATE_POSTCOPY_RESUME) { - rc = qemuMigrationDstFinishResume(vm); + rc = qemuMigrationDstWaitForCompletion(vm, VIR_ASYNC_JOB_MIGRATION_IN, false); inPostCopy = true; } else { rc = qemuMigrationDstFinishFresh(driver, vm, mig, flags, v3proto, -- 2.45.2
'qemuProcessStop()' clears the 'current' job data. While the code under the 'error' label in 'qemuMigrationSrcRun()' does check that the VM is active before accessing the job, it also invokes multiple helper functions to clean up the migration including 'qemuMigrationSrcNBDCopyCancel()' which calls 'qemuDomainObjWait()' invalidating the result of the liveness check as it unlocks the VM. Duplicate the liveness check and explain why. The rest of the code e.g. accessing the monitor is safe as 'qemuDomainEnterMonitorAsync()' performs a liveness check. The cleanup path just ignores the return values of those functions. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 3524915e9d..89ddc586bd 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -5074,7 +5074,13 @@ qemuMigrationSrcRun(virQEMUDriver *driver, dconn); qemuMigrationSrcCancelRemoveTempBitmaps(vm, VIR_ASYNC_JOB_MIGRATION_OUT); + } + /* We need to re-check that the VM is active as functions like + * qemuMigrationSrcCancel/qemuMigrationSrcNBDCopyCancel wait on the VM + * condition unlocking the VM object which can lead to a cleanup of the + * 'current' job via qemuProcessStop */ + if (qemuDomainObjIsActive(vm)) { if (vm->job->current->status != VIR_DOMAIN_JOB_STATUS_CANCELED) vm->job->current->status = VIR_DOMAIN_JOB_STATUS_FAILED; } -- 2.45.2
When a VM terminates itself while it's being migrated in running state libvirt would report wrong error: error: cannot get locked memory limit of process 2502057: No such file or directory rather than the proper error: error: operation failed: domain is not running Remember the error on error paths in qemuMigrationSrcConfirmPhase and qemuMigrationSrcPerformPhase. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 89ddc586bd..26c082fc08 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -4015,8 +4015,6 @@ qemuMigrationSrcConfirmPhase(virQEMUDriver *driver, qemuMigrationSrcNBDCopyCancel(vm, false, VIR_ASYNC_JOB_MIGRATION_OUT, NULL); - virErrorRestore(&orig_err); - if (virDomainObjGetState(vm, &reason) == VIR_DOMAIN_PAUSED && reason == VIR_DOMAIN_PAUSED_POSTCOPY) { qemuMigrationSrcPostcopyFailed(vm); @@ -4029,6 +4027,7 @@ qemuMigrationSrcConfirmPhase(virQEMUDriver *driver, } qemuDomainSaveStatus(vm); + virErrorRestore(&orig_err); } return 0; @@ -6230,11 +6229,15 @@ qemuMigrationSrcPerformPhase(virQEMUDriver *driver, cleanup: if (ret < 0 && !virDomainObjIsFailedPostcopy(vm, vm->job)) { + virErrorPtr orig_err; + virErrorPreserveLast(&orig_err); + qemuMigrationSrcRestoreDomainState(driver, vm); qemuMigrationParamsReset(vm, VIR_ASYNC_JOB_MIGRATION_OUT, jobPriv->migParams, vm->job->apiFlags); qemuDomainSetMaxMemLock(vm, 0, &priv->preMigrationMemlock); qemuMigrationJobFinish(vm); + virErrorRestore(&orig_err); } else { if (ret < 0) ignore_value(qemuMigrationJobSetPhase(vm, QEMU_MIGRATION_PHASE_POSTCOPY_FAILED)); -- 2.45.2
On 6/13/24 17:11, Peter Krempa wrote:
When a VM terminates itself while it's being migrated in running state libvirt would report wrong error:
error: cannot get locked memory limit of process 2502057: No such file or directory
rather than the proper error:
error: operation failed: domain is not running
Remember the error on error paths in qemuMigrationSrcConfirmPhase and qemuMigrationSrcPerformPhase.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
Trying to NOT open pandora's box: sometimes I wish we had stacked error messages. Michal
On 6/13/24 17:11, Peter Krempa wrote:
The daemon crashes due to unexpected cleanup happening due to bad assumptions about locking and state. See patch 5.
Peter Krempa (12): qemuBlockJobProcessEventConcludedBackup: Handle potentially NULL 'job->disk' qemuDomainDiskPrivateDispose: Prevent dangling 'disk' pointer in blockjob data qemuDomainDeviceBackendChardevForeach: Fix typo in comment qemuDomainObjWait: Add documentation qemuProcessStop: Prevent crash when qemuDomainObjStopWorker() unlocks the VM qemuProcessStop: Move code not depending on 'vm->def->id' after reset of the ID qemu: process: Ensure that 'beingDestroyed' gets cleared only after VM id is reset qemu: domain: Introduce qemuDomainObjIsActive helper qemu: migration: Properly check for live VM after qemuDomainObjWait() qemu: migration: Inline 'qemuMigrationDstFinishResume()' qemuMigrationSrcRun: Re-check whether VM is active before accessing job data qemu: migration: Preserve error across qemuDomainSetMaxMemLock() on error paths
src/qemu/qemu_backup.c | 6 +-- src/qemu/qemu_backup.h | 2 +- src/qemu/qemu_blockjob.c | 9 +++- src/qemu/qemu_domain.c | 40 +++++++++++++- src/qemu/qemu_domain.h | 2 + src/qemu/qemu_migration.c | 43 +++++++--------- src/qemu/qemu_process.c | 106 ++++++++++++++++++++++---------------- 7 files changed, 131 insertions(+), 77 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Prívozník -
Peter Krempa