This was found while researching the root cause of: https://bugzilla.redhat.com/show_bug.cgi?id=670848 virDomainUnref should only be called with the lock held for the virDomainObj in question. However, when a transient qemu domain gets EOF on its monitor socket, it queues an event which frees the monitor, which unref's the virDomainObj without first locking it. If another thread has already locked the virDomainObj, the modification of the refcount could potentially be corrupted. In an extreme case, it could also be potentially unlocked by virDomainObjFree, thus left open to modification by anyone else who would have otherwise waited for the lock (not to mention the fact that they would be accessing freed data!). The solution is to have qemuMonitorFree lock the domain object right before unrefing it. Since the caller to qemuMonitorFree doesn't expect this lock to be held, if the refcount doesn't go all the way to 0, qemuMonitorFree must unlock it after the unref.

--- src/qemu/qemu_process.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index c419c75..1d67b3c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -596,7 +596,9 @@ static void qemuProcessHandleMonitorDestroy(qemuMonitorPtr mon, qemuDomainObjPrivatePtr priv = vm->privateData; if (priv->mon == mon) priv->mon = NULL;

- virDomainObjUnref(vm); + virDomainObjLock(vm);

+ if (virDomainObjUnref(vm) > 0) + virDomainObjUnlock(vm); } static qemuMonitorCallbacks monitorCallbacks = {

(Eric pointed out in IRC that I should be acquiring the domainobj lock prior to modifying obj->privateData->mon)

The solution is to have qemuMonitorFree lock the domain object right before unrefing it. Since the caller to qemuMonitorFree doesn't expect this lock to be held, if the refcount doesn't go all the way to 0, qemuMonitorFree must unlock it after the unref. --- src/qemu/qemu_process.c | 8 ++++++-- 1 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index c419c75..9084725 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -593,10 +593,14 @@ no_memory: static void qemuProcessHandleMonitorDestroy(qemuMonitorPtr mon, virDomainObjPtr vm) { - qemuDomainObjPrivatePtr priv = vm->privateData; + qemuDomainObjPrivatePtr priv; + + virDomainObjLock(vm); + priv = vm->privateData; if (priv->mon == mon) priv->mon = NULL; - virDomainObjUnref(vm); + if (virDomainObjUnref(vm) > 0) + virDomainObjUnlock(vm); }

> +++ b/src/qemu/qemu_process.c > @@ -596,7 +596,9 @@ static void qemuProcessHandleMonitorDestroy(qemuMonitorPtr mon, > qemuDomainObjPrivatePtr priv = vm->privateData; > if (priv->mon == mon) > priv->mon = NULL; > - virDomainObjUnref(vm); > + virDomainObjLock(vm); > + if (virDomainObjUnref(vm) > 0) > + virDomainObjUnlock(vm); Phooey, this causes 'virsh save domain file' to deadlock. I'm still looking into why, but it may be that we're going about fixing this wrong. Maybe we should be making sure that qemuProcessStop ensures that the monitor callback function won't trigger if there is no domain for it to trigger on.

On 03/04/2011 02:46 PM, Eric Blake wrote: >> +++ b/src/qemu/qemu_process.c >> @@ -596,7 +596,9 @@ static void qemuProcessHandleMonitorDestroy(qemuMonitorPtr mon, >> qemuDomainObjPrivatePtr priv = vm->privateData; >> if (priv->mon == mon) >> priv->mon = NULL; >> - virDomainObjUnref(vm); >> + virDomainObjLock(vm); >> + if (virDomainObjUnref(vm) > 0) >> + virDomainObjUnlock(vm); > > Phooey, this causes 'virsh save domain file' to deadlock. > > I'm still looking into why, but it may be that we're going about fixing > this wrong. Maybe we should be making sure that qemuProcessStop ensures > that the monitor callback function won't trigger if there is no domain > for it to trigger on. I think the real reason for the deadlock is that: qemudDomainSaveFlag holds the domain lock while it calls qemuDomainEventQueue, which requests the event loop lock the event loop thread holds the event loop lock, and can make several different callbacks that can result in requesting a domain lock

This was also found while investigating https://bugzilla.redhat.com/show_bug.cgi?id=670848 An EOF on a domain's monitor socket results in an event being queued to handle the EOF. The handler calls qemuProcessHandleMonitorEOF. If it is a transient domain, this leads to a call to virDomainRemoveInactive, which removes the domain from the driver's hashtable and unref's it. Nowhere in this code is the qemu driver lock acquired. However, all modifications to the driver's domain hashtable *must* be done while holding the driver lock, otherwise the hashtable can become corrupt, and (even more likely) another thread could call a different hashtable function and acquire a pointer to the domain that is in the process of being destroyed. To prevent such a disaster, qemuProcessHandleMonitorEOF must get the qemu driver lock *before* it gets the DomainObj's lock, and hold it until it is finished with the DomainObj. This guarantees that nobody else modifies the hashtable at the same time, and that anyone who had already gotten the DomainObj from the hashtable prior to this call has finished with it before we remove/destroy it. --- src/qemu/qemu_process.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1d67b3c..6a65a33 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -107,11 +107,13 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED, VIR_DEBUG("Received EOF on %p '%s'", vm, vm->def->name); + qemuDriverLock(driver); virDomainObjLock(vm); if (!virDomainObjIsActive(vm)) { VIR_DEBUG("Domain %p is not active, ignoring EOF", vm); virDomainObjUnlock(vm); + qemuDriverUnlock(driver); return; } @@ -137,10 +139,9 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED, virDomainObjUnlock(vm); if (event) { - qemuDriverLock(driver); qemuDomainEventQueue(driver, event); - qemuDriverUnlock(driver); } + qemuDriverUnlock(driver); }