From 1439dddbfb5d08046e0d2f29f322f067cafff550 Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Wed, 22 Jan 2014 17:28:29 +0000 Subject: [PATCH] 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: Daniel P. Berrange (cherry picked from commit 6e5c79a1b5a8b3a23e7df7ffe58fb272aa17fbfb) 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. src/qemu/qemu_driver.c - qemuDomainStartWithFlags (called qemuDomainCreateWithFlags upstream) gets the domain object using qemuDomObjFromDomain() upstream, but virDomainObjListFindByUUID() in 1.0.4. This creates a small conflict in context. --- src/conf/nwfilter_conf.c | 25 ++++++++++++------------- src/conf/nwfilter_conf.h | 3 ++- src/libvirt_private.syms | 3 ++- src/lxc/lxc_driver.c | 8 +++++++- src/nwfilter/nwfilter_driver.c | 10 ++++++---- src/nwfilter/nwfilter_gentech_driver.c | 6 +----- src/qemu/qemu_driver.c | 8 +++++++- src/uml/uml_driver.c | 4 ++++ 8 files changed, 41 insertions(+), 26 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index f5b6bcc326..e041843159 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2,7 +2,7 @@ * nwfilter_conf.c: network filter XML processing * (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) 2010-2011 IBM Corporation @@ -143,17 +143,22 @@ static const struct int_map chain_priorities[] = { /* * only one filter update allowed */ -static virMutex updateMutex; +static virRWLock updateLock; static bool initialized = false; void -virNWFilterLockFilterUpdates(void) { - virMutexLock(&updateMutex); +virNWFilterReadLockFilterUpdates(void) { + virRWLockRead(&updateLock); +} + +void +virNWFilterWriteLockFilterUpdates(void) { + virRWLockWrite(&updateLock); } void virNWFilterUnlockFilterUpdates(void) { - virMutexUnlock(&updateMutex); + virRWLockUnlock(&updateLock); } @@ -2997,14 +3002,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; } @@ -3012,7 +3015,6 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, /* trigger the update on VMs referencing the filter */ if (virNWFilterTriggerVMFilterRebuild()) { nwfilter->newDef = NULL; - virNWFilterUnlockFilterUpdates(); virNWFilterObjUnlock(nwfilter); return NULL; } @@ -3020,12 +3022,9 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, virNWFilterDefFree(nwfilter->def); nwfilter->def = def; nwfilter->newDef = NULL; - virNWFilterUnlockFilterUpdates(); return nwfilter; } - virNWFilterUnlockFilterUpdates(); - if (VIR_ALLOC(nwfilter) < 0) { virReportOOMError(); return NULL; @@ -3497,7 +3496,7 @@ int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB, initialized = true; - if (virMutexInitRecursive(&updateMutex) < 0) + if (virRWLockInit(&updateLock) < 0) return -1; return 0; @@ -3509,7 +3508,7 @@ void virNWFilterConfLayerShutdown(void) if (!initialized) return; - virMutexDestroy(&updateMutex); + virRWLockDestroy(&updateLock); initialized = false; virNWFilterDomainFWUpdateOpaque = NULL; diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 1b72ba8cba..359244ffce 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -717,7 +717,8 @@ virNWFilterDefPtr virNWFilterDefParseFile(const char *filename); void virNWFilterObjLock(virNWFilterObjPtr obj); void virNWFilterObjUnlock(virNWFilterObjPtr obj); -void virNWFilterLockFilterUpdates(void); +void virNWFilterWriteLockFilterUpdates(void); +void virNWFilterReadLockFilterUpdates(void); void virNWFilterUnlockFilterUpdates(void); int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB, void *opaque); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 46b6604010..d02a91a768 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -505,7 +505,6 @@ virNWFilterDefParseString; virNWFilterInstFiltersOnAllVMs; virNWFilterJumpTargetTypeToString; virNWFilterLoadAllConfigs; -virNWFilterLockFilterUpdates; virNWFilterObjAssignDef; virNWFilterObjDeleteDef; virNWFilterObjFindByName; @@ -517,6 +516,7 @@ virNWFilterObjSaveDef; virNWFilterObjUnlock; virNWFilterPrintStateMatchFlags; virNWFilterPrintTCPFlags; +virNWFilterReadLockFilterUpdates; virNWFilterRegisterCallbackDriver; virNWFilterRuleActionTypeToString; virNWFilterRuleDirectionTypeToString; @@ -524,6 +524,7 @@ virNWFilterRuleProtocolTypeToString; virNWFilterTestUnassignDef; virNWFilterUnlockFilterUpdates; virNWFilterUnRegisterCallbackDriver; +virNWFilterWriteLockFilterUpdates; # conf/nwfilter_ipaddrmap.h diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 0d1af09c77..b116c19736 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2010-2013 Red Hat, Inc. + * Copyright (C) 2010-2014 Red Hat, Inc. * Copyright IBM Corp. 2008 * * lxc_driver.c: linux container driver functions @@ -988,6 +988,8 @@ static int lxcDomainStartWithFlags(virDomainPtr dom, unsigned int flags) virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1); + virNWFilterReadLockFilterUpdates(); + lxcDriverLock(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (!vm) { @@ -1029,6 +1031,7 @@ cleanup: if (event) virDomainEventStateQueue(driver->domainEventState, event); lxcDriverUnlock(driver); + virNWFilterUnlockFilterUpdates(); return ret; } @@ -1067,6 +1070,8 @@ lxcDomainCreateAndStart(virConnectPtr conn, virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, NULL); + virNWFilterReadLockFilterUpdates(); + lxcDriverLock(driver); if (!(def = virDomainDefParseString(driver->caps, driver->xmlconf, xml, 1 << VIR_DOMAIN_VIRT_LXC, @@ -1116,6 +1121,7 @@ cleanup: if (event) virDomainEventStateQueue(driver->domainEventState, event); lxcDriverUnlock(driver); + virNWFilterUnlockFilterUpdates(); return dom; } diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 743947b36b..34c0ed22a6 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -282,12 +282,14 @@ nwfilterDriverReload(void) virNWFilterLearnThreadsTerminate(true); nwfilterDriverLock(driverState); + virNWFilterWriteLockFilterUpdates(); virNWFilterCallbackDriversLock(); virNWFilterLoadAllConfigs(&driverState->nwfilters, driverState->configDir); virNWFilterCallbackDriversUnlock(); + virNWFilterUnlockFilterUpdates(); nwfilterDriverUnlock(driverState); virNWFilterInstFiltersOnAllVMs(); @@ -529,6 +531,7 @@ nwfilterDefine(virConnectPtr conn, virNWFilterPtr ret = NULL; nwfilterDriverLock(driver); + virNWFilterWriteLockFilterUpdates(); virNWFilterCallbackDriversLock(); if (!(def = virNWFilterDefParseString(xml))) @@ -552,6 +555,7 @@ cleanup: virNWFilterObjUnlock(nwfilter); virNWFilterCallbackDriversUnlock(); + virNWFilterUnlockFilterUpdates(); nwfilterDriverUnlock(driver); return ret; } @@ -564,10 +568,9 @@ nwfilterUndefine(virNWFilterPtr obj) { int ret = -1; nwfilterDriverLock(driver); + virNWFilterWriteLockFilterUpdates(); virNWFilterCallbackDriversLock(); - virNWFilterLockFilterUpdates(); - nwfilter = virNWFilterObjFindByUUID(&driver->nwfilters, obj->uuid); if (!nwfilter) { virReportError(VIR_ERR_NO_NWFILTER, @@ -595,9 +598,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 5323e455db..0aa0f0caac 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -948,8 +948,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 (while holding the lock) and we don't want to build new ones */ @@ -977,8 +975,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, foundNewFilter); cleanup: - virNWFilterUnlockFilterUpdates(); - return rc; } @@ -997,7 +993,7 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, int rc; bool foundNewFilter = false; - virNWFilterLockFilterUpdates(); + virNWFilterReadLockFilterUpdates(); rc = __virNWFilterInstantiateFilter(driver, vmuuid, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 527a3b5654..55a1ca0e22 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1,7 +1,7 @@ /* * qemu_driver.c: core driver methods for managing qemu guests * - * Copyright (C) 2006-2013 Red Hat, Inc. + * Copyright (C) 2006-2014 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -1482,6 +1482,8 @@ static virDomainPtr qemuDomainCreate(virConnectPtr conn, const char *xml, if (flags & VIR_DOMAIN_START_AUTODESTROY) start_flags |= VIR_QEMU_PROCESS_START_AUTODESTROY; + virNWFilterReadLockFilterUpdates(); + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; @@ -1557,6 +1559,7 @@ cleanup: } virObjectUnref(caps); virObjectUnref(qemuCaps); + virNWFilterUnlockFilterUpdates(); return dom; } @@ -5491,6 +5494,8 @@ qemuDomainStartWithFlags(virDomainPtr dom, unsigned int flags) VIR_DOMAIN_START_BYPASS_CACHE | VIR_DOMAIN_START_FORCE_BOOT, -1); + virNWFilterReadLockFilterUpdates(); + vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (!vm) { @@ -5522,6 +5527,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 de8c5dd036..a831769bd4 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1503,6 +1503,7 @@ static virDomainPtr umlDomainCreate(virConnectPtr conn, const char *xml, virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, NULL); + virNWFilterReadLockFilterUpdates(); umlDriverLock(driver); if (!(def = virDomainDefParseString(driver->caps, driver->xmlconf, xml, 1 << VIR_DOMAIN_VIRT_UML, @@ -1540,6 +1541,7 @@ cleanup: if (event) umlDomainEventQueue(driver, event); umlDriverUnlock(driver); + virNWFilterUnlockFilterUpdates(); return dom; } @@ -1888,6 +1890,7 @@ static int umlDomainStartWithFlags(virDomainPtr dom, unsigned int flags) { virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1); + virNWFilterReadLockFilterUpdates(); umlDriverLock(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); @@ -1911,6 +1914,7 @@ cleanup: if (event) umlDomainEventQueue(driver, event); umlDriverUnlock(driver); + virNWFilterUnlockFilterUpdates(); return ret; }