diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 2d5dcaf8311dae2840b557493440633fafde5e85..79a35466ae313e4d5ef239c1409468010bdc997d 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -201,7 +201,8 @@ nwfilterStateInitialize(bool privileged, if (virNWFilterDHCPSnoopInit() < 0) goto err_exit_learnshutdown; - virNWFilterTechDriversInit(privileged); + if (virNWFilterTechDriversInit(privileged) < 0) + goto err_dhcpsnoop_shutdown; if (virNWFilterConfLayerInit(virNWFilterDomainFWUpdateCB, driverState) < 0) @@ -252,6 +253,7 @@ error: err_techdrivers_shutdown: virNWFilterTechDriversShutdown(); +err_dhcpsnoop_shutdown: virNWFilterDHCPSnoopShutdown(); err_exit_learnshutdown: virNWFilterLearnShutdown(); @@ -328,10 +330,10 @@ nwfilterStateCleanup(void) { if (driverState->privileged) { virNWFilterConfLayerShutdown(); - virNWFilterTechDriversShutdown(); virNWFilterDHCPSnoopShutdown(); virNWFilterLearnShutdown(); virNWFilterIPAddrMapShutdown(); + virNWFilterTechDriversShutdown(); nwfilterDriverLock(driverState); diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index c9ce5142d3527d77598e24b5c56712e78e0bb782..5803acf10aaa330ff872d35f218f29a4d7c0cb90 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -55,30 +55,53 @@ static virNWFilterTechDriverPtr filter_tech_drivers[] = { NULL }; +/* Serializes instantiation of filters. This is necessary + * to avoid lock ordering deadlocks. eg __virNWFilterInstantiateFilter + * will hold a lock on a virNWFilterObjPtr. This in turn invokes + * virNWFilterInstantiate which invokes virNWFilterDetermineMissingVarsRec + * which invokes virNWFilterObjFindByName. This iterates over every single + * virNWFilterObjPtr in the list. So if 2 threads try to instantiate a + * filter in parallel, they'll both hold 1 lock at the top level in + * __virNWFilterInstantiateFilter which will cause the other thread + * to deadlock in virNWFilterObjFindByName. + * + * XXX better long term solution is to make virNWFilterObjList use a + * hash table as is done for virDomainObjList. You can then get + * lockless lookup of objects by name. + */ +static virMutex updateMutex; -void virNWFilterTechDriversInit(bool privileged) { +int virNWFilterTechDriversInit(bool privileged) +{ int i = 0; VIR_DEBUG("Initializing NWFilter technology drivers"); + if (virMutexInitRecursive(&updateMutex) < 0) + return -1; + while (filter_tech_drivers[i]) { if (!(filter_tech_drivers[i]->flags & TECHDRV_FLAG_INITIALIZED)) filter_tech_drivers[i]->init(privileged); i++; } + return 0; } -void virNWFilterTechDriversShutdown(void) { +void virNWFilterTechDriversShutdown(void) +{ int i = 0; while (filter_tech_drivers[i]) { if ((filter_tech_drivers[i]->flags & TECHDRV_FLAG_INITIALIZED)) filter_tech_drivers[i]->shutdown(); i++; } + virMutexDestroy(&updateMutex); } virNWFilterTechDriverPtr -virNWFilterTechDriverForName(const char *name) { +virNWFilterTechDriverForName(const char *name) +{ int i = 0; while (filter_tech_drivers[i]) { if (STREQ(filter_tech_drivers[i]->name, name)) { @@ -947,6 +970,8 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, int ifindex; int rc; + virMutexLock(&updateMutex); + /* after grabbing the filter update lock check for the interface; if 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 */ @@ -974,6 +999,8 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, foundNewFilter); cleanup: + virMutexUnlock(&updateMutex); + return rc; } @@ -993,6 +1020,7 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, bool foundNewFilter = false; virNWFilterReadLockFilterUpdates(); + virMutexLock(&updateMutex); rc = __virNWFilterInstantiateFilter(driver, vmuuid, @@ -1018,6 +1046,7 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, } virNWFilterUnlockFilterUpdates(); + virMutexUnlock(&updateMutex); return rc; } @@ -1141,7 +1170,11 @@ _virNWFilterTeardownFilter(const char *ifname) int virNWFilterTeardownFilter(const virDomainNetDefPtr net) { - return _virNWFilterTeardownFilter(net->ifname); + int ret; + virMutexLock(&updateMutex); + ret = _virNWFilterTeardownFilter(net->ifname); + virMutexUnlock(&updateMutex); + return ret; } diff --git a/src/nwfilter/nwfilter_gentech_driver.h b/src/nwfilter/nwfilter_gentech_driver.h index 8528e2a41eab8d912d8d1ca5e852787a4d797ba7..48ee1dfbfefbd1d82b09ff84d9e8e122cab06b83 100644 --- a/src/nwfilter/nwfilter_gentech_driver.h +++ b/src/nwfilter/nwfilter_gentech_driver.h @@ -30,7 +30,7 @@ virNWFilterTechDriverPtr virNWFilterTechDriverForName(const char *name); int virNWFilterRuleInstAddData(virNWFilterRuleInstPtr res, void *data); -void virNWFilterTechDriversInit(bool privileged); +int virNWFilterTechDriversInit(bool privileged); void virNWFilterTechDriversShutdown(void); enum instCase {