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(a)redhat.com>
> >> ---
> >> 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).