提交 2331e5c8 编写于 作者: D Daniel P. Berrange 提交者: Laine Stump

Push nwfilter update locking up to top level

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. There is also the possibility
of deadlock due to a call graph loop via virNWFilterInstantiate
and virNWFilterInstantiateFilterLate.

These two problems mean the lock must be turned into a read/write
lock instead of a plain mutex at the same time. The lock is used to
serialize changes to the "driver->nwfilters" hash, so the write lock
only needs to be held by the define/undefine methods. All other
methods can rely on a read lock which allows good concurrency.
Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>
(cherry picked from commit 6e5c79a1)

Conflicts:
	src/conf/nwfilter_conf.c
          - virReportOOMError() in context of one hunk.
	src/lxc/lxc_driver.c
          - functions renamed, and lxc object locking changed, creating
            a conflict in the context.
上级 8e48acae
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
* nwfilter_conf.c: network filter XML processing * nwfilter_conf.c: network filter XML processing
* (derived from storage_conf.c) * (derived from storage_conf.c)
* *
* Copyright (C) 2006-2012 Red Hat, Inc. * Copyright (C) 2006-2012, 2014 Red Hat, Inc.
* Copyright (C) 2006-2008 Daniel P. Berrange * Copyright (C) 2006-2008 Daniel P. Berrange
* *
* Copyright (C) 2010-2011 IBM Corporation * Copyright (C) 2010-2011 IBM Corporation
...@@ -143,17 +143,22 @@ static const struct int_map chain_priorities[] = { ...@@ -143,17 +143,22 @@ static const struct int_map chain_priorities[] = {
/* /*
* only one filter update allowed * only one filter update allowed
*/ */
static virMutex updateMutex; static virRWLock updateLock;
static bool initialized = false; static bool initialized = false;
void void
virNWFilterLockFilterUpdates(void) { virNWFilterReadLockFilterUpdates(void) {
virMutexLock(&updateMutex); virRWLockRead(&updateLock);
}
void
virNWFilterWriteLockFilterUpdates(void) {
virRWLockWrite(&updateLock);
} }
void void
virNWFilterUnlockFilterUpdates(void) { virNWFilterUnlockFilterUpdates(void) {
virMutexUnlock(&updateMutex); virRWLockUnlock(&updateLock);
} }
...@@ -2992,14 +2997,12 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, ...@@ -2992,14 +2997,12 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters,
return NULL; return NULL;
} }
virNWFilterLockFilterUpdates();
if ((nwfilter = virNWFilterObjFindByName(nwfilters, def->name))) { if ((nwfilter = virNWFilterObjFindByName(nwfilters, def->name))) {
if (virNWFilterDefEqual(def, nwfilter->def, false)) { if (virNWFilterDefEqual(def, nwfilter->def, false)) {
virNWFilterDefFree(nwfilter->def); virNWFilterDefFree(nwfilter->def);
nwfilter->def = def; nwfilter->def = def;
virNWFilterUnlockFilterUpdates();
return nwfilter; return nwfilter;
} }
...@@ -3007,7 +3010,6 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, ...@@ -3007,7 +3010,6 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters,
/* trigger the update on VMs referencing the filter */ /* trigger the update on VMs referencing the filter */
if (virNWFilterTriggerVMFilterRebuild()) { if (virNWFilterTriggerVMFilterRebuild()) {
nwfilter->newDef = NULL; nwfilter->newDef = NULL;
virNWFilterUnlockFilterUpdates();
virNWFilterObjUnlock(nwfilter); virNWFilterObjUnlock(nwfilter);
return NULL; return NULL;
} }
...@@ -3015,12 +3017,9 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, ...@@ -3015,12 +3017,9 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters,
virNWFilterDefFree(nwfilter->def); virNWFilterDefFree(nwfilter->def);
nwfilter->def = def; nwfilter->def = def;
nwfilter->newDef = NULL; nwfilter->newDef = NULL;
virNWFilterUnlockFilterUpdates();
return nwfilter; return nwfilter;
} }
virNWFilterUnlockFilterUpdates();
if (VIR_ALLOC(nwfilter) < 0) { if (VIR_ALLOC(nwfilter) < 0) {
virReportOOMError(); virReportOOMError();
return NULL; return NULL;
...@@ -3492,7 +3491,7 @@ int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB, ...@@ -3492,7 +3491,7 @@ int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB,
initialized = true; initialized = true;
if (virMutexInitRecursive(&updateMutex) < 0) if (virRWLockInit(&updateLock) < 0)
return -1; return -1;
return 0; return 0;
...@@ -3504,7 +3503,7 @@ void virNWFilterConfLayerShutdown(void) ...@@ -3504,7 +3503,7 @@ void virNWFilterConfLayerShutdown(void)
if (!initialized) if (!initialized)
return; return;
virMutexDestroy(&updateMutex); virRWLockDestroy(&updateLock);
initialized = false; initialized = false;
virNWFilterDomainFWUpdateOpaque = NULL; virNWFilterDomainFWUpdateOpaque = NULL;
......
...@@ -716,7 +716,8 @@ virNWFilterDefPtr virNWFilterDefParseFile(const char *filename); ...@@ -716,7 +716,8 @@ virNWFilterDefPtr virNWFilterDefParseFile(const char *filename);
void virNWFilterObjLock(virNWFilterObjPtr obj); void virNWFilterObjLock(virNWFilterObjPtr obj);
void virNWFilterObjUnlock(virNWFilterObjPtr obj); void virNWFilterObjUnlock(virNWFilterObjPtr obj);
void virNWFilterLockFilterUpdates(void); void virNWFilterWriteLockFilterUpdates(void);
void virNWFilterReadLockFilterUpdates(void);
void virNWFilterUnlockFilterUpdates(void); void virNWFilterUnlockFilterUpdates(void);
int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB, void *opaque); int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB, void *opaque);
......
...@@ -551,7 +551,6 @@ virNWFilterDefParseString; ...@@ -551,7 +551,6 @@ virNWFilterDefParseString;
virNWFilterInstFiltersOnAllVMs; virNWFilterInstFiltersOnAllVMs;
virNWFilterJumpTargetTypeToString; virNWFilterJumpTargetTypeToString;
virNWFilterLoadAllConfigs; virNWFilterLoadAllConfigs;
virNWFilterLockFilterUpdates;
virNWFilterObjAssignDef; virNWFilterObjAssignDef;
virNWFilterObjDeleteDef; virNWFilterObjDeleteDef;
virNWFilterObjFindByName; virNWFilterObjFindByName;
...@@ -563,6 +562,7 @@ virNWFilterObjSaveDef; ...@@ -563,6 +562,7 @@ virNWFilterObjSaveDef;
virNWFilterObjUnlock; virNWFilterObjUnlock;
virNWFilterPrintStateMatchFlags; virNWFilterPrintStateMatchFlags;
virNWFilterPrintTCPFlags; virNWFilterPrintTCPFlags;
virNWFilterReadLockFilterUpdates;
virNWFilterRegisterCallbackDriver; virNWFilterRegisterCallbackDriver;
virNWFilterRuleActionTypeToString; virNWFilterRuleActionTypeToString;
virNWFilterRuleDirectionTypeToString; virNWFilterRuleDirectionTypeToString;
...@@ -570,6 +570,7 @@ virNWFilterRuleProtocolTypeToString; ...@@ -570,6 +570,7 @@ virNWFilterRuleProtocolTypeToString;
virNWFilterTestUnassignDef; virNWFilterTestUnassignDef;
virNWFilterUnlockFilterUpdates; virNWFilterUnlockFilterUpdates;
virNWFilterUnRegisterCallbackDriver; virNWFilterUnRegisterCallbackDriver;
virNWFilterWriteLockFilterUpdates;
# conf/nwfilter_ipaddrmap.h # conf/nwfilter_ipaddrmap.h
......
/* /*
* Copyright (C) 2010-2013 Red Hat, Inc. * Copyright (C) 2010-2014 Red Hat, Inc.
* Copyright IBM Corp. 2008 * Copyright IBM Corp. 2008
* *
* lxc_driver.c: linux container driver functions * lxc_driver.c: linux container driver functions
...@@ -1102,6 +1102,8 @@ static int lxcDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) ...@@ -1102,6 +1102,8 @@ static int lxcDomainCreateWithFlags(virDomainPtr dom, unsigned int flags)
virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1); virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1);
virNWFilterReadLockFilterUpdates();
lxcDriverLock(driver); lxcDriverLock(driver);
vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
if (!vm) { if (!vm) {
...@@ -1146,6 +1148,7 @@ cleanup: ...@@ -1146,6 +1148,7 @@ cleanup:
if (event) if (event)
virDomainEventStateQueue(driver->domainEventState, event); virDomainEventStateQueue(driver->domainEventState, event);
lxcDriverUnlock(driver); lxcDriverUnlock(driver);
virNWFilterUnlockFilterUpdates();
return ret; return ret;
} }
...@@ -1184,6 +1187,8 @@ lxcDomainCreateXML(virConnectPtr conn, ...@@ -1184,6 +1187,8 @@ lxcDomainCreateXML(virConnectPtr conn,
virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, NULL); virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, NULL);
virNWFilterReadLockFilterUpdates();
lxcDriverLock(driver); lxcDriverLock(driver);
if (!(def = virDomainDefParseString(xml, driver->caps, driver->xmlopt, if (!(def = virDomainDefParseString(xml, driver->caps, driver->xmlopt,
1 << VIR_DOMAIN_VIRT_LXC, 1 << VIR_DOMAIN_VIRT_LXC,
...@@ -1235,6 +1240,7 @@ cleanup: ...@@ -1235,6 +1240,7 @@ cleanup:
if (event) if (event)
virDomainEventStateQueue(driver->domainEventState, event); virDomainEventStateQueue(driver->domainEventState, event);
lxcDriverUnlock(driver); lxcDriverUnlock(driver);
virNWFilterUnlockFilterUpdates();
return dom; return dom;
} }
......
...@@ -284,12 +284,14 @@ nwfilterStateReload(void) ...@@ -284,12 +284,14 @@ nwfilterStateReload(void)
virNWFilterLearnThreadsTerminate(true); virNWFilterLearnThreadsTerminate(true);
nwfilterDriverLock(driverState); nwfilterDriverLock(driverState);
virNWFilterWriteLockFilterUpdates();
virNWFilterCallbackDriversLock(); virNWFilterCallbackDriversLock();
virNWFilterLoadAllConfigs(&driverState->nwfilters, virNWFilterLoadAllConfigs(&driverState->nwfilters,
driverState->configDir); driverState->configDir);
virNWFilterCallbackDriversUnlock(); virNWFilterCallbackDriversUnlock();
virNWFilterUnlockFilterUpdates();
nwfilterDriverUnlock(driverState); nwfilterDriverUnlock(driverState);
virNWFilterInstFiltersOnAllVMs(); virNWFilterInstFiltersOnAllVMs();
...@@ -540,6 +542,7 @@ nwfilterDefineXML(virConnectPtr conn, ...@@ -540,6 +542,7 @@ nwfilterDefineXML(virConnectPtr conn,
virNWFilterPtr ret = NULL; virNWFilterPtr ret = NULL;
nwfilterDriverLock(driver); nwfilterDriverLock(driver);
virNWFilterWriteLockFilterUpdates();
virNWFilterCallbackDriversLock(); virNWFilterCallbackDriversLock();
if (!(def = virNWFilterDefParseString(xml))) if (!(def = virNWFilterDefParseString(xml)))
...@@ -566,6 +569,7 @@ cleanup: ...@@ -566,6 +569,7 @@ cleanup:
virNWFilterObjUnlock(nwfilter); virNWFilterObjUnlock(nwfilter);
virNWFilterCallbackDriversUnlock(); virNWFilterCallbackDriversUnlock();
virNWFilterUnlockFilterUpdates();
nwfilterDriverUnlock(driver); nwfilterDriverUnlock(driver);
return ret; return ret;
} }
...@@ -578,10 +582,9 @@ nwfilterUndefine(virNWFilterPtr obj) { ...@@ -578,10 +582,9 @@ nwfilterUndefine(virNWFilterPtr obj) {
int ret = -1; int ret = -1;
nwfilterDriverLock(driver); nwfilterDriverLock(driver);
virNWFilterWriteLockFilterUpdates();
virNWFilterCallbackDriversLock(); virNWFilterCallbackDriversLock();
virNWFilterLockFilterUpdates();
nwfilter = virNWFilterObjFindByUUID(&driver->nwfilters, obj->uuid); nwfilter = virNWFilterObjFindByUUID(&driver->nwfilters, obj->uuid);
if (!nwfilter) { if (!nwfilter) {
virReportError(VIR_ERR_NO_NWFILTER, virReportError(VIR_ERR_NO_NWFILTER,
...@@ -612,9 +615,8 @@ cleanup: ...@@ -612,9 +615,8 @@ cleanup:
if (nwfilter) if (nwfilter)
virNWFilterObjUnlock(nwfilter); virNWFilterObjUnlock(nwfilter);
virNWFilterUnlockFilterUpdates();
virNWFilterCallbackDriversUnlock(); virNWFilterCallbackDriversUnlock();
virNWFilterUnlockFilterUpdates();
nwfilterDriverUnlock(driver); nwfilterDriverUnlock(driver);
return ret; return ret;
} }
......
...@@ -947,8 +947,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, ...@@ -947,8 +947,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,
int ifindex; int ifindex;
int rc; int rc;
virNWFilterLockFilterUpdates();
/* after grabbing the filter update lock check for the interface; if /* after grabbing the filter update lock check for the interface; if
it's not there anymore its filters will be or are being removed it's not there anymore its filters will be or are being removed
(while holding the lock) and we don't want to build new ones */ (while holding the lock) and we don't want to build new ones */
...@@ -976,8 +974,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, ...@@ -976,8 +974,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,
foundNewFilter); foundNewFilter);
cleanup: cleanup:
virNWFilterUnlockFilterUpdates();
return rc; return rc;
} }
...@@ -996,7 +992,7 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, ...@@ -996,7 +992,7 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver,
int rc; int rc;
bool foundNewFilter = false; bool foundNewFilter = false;
virNWFilterLockFilterUpdates(); virNWFilterReadLockFilterUpdates();
rc = __virNWFilterInstantiateFilter(driver, rc = __virNWFilterInstantiateFilter(driver,
vmuuid, vmuuid,
......
...@@ -1542,6 +1542,8 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, ...@@ -1542,6 +1542,8 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn,
if (flags & VIR_DOMAIN_START_AUTODESTROY) if (flags & VIR_DOMAIN_START_AUTODESTROY)
start_flags |= VIR_QEMU_PROCESS_START_AUTODESTROY; start_flags |= VIR_QEMU_PROCESS_START_AUTODESTROY;
virNWFilterReadLockFilterUpdates();
if (!(caps = virQEMUDriverGetCapabilities(driver, false))) if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup; goto cleanup;
...@@ -1619,6 +1621,7 @@ cleanup: ...@@ -1619,6 +1621,7 @@ cleanup:
} }
virObjectUnref(caps); virObjectUnref(caps);
virObjectUnref(qemuCaps); virObjectUnref(qemuCaps);
virNWFilterUnlockFilterUpdates();
return dom; return dom;
} }
...@@ -5846,6 +5849,8 @@ qemuDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) ...@@ -5846,6 +5849,8 @@ qemuDomainCreateWithFlags(virDomainPtr dom, unsigned int flags)
VIR_DOMAIN_START_BYPASS_CACHE | VIR_DOMAIN_START_BYPASS_CACHE |
VIR_DOMAIN_START_FORCE_BOOT, -1); VIR_DOMAIN_START_FORCE_BOOT, -1);
virNWFilterReadLockFilterUpdates();
if (!(vm = qemuDomObjFromDomain(dom))) if (!(vm = qemuDomObjFromDomain(dom)))
return -1; return -1;
...@@ -5873,6 +5878,7 @@ endjob: ...@@ -5873,6 +5878,7 @@ endjob:
cleanup: cleanup:
if (vm) if (vm)
virObjectUnlock(vm); virObjectUnlock(vm);
virNWFilterUnlockFilterUpdates();
return ret; return ret;
} }
......
...@@ -1572,6 +1572,7 @@ static virDomainPtr umlDomainCreateXML(virConnectPtr conn, const char *xml, ...@@ -1572,6 +1572,7 @@ static virDomainPtr umlDomainCreateXML(virConnectPtr conn, const char *xml,
virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, NULL); virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, NULL);
virNWFilterReadLockFilterUpdates();
umlDriverLock(driver); umlDriverLock(driver);
if (!(def = virDomainDefParseString(xml, driver->caps, driver->xmlopt, if (!(def = virDomainDefParseString(xml, driver->caps, driver->xmlopt,
1 << VIR_DOMAIN_VIRT_UML, 1 << VIR_DOMAIN_VIRT_UML,
...@@ -1611,6 +1612,7 @@ cleanup: ...@@ -1611,6 +1612,7 @@ cleanup:
if (event) if (event)
umlDomainEventQueue(driver, event); umlDomainEventQueue(driver, event);
umlDriverUnlock(driver); umlDriverUnlock(driver);
virNWFilterUnlockFilterUpdates();
return dom; return dom;
} }
...@@ -1993,6 +1995,7 @@ static int umlDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) { ...@@ -1993,6 +1995,7 @@ static int umlDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) {
virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1); virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1);
virNWFilterReadLockFilterUpdates();
umlDriverLock(driver); umlDriverLock(driver);
vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
...@@ -2019,6 +2022,7 @@ cleanup: ...@@ -2019,6 +2022,7 @@ cleanup:
if (event) if (event)
umlDomainEventQueue(driver, event); umlDomainEventQueue(driver, event);
umlDriverUnlock(driver); umlDriverUnlock(driver);
virNWFilterUnlockFilterUpdates();
return ret; return ret;
} }
......
Markdown is supported
0% .
You are about to add 0 people to the discussion. Proceed with caution.
先完成此消息的编辑!
想要评论请 注册