7:33 a.m.
From: "Daniel P. Berrange" <berrange(a)redhat.com>
There is a lock ordering problem in the QEMU close callback
APIs.
When starting a guest we have a lock on the VM. We then
set a autodestroy callback, which acquires a lock on the
close callbacks.
When running auto-destroy, we obtain a lock on the close
callbacks, then run each callbacks - which obtains a lock
on the VM.
This causes deadlock if anyone tries to start a VM, while
autodestroy is taking place.
The fix is to do autodestroy in 2 phases. First obtain
all the callbacks and remove them from the list under
the close callback lock. Then invoke each callback
from outside the close callback lock.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/qemu/qemu_conf.c | 106 ++++++++++++++++++++++++++++++++++++++++++--------
src/util/virnetlink.c | 5 ++-
2 files changed, 92 insertions(+), 19 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 1cd4b7c..f3b09cf 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -797,20 +797,37 @@ virQEMUCloseCallbacksGet(virQEMUCloseCallbacksPtr closeCallbacks,
return cb;
}
+
+typedef struct _virQEMUCloseCallbacksListEntry virQEMUCloseCallbacksListEntry;
+typedef virQEMUCloseCallbacksListEntry *virQEMUCloseCallbacksListEntryPtr;
+struct _virQEMUCloseCallbacksListEntry {
+ unsigned char uuid[VIR_UUID_BUFLEN];
+ virQEMUCloseCallback callback;
+};
+
+
+typedef struct _virQEMUCloseCallbacksList virQEMUCloseCallbacksList;
+typedef virQEMUCloseCallbacksList *virQEMUCloseCallbacksListPtr;
+struct _virQEMUCloseCallbacksList {
+ size_t nentries;
+ virQEMUCloseCallbacksListEntryPtr entries;
+};
+
+
struct virQEMUCloseCallbacksData {
- virHashTablePtr list;
- virQEMUDriverPtr driver;
virConnectPtr conn;
+ virQEMUCloseCallbacksListPtr list;
+ bool oom;
};
+
static void
-virQEMUCloseCallbacksRunOne(void *payload,
+virQEMUCloseCallbacksGetOne(void *payload,
const void *key,
void *opaque)
{
struct virQEMUCloseCallbacksData *data = opaque;
qemuDriverCloseDefPtr closeDef = payload;
- virDomainObjPtr dom;
const char *uuidstr = key;
unsigned char uuid[VIR_UUID_BUFLEN];
@@ -823,35 +840,90 @@ virQEMUCloseCallbacksRunOne(void *payload,
if (data->conn != closeDef->conn || !closeDef->cb)
return;
- if (!(dom = virDomainObjListFindByUUID(data->driver->domains, uuid))) {
- VIR_DEBUG("No domain object with UUID %s", uuidstr);
+ if (VIR_EXPAND_N(data->list->entries,
+ data->list->nentries, 1) < 0) {
+ data->oom = true;
return;
}
- dom = closeDef->cb(data->driver, dom, data->conn);
- if (dom)
- virObjectUnlock(dom);
+ memcpy(data->list->entries[data->list->nentries - 1].uuid,
+ uuid, VIR_UUID_BUFLEN);
+ data->list->entries[data->list->nentries - 1].callback =
closeDef->cb;
+}
+
+
+static virQEMUCloseCallbacksListPtr
+virQEMUCloseCallbacksGetForConn(virQEMUCloseCallbacksPtr closeCallbacks,
+ virConnectPtr conn)
+{
+ virQEMUCloseCallbacksListPtr list = NULL;
+ struct virQEMUCloseCallbacksData data;
+
+ if (VIR_ALLOC(list) < 0) {
+ virReportOOMError();
+ return NULL;
+ }
+
+ data.conn = conn;
+ data.list = list;
+ data.oom = false;
+
+ virHashForEach(closeCallbacks->list, virQEMUCloseCallbacksGetOne, &data);
- virHashRemoveEntry(data->list, uuid);
+ if (data.oom) {
+ VIR_FREE(list->entries);
+ VIR_FREE(list);
+ virReportOOMError();
+ return NULL;
+ }
+
+ return list;
}
+
void
virQEMUCloseCallbacksRun(virQEMUCloseCallbacksPtr closeCallbacks,
virConnectPtr conn,
virQEMUDriverPtr driver)
{
- struct virQEMUCloseCallbacksData data = {
- .driver = driver,
- .conn = conn
- };
+ virQEMUCloseCallbacksListPtr list;
+ size_t i;
VIR_DEBUG("conn=%p", conn);
- virObjectLock(closeCallbacks);
- data.list = closeCallbacks->list;
- virHashForEach(data.list, virQEMUCloseCallbacksRunOne, &data);
+ /* We must not hold the lock while running the callbacks,
+ * so first we obtain the list of callbacks, then remove
+ * them all from the hash. At that point we can release
+ * the lock and run the callbacks safely. */
+ virObjectLock(closeCallbacks);
+ list = virQEMUCloseCallbacksGetForConn(closeCallbacks, conn);
+ if (!list)
+ return;
+
+ for (i = 0 ; i < list->nentries ; i++) {
+ virHashRemoveEntry(closeCallbacks->list,
+ list->entries[i].uuid);
+ }
virObjectUnlock(closeCallbacks);
+
+ for (i = 0 ; i < list->nentries ; i++) {
+ virDomainObjPtr vm;
+
+ if (!(vm = virDomainObjListFindByUUID(driver->domains,
+ list->entries[i].uuid))) {
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
+ virUUIDFormat(list->entries[i].uuid, uuidstr);
+ VIR_DEBUG("No domain object with UUID %s", uuidstr);
+ continue;
+ }
+
+ vm = list->entries[i].callback(driver, vm, conn);
+ if (vm)
+ virObjectUnlock(vm);
+ }
+ VIR_FREE(list->entries);
+ VIR_FREE(list);
}
struct _qemuSharedDiskEntry {
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
index 0b36fdc..8b47ede 100644
--- a/src/util/virnetlink.c
+++ b/src/util/virnetlink.c
@@ -335,8 +335,9 @@ virNetlinkEventCallback(int watch,
if (length == 0)
return;
if (length < 0) {
- virReportSystemError(errno,
- "%s", _("nl_recv returned with
error"));
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("nl_recv returned with error: %s"),
+ nl_geterror(length));
return;
}
--
1.7.11.7