diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 5908df723c4c5e44d70e899623ac5c4d9068f0db..2e89d07b64127d936e32267d2e6bb3a732209873 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -200,7 +200,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) @@ -251,6 +252,7 @@ error: err_techdrivers_shutdown: virNWFilterTechDriversShutdown(); +err_dhcpsnoop_shutdown: virNWFilterDHCPSnoopShutdown(); err_exit_learnshutdown: virNWFilterLearnShutdown(); @@ -327,10 +329,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 8c5cd57b613395556ebf085c830ed4e81c7e503f..5b1fac45b3fea4fb78330d0c2624ee273e62bfcd 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) +{ size_t 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) +{ size_t 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) +{ size_t i = 0; while (filter_tech_drivers[i]) { if (STREQ(filter_tech_drivers[i]->name, name)) { @@ -935,6 +958,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 */ @@ -962,6 +987,8 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, foundNewFilter); cleanup: + virMutexUnlock(&updateMutex); + return rc; } @@ -981,6 +1008,7 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, bool foundNewFilter = false; virNWFilterReadLockFilterUpdates(); + virMutexLock(&updateMutex); rc = __virNWFilterInstantiateFilter(driver, vmuuid, @@ -1006,6 +1034,7 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, } virNWFilterUnlockFilterUpdates(); + virMutexUnlock(&updateMutex); return rc; } @@ -1129,7 +1158,11 @@ _virNWFilterTeardownFilter(const char *ifname) int virNWFilterTeardownFilter(const virDomainNetDef *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 f4789e191b6d5a7edeb61f99ef5e2b9e1243b8a6..d72e0407253d45bdfb97889787f9190ab6ad8d71 100644 --- a/src/nwfilter/nwfilter_gentech_driver.h +++ b/src/nwfilter/nwfilter_gentech_driver.h @@ -31,7 +31,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 {