On Thu, Jul 25, 2024 at 12:57:59PM +0200, Michal Privoznik wrote:
Currently, qemuProcessStop() unlocks given domain object right in the middle of cleanup process. This is dangerous because there might be another thread which is executing virDomainObjListAdd(). And since the domain object is on the list of domain objects AND by the time qemuProcessStop() unlocks it the object is also marked as inactive, the other thread acquires the lock and switches vm->def pointer. The unlocking of domain object is needed though, to allow even processing thread finish its queue. Well, the processing can be done before any cleanup is attempted. Therefore, use freshly introduced virEventThreadStop() to join the event thread and drop lock/unlock from the middle of qemuProcessStop(). Fixes: 3865410e7f67ca4ec66e9a905e75f452762a97f0 Resolves: https://issues.redhat.com/browse/RHEL-49607 Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> --- src/qemu/qemu_process.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6255ba92e7..0869307a90 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8615,6 +8615,14 @@ void qemuProcessStop(virQEMUDriver *driver, * reporting so we don't squash a legit error. */ virErrorPreserveLast(&orig_err); + /* By unlocking the domain object the events processing thread is + * allowed to finish its job. */ + if (priv->eventThread) { + virObjectUnlock(vm); + virEventThreadStop(priv->eventThread); + virObjectLock(vm); + } +
I think we would be even "safer" if this was done after the job was acquired. Other than that I think that's fine.
if (asyncJob != VIR_ASYNC_JOB_NONE) {
if (virDomainObjBeginNestedJob(vm, asyncJob) < 0)
goto cleanup;
@@ -8681,10 +8689,8 @@ 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);
+ if (priv->eventThread)
+ g_object_unref(g_steal_pointer(&priv->eventThread));
More often we use `g_clear_pointer(ptr, g_object_unref)` instead even though it does the same thing. Reviewed-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
if (!!g_atomic_int_dec_and_test(&driver->nactive) && driver->inhibitCallback)
driver->inhibitCallback(false, driver->inhibitOpaque);
--
2.44.2
Attachment:
signature.asc
Description: PGP signature
