Signaling the condition before vm->def->id is reset to -1 is dangerous:
in case a waiting thread wakes up, it does not see anything interesting
(the domain is still marked as running) and just enters virDomainObjWait
where it waits forever because the condition will never be signalled
again.

Originally it was impossible to get into such situation because the vm
object was locked all the time between signaling the condition and
resetting vm->def->id, but after commit 860a999802 released in 6.8.0,
qemuDomainObjStopWorker called in qemuProcessStop between
virDomainObjBroadcast and setting vm->def->id to -1 unlocks the vm
object giving other threads a chance to wake up and possibly hang.

In real world, this can be easily reproduced by killing, destroying, or
just shutting down (from the guest OS) a domain while it is being
migrated somewhere else. The migration job would never finish.

We can't fix this by reseting vm->def->id earlier because other
functions (such as qemuProcessKill) do nothing when the domain is
already marked as inactive. So let's make sure we delay signaling the
domain condition to the point when a woken up thread can detect the
domain is not active anymore.

https://bugzilla.redhat.com/show_bug.cgi?id=1949869

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---
 src/qemu/qemu_process.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index c972c90801..914f936e45 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -7852,9 +7852,6 @@ void qemuProcessStop(virQEMUDriver *driver,
     if (!!g_atomic_int_dec_and_test(&driver->nactive) && driver->inhibitCallback)
         driver->inhibitCallback(false, driver->inhibitOpaque);
 
-    /* Wake up anything waiting on domain condition */
-    virDomainObjBroadcast(vm);
-
     if ((timestamp = virTimeStringNow()) != NULL) {
         qemuDomainLogAppendMessage(driver, vm, "%s: shutting down, reason=%s\n",
                                    timestamp,
@@ -7925,6 +7922,9 @@ void qemuProcessStop(virQEMUDriver *driver,
 
     vm->def->id = -1;
 
+    /* Wake up anything waiting on domain condition */
+    virDomainObjBroadcast(vm);
+
     virFileDeleteTree(priv->libDir);
     virFileDeleteTree(priv->channelTargetDir);
 
-- 
2.32.0

On 7/16/21 5:06 PM, Jiri Denemark wrote:
> Signaling the condition before vm->def->id is reset to -1 is dangerous:
> in case a waiting thread wakes up, it does not see anything interesting
> (the domain is still marked as running) and just enters virDomainObjWait
> where it waits forever because the condition will never be signalled
> again.
> 
> Originally it was impossible to get into such situation because the vm
> object was locked all the time between signaling the condition and
> resetting vm->def->id, but after commit 860a999802 released in 6.8.0,
> qemuDomainObjStopWorker called in qemuProcessStop between
> virDomainObjBroadcast and setting vm->def->id to -1 unlocks the vm
> object giving other threads a chance to wake up and possibly hang.
> 
> In real world, this can be easily reproduced by killing, destroying, or
> just shutting down (from the guest OS) a domain while it is being
> migrated somewhere else. The migration job would never finish.
> 
> We can't fix this by reseting vm->def->id earlier because other
> functions (such as qemuProcessKill) do nothing when the domain is
> already marked as inactive. So let's make sure we delay signaling the

Not true really. The VIR_QEMU_PROCESS_KILL_NOCHECK flag is passed to
qemuProcessKill which means the check for domain state (aka vm->def->id
== -1) is skipped.

> domain condition to the point when a woken up thread can detect the
> domain is not active anymore.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1949869
> 
> Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> ---
>  src/qemu/qemu_process.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index c972c90801..914f936e45 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -7852,9 +7852,6 @@ void qemuProcessStop(virQEMUDriver *driver,
>      if (!!g_atomic_int_dec_and_test(&driver->nactive) && driver->inhibitCallback)
>          driver->inhibitCallback(false, driver->inhibitOpaque);
>  
> -    /* Wake up anything waiting on domain condition */
> -    virDomainObjBroadcast(vm);
> -
>      if ((timestamp = virTimeStringNow()) != NULL) {
>          qemuDomainLogAppendMessage(driver, vm, "%s: shutting down, reason=%s\n",
>                                     timestamp,
> @@ -7925,6 +7922,9 @@ void qemuProcessStop(virQEMUDriver *driver,
>  
>      vm->def->id = -1;
>  
> +    /* Wake up anything waiting on domain condition */
> +    virDomainObjBroadcast(vm);
> +
>      virFileDeleteTree(priv->libDir);
>      virFileDeleteTree(priv->channelTargetDir);
>  
> 

Right. The fix is correct, and for that you have my:

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

But I think we can do some re-ordering, because apparently we put calls
at random places in this function. For instance, nbd port is released
before QEMU is killed, which creates a window where QEMU might still be
running and have that port open BUT our port allocator has the port
marked as free already. But that's something we can fix some other time.

Michal

On Mon, Jul 19, 2021 at 09:37:01 +0200, Michal Prívozník wrote:
> On 7/16/21 5:06 PM, Jiri Denemark wrote:
> > Signaling the condition before vm->def->id is reset to -1 is dangerous:
> > in case a waiting thread wakes up, it does not see anything interesting
> > (the domain is still marked as running) and just enters virDomainObjWait
> > where it waits forever because the condition will never be signalled
> > again.
> > 
> > Originally it was impossible to get into such situation because the vm
> > object was locked all the time between signaling the condition and
> > resetting vm->def->id, but after commit 860a999802 released in 6.8.0,
> > qemuDomainObjStopWorker called in qemuProcessStop between
> > virDomainObjBroadcast and setting vm->def->id to -1 unlocks the vm
> > object giving other threads a chance to wake up and possibly hang.
> > 
> > In real world, this can be easily reproduced by killing, destroying, or
> > just shutting down (from the guest OS) a domain while it is being
> > migrated somewhere else. The migration job would never finish.
> > 
> > We can't fix this by reseting vm->def->id earlier because other
> > functions (such as qemuProcessKill) do nothing when the domain is
> > already marked as inactive. So let's make sure we delay signaling the
> 
> Not true really. The VIR_QEMU_PROCESS_KILL_NOCHECK flag is passed to
> qemuProcessKill which means the check for domain state (aka vm->def->id
> == -1) is skipped.

Heh, I completely missed this flag when looking at the code :-/

I dropped this part of the commit message.

Jirka