6:37 a.m.
The NWFilter code has as a deadlock race condition between
the virNWFilter{Define,Undefine} APIs and starting of guest
VMs due to mis-matched lock ordering.
In the virNWFilter{Define,Undefine} codepaths the lock ordering
is
1. nwfilter driver lock
2. virt driver lock
3. nwfilter update lock
4. domain object lock
In the VM guest startup paths the lock ordering is
1. virt driver lock
2. domain object lock
3. nwfilter update lock
As can be seen the domain object and nwfilter update locks are
not acquired in a consistent order.
The fix used is to push the nwfilter update lock upto the top
level resulting in a lock ordering for virNWFilter{Define,Undefine}
of
1. nwfilter driver lock
2. nwfilter update lock
3. virt driver lock
4. domain object lock
and VM start using
1. nwfilter update lock
2. virt driver lock
3. domain object lock
This has the effect of serializing VM startup once again, even if
no nwfilters are applied to the guest.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/conf/nwfilter_conf.c | 6 ------
src/lxc/lxc_driver.c | 6 ++++++
src/nwfilter/nwfilter_driver.c | 8 ++++----
src/nwfilter/nwfilter_gentech_driver.c | 6 ------
src/qemu/qemu_driver.c | 6 ++++++
src/uml/uml_driver.c | 4 ++++
6 files changed, 20 insertions(+), 16 deletions(-)
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
index 6db8ea9..e712ca5 100644
--- a/src/conf/nwfilter_conf.c
+++ b/src/conf/nwfilter_conf.c
@@ -2990,14 +2990,12 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters,
return NULL;
}
- virNWFilterLockFilterUpdates();
if ((nwfilter = virNWFilterObjFindByName(nwfilters, def->name))) {
if (virNWFilterDefEqual(def, nwfilter->def, false)) {
virNWFilterDefFree(nwfilter->def);
nwfilter->def = def;
- virNWFilterUnlockFilterUpdates();
return nwfilter;
}
@@ -3005,7 +3003,6 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters,
/* trigger the update on VMs referencing the filter */
if (virNWFilterTriggerVMFilterRebuild()) {
nwfilter->newDef = NULL;
- virNWFilterUnlockFilterUpdates();
virNWFilterObjUnlock(nwfilter);
return NULL;
}
@@ -3013,12 +3010,9 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters,
virNWFilterDefFree(nwfilter->def);
nwfilter->def = def;
nwfilter->newDef = NULL;
- virNWFilterUnlockFilterUpdates();
return nwfilter;
}
- virNWFilterUnlockFilterUpdates();
-
if (VIR_ALLOC(nwfilter) < 0)
return NULL;
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index e319234..b1f8a89 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -1015,6 +1015,8 @@ static int lxcDomainCreateWithFiles(virDomainPtr dom,
virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1);
+ virNWFilterLockFilterUpdates();
+
if (!(vm = lxcDomObjFromDomain(dom)))
goto cleanup;
@@ -1053,6 +1055,7 @@ cleanup:
if (event)
virObjectEventStateQueue(driver->domainEventState, event);
virObjectUnref(cfg);
+ virNWFilterUnlockFilterUpdates();
return ret;
}
@@ -1109,6 +1112,8 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn,
virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, NULL);
+ virNWFilterLockFilterUpdates();
+
if (!(caps = virLXCDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -1164,6 +1169,7 @@ cleanup:
virObjectEventStateQueue(driver->domainEventState, event);
virObjectUnref(caps);
virObjectUnref(cfg);
+ virNWFilterUnlockFilterUpdates();
return dom;
}
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index d21dd82..2972731 100644
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -554,6 +554,7 @@ nwfilterDefineXML(virConnectPtr conn,
virNWFilterPtr ret = NULL;
nwfilterDriverLock(driver);
+ virNWFilterLockFilterUpdates();
virNWFilterCallbackDriversLock();
if (!(def = virNWFilterDefParseString(xml)))
@@ -580,6 +581,7 @@ cleanup:
virNWFilterObjUnlock(nwfilter);
virNWFilterCallbackDriversUnlock();
+ virNWFilterUnlockFilterUpdates();
nwfilterDriverUnlock(driver);
return ret;
}
@@ -592,9 +594,8 @@ nwfilterUndefine(virNWFilterPtr obj) {
int ret = -1;
nwfilterDriverLock(driver);
- virNWFilterCallbackDriversLock();
-
virNWFilterLockFilterUpdates();
+ virNWFilterCallbackDriversLock();
nwfilter = virNWFilterObjFindByUUID(&driver->nwfilters, obj->uuid);
if (!nwfilter) {
@@ -626,9 +627,8 @@ cleanup:
if (nwfilter)
virNWFilterObjUnlock(nwfilter);
- virNWFilterUnlockFilterUpdates();
-
virNWFilterCallbackDriversUnlock();
+ virNWFilterUnlockFilterUpdates();
nwfilterDriverUnlock(driver);
return ret;
}
diff --git a/src/nwfilter/nwfilter_gentech_driver.c
b/src/nwfilter/nwfilter_gentech_driver.c
index 89913cf8..f0e78ed 100644
--- a/src/nwfilter/nwfilter_gentech_driver.c
+++ b/src/nwfilter/nwfilter_gentech_driver.c
@@ -935,7 +935,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,
int ifindex;
int rc;
- virNWFilterLockFilterUpdates();
/* after grabbing the filter update lock check for the interface; if
it's not there anymore its filters will be or are being removed
@@ -964,7 +963,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,
foundNewFilter);
cleanup:
- virNWFilterUnlockFilterUpdates();
return rc;
}
@@ -984,8 +982,6 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver,
int rc;
bool foundNewFilter = false;
- virNWFilterLockFilterUpdates();
-
rc = __virNWFilterInstantiateFilter(driver,
vmuuid,
true,
@@ -1009,8 +1005,6 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver,
}
}
- virNWFilterUnlockFilterUpdates();
-
return rc;
}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bc29714..2e55cfd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1576,6 +1576,8 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn,
if (flags & VIR_DOMAIN_START_AUTODESTROY)
start_flags |= VIR_QEMU_PROCESS_START_AUTODESTROY;
+ virNWFilterLockFilterUpdates();
+
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -1656,6 +1658,7 @@ cleanup:
}
virObjectUnref(caps);
virObjectUnref(qemuCaps);
+ virNWFilterUnlockFilterUpdates();
return dom;
}
@@ -6095,6 +6098,8 @@ qemuDomainCreateWithFlags(virDomainPtr dom, unsigned int flags)
VIR_DOMAIN_START_BYPASS_CACHE |
VIR_DOMAIN_START_FORCE_BOOT, -1);
+ virNWFilterLockFilterUpdates();
+
if (!(vm = qemuDomObjFromDomain(dom)))
return -1;
@@ -6122,6 +6127,7 @@ endjob:
cleanup:
if (vm)
virObjectUnlock(vm);
+ virNWFilterUnlockFilterUpdates();
return ret;
}
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index f286f41..19b5f62 100644
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -1574,6 +1574,7 @@ static virDomainPtr umlDomainCreateXML(virConnectPtr conn, const
char *xml,
virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, NULL);
+ virNWFilterLockFilterUpdates();
umlDriverLock(driver);
if (!(def = virDomainDefParseString(xml, driver->caps, driver->xmlopt,
1 << VIR_DOMAIN_VIRT_UML,
@@ -1613,6 +1614,7 @@ cleanup:
if (event)
umlDomainEventQueue(driver, event);
umlDriverUnlock(driver);
+ virNWFilterUnlockFilterUpdates();
return dom;
}
@@ -1997,6 +1999,7 @@ static int umlDomainCreateWithFlags(virDomainPtr dom, unsigned int
flags) {
virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1);
+ virNWFilterLockFilterUpdates();
umlDriverLock(driver);
vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
@@ -2023,6 +2026,7 @@ cleanup:
if (event)
umlDomainEventQueue(driver, event);
umlDriverUnlock(driver);
+ virNWFilterUnlockFilterUpdates();
return ret;
}
--
1.8.4.2