On Fri, Jul 26, 2024 at 11:01:43 +0200, Peter Krempa wrote: > On Thu, Jul 25, 2024 at 15:04:51 +0200, Michal Prívozník wrote: > > On 7/25/24 14:33, Peter Krempa wrote: > > > On Thu, Jul 25, 2024 at 12:57:59 +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, > > > So we're still in qemuProcessStop ... > > > > >> * 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. */ Based on what this patch is fixing I'd also suggest adding basically the exact opposite warning than there was originally below. That any unlocking must happen before resetting vm->def->id as the global domain object list code depends on it (and it can't actually check 'priv->beingDestroyed as that's private).
