On Wed, Aug 07, 2024 at 14:44:56 +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> > --- I've requested that the commit message carries an explanation why you can change the code to contradict what the coment, which is being deleted below states. > src/qemu/qemu_process.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index cec739c984..a07ce50fbf 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -8692,6 +8692,16 @@ void qemuProcessStop(virQEMUDriver *driver, > VIR_QEMU_PROCESS_KILL_FORCE| > VIR_QEMU_PROCESS_KILL_NOCHECK)); > > + /* By unlocking the domain object the events processing thread is allowed > + * to finish its job. 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). */ > + if (priv->eventThread) { I've also requested to explicitly set 'priv->beingDestroyed' before unlocking as qemuProcessStop may be called from code paths which don't do that. With both of the above addressed as I've requested in previous review: Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx>
