[libvirt PATCH 0/5] nwfilter: fix deadlock with binding create/delete
Historically the nwfilter driver didn't keep track of any instances of filters. When it needed to rebuild filters it called back to the virt drivers to ask them to re-create the filters for their VMs. This lead to some complex locking requirements where the virt driver needed to acquire locks on the nwfilter driver before creating/deleting filter instances. Somewhat recently we introduced the virNWFilterBinding concept which allowed the nwfilter driver to keep track of all filter instances aka bindings. Thus it could rebuild them without talking to the virt drivers. We never updated the locking model when doing this though, so we were still reliant on the virt drivers acquiring locks before creating or deleting virNWFilterBinding objects. When we started using the modular daemons though, the locks acquired by the virt drivers no longer protected the nwfilter driver as they were separate processes. Thus the race condition fixed back in commit 6e5c79a1b5a8b3a23e7df7ffe58fb272aa17fbfb Author: Daniel P. Berrangé <berrange@redhat.com> Date: Wed Jan 22 17:28:29 2014 +0000 Push nwfilter update locking up to top level has now re-appeared when running modular daemons. In fact it is possible to trigger it even in libvirtd if you ignore the virt drivers and directly call the virNWFilterBinding APIs, though that won't hit users in practice. The solution is surprisingly simple. Since the nwfilter driver keeps track of virNWFilterBinding objects, we can simply put locking calls around the Create/Delete methods for those objects, and remove the locking from the virt drivers. The only slight difference is that previous the locking serialized the entire VM startup/shutdown sequence, while the new locking only serializes individual NICs. This should not matter in practice, since there's no shared state between filters for NICs on the same VM. This is a minimal fix suitable for the freeze. There is scope for some significant improvements in locking which I'm still working on. In particular we currently have a terrible writer starvation problem with the RWLock that protects the filter create/delete/redefine operations that has existed forever. Repeatedly creating/deleting virNWFilterBinding objects can prevent a virNWFilterDefineXML call from ever running. Daniel P. Berrangé (5): nwfilter: stop using recursive mutex for IP learning nwfilter: hold filter update lock when creating/deleting bindings qemu,lxc: remove use to nwfilter update lock nwfilter: remove decl of virNWFilterCreateVarHashmap nwfilter: make some gentech driver methods static src/lxc/lxc_driver.c | 6 ------ src/nwfilter/nwfilter_driver.c | 5 +++++ src/nwfilter/nwfilter_gentech_driver.c | 4 ++-- src/nwfilter/nwfilter_gentech_driver.h | 8 -------- src/nwfilter/nwfilter_learnipaddr.c | 2 +- src/qemu/qemu_driver.c | 18 ------------------ src/qemu/qemu_migration.c | 3 --- src/qemu/qemu_process.c | 4 ---- 8 files changed, 8 insertions(+), 42 deletions(-) -- 2.35.1
The virNWFilterLockIface method is only called from one place, the learnIPAddressThread method. This is the entry point for the learning thread on the interface in question. As such there is never any recursive locking on this mutex from the same thread. This appears to have been the case since the code was first introduced. Thus a plain mutex is sufficient. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/nwfilter/nwfilter_learnipaddr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index fb552bd1e6..4840b0539f 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -151,7 +151,7 @@ virNWFilterLockIface(const char *ifname) if (!ifaceLock) { ifaceLock = g_new0(virNWFilterIfaceLock, 1); - if (virMutexInitRecursive(&ifaceLock->lock) < 0) { + if (virMutexInit(&ifaceLock->lock) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("mutex initialization failed")); g_free(ifaceLock); -- 2.35.1
On 2/25/22 10:30 AM, Daniel P. Berrangé wrote:
The virNWFilterLockIface method is only called from one place, the learnIPAddressThread method.
nwfilter_gentech_driver.c:virNWFilterDoInstantiate() and _virNWFilterTeardownFilter() also call virNWFilterLockIface. And there is this call chain from learnIPAddressThread() that ends up at one or the other of those: learnIPAddressThread() virNWFilterInstantiateFilterLate() virNWFilterInstantiateFilterUpdate() virNWFilterDoInstantiate() <---- _virNWFilterTeardownFilter() <---- But of course just prior to calling virNWFilterINstantiateFilterLate(), learnIPAddressThread() calls: virNWFilterUnlockIface(req->binding->portdevname); which matches the previous virNWFilterLockIface(), so there shouldn't be a problem for the learnIPAddressThread at least. I'm not sure about the threads of other calls to virNWFilterDoInstantiate/TeardownFilter though, haven't tried to decipher them yet.
This is the entry point for the learning thread on the interface in question. As such there is never any recursive locking on this mutex from the same thread. This appears to have been the case since the code was first introduced. Thus a plain mutex is sufficient.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/nwfilter/nwfilter_learnipaddr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index fb552bd1e6..4840b0539f 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -151,7 +151,7 @@ virNWFilterLockIface(const char *ifname) if (!ifaceLock) { ifaceLock = g_new0(virNWFilterIfaceLock, 1);
- if (virMutexInitRecursive(&ifaceLock->lock) < 0) { + if (virMutexInit(&ifaceLock->lock) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("mutex initialization failed")); g_free(ifaceLock);
On Mon, Feb 28, 2022 at 11:05:09AM -0500, Laine Stump wrote:
On 2/25/22 10:30 AM, Daniel P. Berrangé wrote:
The virNWFilterLockIface method is only called from one place, the learnIPAddressThread method.
nwfilter_gentech_driver.c:virNWFilterDoInstantiate() and _virNWFilterTeardownFilter() also call virNWFilterLockIface.
Sigh, yes. I didn't properly consider that stuff outside of learnip.c would be using its mutex. We should move the iface locking into the gentech driver to make it clear is is shared functionality. Consider this patch dropped. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
The nwfilter update lock is historically acquired by the virt drivers in order to achieve serialization between nwfilter define/undefine, and instantiation/teardown of filters. When running in the modular daemons, however, the mutex that the virt drivers are locking is in a completely different process from the mutex that the nwfilter driver is locking. Serialization is lost and thus call from the virt driver to virNWFilterBindingCreateXML can deadlock with a concurrent call to the virNWFilterDefineXML method. The solution is surprisingly easy, the update lock simply needs acquiring in the virNWFilterBindingCreateXML method and virNWFilterBindingUndefine method instead of in the virt drivers. The only semantic difference here is that when a virtual machine has multiple NICs, the instantiation and teardown of filters is no longer serialized for the whole VM, but rather for each NIC. This should not be a problem since the virt drivers already need to cope with tearing down a partially created VM where only some of the NICs are setup. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/nwfilter/nwfilter_driver.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 08f138dd79..3ce8fce7f9 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -760,11 +760,14 @@ nwfilterBindingCreateXML(virConnectPtr conn, if (!(ret = virGetNWFilterBinding(conn, def->portdevname, def->filter))) goto cleanup; + virNWFilterReadLockFilterUpdates(); if (virNWFilterInstantiateFilter(driver, def) < 0) { + virNWFilterUnlockFilterUpdates(); virNWFilterBindingObjListRemove(driver->bindings, obj); g_clear_pointer(&ret, virObjectUnref); goto cleanup; } + virNWFilterUnlockFilterUpdates(); virNWFilterBindingObjSave(obj, driver->bindingDir); cleanup: @@ -801,7 +804,9 @@ nwfilterBindingDelete(virNWFilterBindingPtr binding) if (virNWFilterBindingDeleteEnsureACL(binding->conn, def) < 0) goto cleanup; + virNWFilterReadLockFilterUpdates(); virNWFilterTeardownFilter(def); + virNWFilterUnlockFilterUpdates(); virNWFilterBindingObjDelete(obj, driver->bindingDir); virNWFilterBindingObjListRemove(driver->bindings, obj); -- 2.35.1
On 2/25/22 10:30 AM, Daniel P. Berrangé wrote:
The nwfilter update lock is historically acquired by the virt drivers in order to achieve serialization between nwfilter define/undefine, and instantiation/teardown of filters.
When running in the modular daemons, however, the mutex that the virt drivers are locking is in a completely different process from the mutex that the nwfilter driver is locking.
Serialization is lost and thus call from the virt driver to virNWFilterBindingCreateXML can deadlock with a concurrent call to the virNWFilterDefineXML method.
The solution is surprisingly easy, the update lock simply needs acquiring in the virNWFilterBindingCreateXML method and virNWFilterBindingUndefine method instead of in the virt drivers.
The only semantic difference here is that when a virtual machine has multiple NICs, the instantiation and teardown of filters is no longer serialized for the whole VM, but rather for each NIC. This should not be a problem since the virt drivers already need to cope with tearing down a partially created VM where only some of the NICs are setup.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com> (I considered this patch together with 3/5 - see my comment there of why I think the re-ordering of the locking is safe)
--- src/nwfilter/nwfilter_driver.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 08f138dd79..3ce8fce7f9 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -760,11 +760,14 @@ nwfilterBindingCreateXML(virConnectPtr conn, if (!(ret = virGetNWFilterBinding(conn, def->portdevname, def->filter))) goto cleanup;
+ virNWFilterReadLockFilterUpdates(); if (virNWFilterInstantiateFilter(driver, def) < 0) { + virNWFilterUnlockFilterUpdates(); virNWFilterBindingObjListRemove(driver->bindings, obj); g_clear_pointer(&ret, virObjectUnref); goto cleanup; } + virNWFilterUnlockFilterUpdates(); virNWFilterBindingObjSave(obj, driver->bindingDir);
cleanup: @@ -801,7 +804,9 @@ nwfilterBindingDelete(virNWFilterBindingPtr binding) if (virNWFilterBindingDeleteEnsureACL(binding->conn, def) < 0) goto cleanup;
+ virNWFilterReadLockFilterUpdates(); virNWFilterTeardownFilter(def); + virNWFilterUnlockFilterUpdates(); virNWFilterBindingObjDelete(obj, driver->bindingDir); virNWFilterBindingObjListRemove(driver->bindings, obj);
Now that the virNWFilterBinding APIs are using the nwfilter update lock directly, there is no need for the virt drivers to do it themselves. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/lxc/lxc_driver.c | 6 ------ src/qemu/qemu_driver.c | 18 ------------------ src/qemu/qemu_migration.c | 3 --- src/qemu/qemu_process.c | 4 ---- 4 files changed, 31 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 020ec257ae..4185a61267 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -971,8 +971,6 @@ static int lxcDomainCreateWithFiles(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1); - virNWFilterReadLockFilterUpdates(); - if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; @@ -1014,7 +1012,6 @@ static int lxcDomainCreateWithFiles(virDomainPtr dom, cleanup: virDomainObjEndAPI(&vm); virObjectEventStateQueue(driver->domainEventState, event); - virNWFilterUnlockFilterUpdates(); return ret; } @@ -1080,8 +1077,6 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn, if (flags & VIR_DOMAIN_START_VALIDATE) parse_flags |= VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA; - virNWFilterReadLockFilterUpdates(); - if (!(caps = virLXCDriverGetCapabilities(driver, false))) goto cleanup; @@ -1138,7 +1133,6 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn, cleanup: virDomainObjEndAPI(&vm); virObjectEventStateQueue(driver->domainEventState, event); - virNWFilterUnlockFilterUpdates(); return dom; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b74b0375a7..e4e1cd7bdf 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1606,8 +1606,6 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, if (flags & VIR_DOMAIN_START_RESET_NVRAM) start_flags |= VIR_QEMU_PROCESS_START_RESET_NVRAM; - virNWFilterReadLockFilterUpdates(); - if (!(def = virDomainDefParseString(xml, driver->xmlopt, NULL, parse_flags))) goto cleanup; @@ -1661,7 +1659,6 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, virDomainObjEndAPI(&vm); virObjectEventStateQueue(driver->domainEventState, event); virObjectEventStateQueue(driver->domainEventState, event2); - virNWFilterUnlockFilterUpdates(); return dom; } @@ -5776,8 +5773,6 @@ qemuDomainRestoreFlags(virConnectPtr conn, if (flags & VIR_DOMAIN_SAVE_RESET_NVRAM) reset_nvram = true; - virNWFilterReadLockFilterUpdates(); - fd = qemuSaveImageOpen(driver, NULL, path, &def, &data, (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, &wrapperFd, false, false); @@ -5849,7 +5844,6 @@ qemuDomainRestoreFlags(virConnectPtr conn, if (vm && ret < 0) qemuDomainRemoveInactiveJob(driver, vm); virDomainObjEndAPI(&vm); - virNWFilterUnlockFilterUpdates(); return ret; } @@ -6403,8 +6397,6 @@ qemuDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) VIR_DOMAIN_START_FORCE_BOOT | VIR_DOMAIN_START_RESET_NVRAM, -1); - virNWFilterReadLockFilterUpdates(); - if (!(vm = qemuDomainObjFromDomain(dom))) goto cleanup; @@ -6433,7 +6425,6 @@ qemuDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) cleanup: virDomainObjEndAPI(&vm); - virNWFilterUnlockFilterUpdates(); return ret; } @@ -7815,8 +7806,6 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom, virDomainObj *vm = NULL; int ret = -1; - virNWFilterReadLockFilterUpdates(); - if (!(vm = qemuDomainObjFromDomain(dom))) goto cleanup; @@ -7839,7 +7828,6 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom, cleanup: virDomainObjEndAPI(&vm); - virNWFilterUnlockFilterUpdates(); return ret; } @@ -7869,8 +7857,6 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1); - virNWFilterReadLockFilterUpdates(); - cfg = virQEMUDriverGetConfig(driver); if (!(vm = qemuDomainObjFromDomain(dom))) @@ -7947,7 +7933,6 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, if (dev != dev_copy) virDomainDeviceDefFree(dev_copy); virDomainObjEndAPI(&vm); - virNWFilterUnlockFilterUpdates(); return ret; } @@ -13654,8 +13639,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, virDomainObj *vm = NULL; int ret = -1; - virNWFilterReadLockFilterUpdates(); - if (!(vm = qemuDomObjFromSnapshot(snapshot))) goto cleanup; @@ -13666,7 +13649,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, cleanup: virDomainObjEndAPI(&vm); - virNWFilterUnlockFilterUpdates(); return ret; } diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 5aecdddff0..43ee094486 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2782,8 +2782,6 @@ qemuMigrationDstPrepareAny(virQEMUDriver *driver, int rv; g_autofree char *tlsAlias = NULL; - virNWFilterReadLockFilterUpdates(); - if (flags & VIR_MIGRATE_OFFLINE) { if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC)) { @@ -3103,7 +3101,6 @@ qemuMigrationDstPrepareAny(virQEMUDriver *driver, virDomainObjEndAPI(&vm); virObjectEventStateQueue(driver->domainEventState, event); qemuMigrationCookieFree(mig); - virNWFilterUnlockFilterUpdates(); virErrorRestore(&origErr); return ret; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 5af925e521..b19a6218d0 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -9004,7 +9004,6 @@ qemuProcessReconnect(void *opaque) qemuDomainRemoveInactiveJob(driver, obj); } virDomainObjEndAPI(&obj); - virNWFilterUnlockFilterUpdates(); virIdentitySetCurrent(NULL); return; @@ -9056,8 +9055,6 @@ qemuProcessReconnectHelper(virDomainObj *obj, data->obj = obj; data->identity = virIdentityGetCurrent(); - virNWFilterReadLockFilterUpdates(); - /* this lock and reference will be eventually transferred to the thread * that handles the reconnect */ virObjectLock(obj); @@ -9080,7 +9077,6 @@ qemuProcessReconnectHelper(virDomainObj *obj, qemuDomainRemoveInactiveJobLocked(src->driver, obj); virDomainObjEndAPI(&obj); - virNWFilterUnlockFilterUpdates(); g_clear_object(&data->identity); VIR_FREE(data); return -1; -- 2.35.1
On 2/25/22 10:30 AM, Daniel P. Berrangé wrote:
Now that the virNWFilterBinding APIs are using the nwfilter update lock directly, there is no need for the virt drivers to do it themselves.
I'm always nervous when the ordering of locks is changed, and that's what is happening with the combination of this patch and the previous patch. Before it was always the NWFilterLock that was acquired first, and then the domain object is retrieved (which involves getting the domain-list lock while getting a ref to the domain object). But since holding of the domain-list lock is localized to that short period (and certainly never across a call to the NWFilter binding API) I'm fairly certain this (along with the previous patch) is safe from creating any new deadlocks.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
--- src/lxc/lxc_driver.c | 6 ------ src/qemu/qemu_driver.c | 18 ------------------ src/qemu/qemu_migration.c | 3 --- src/qemu/qemu_process.c | 4 ---- 4 files changed, 31 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 020ec257ae..4185a61267 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -971,8 +971,6 @@ static int lxcDomainCreateWithFiles(virDomainPtr dom,
virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1);
- virNWFilterReadLockFilterUpdates(); - if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup;
@@ -1014,7 +1012,6 @@ static int lxcDomainCreateWithFiles(virDomainPtr dom, cleanup: virDomainObjEndAPI(&vm); virObjectEventStateQueue(driver->domainEventState, event); - virNWFilterUnlockFilterUpdates(); return ret; }
@@ -1080,8 +1077,6 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn, if (flags & VIR_DOMAIN_START_VALIDATE) parse_flags |= VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA;
- virNWFilterReadLockFilterUpdates(); - if (!(caps = virLXCDriverGetCapabilities(driver, false))) goto cleanup;
@@ -1138,7 +1133,6 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn, cleanup: virDomainObjEndAPI(&vm); virObjectEventStateQueue(driver->domainEventState, event); - virNWFilterUnlockFilterUpdates(); return dom; }
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b74b0375a7..e4e1cd7bdf 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1606,8 +1606,6 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, if (flags & VIR_DOMAIN_START_RESET_NVRAM) start_flags |= VIR_QEMU_PROCESS_START_RESET_NVRAM;
- virNWFilterReadLockFilterUpdates(); - if (!(def = virDomainDefParseString(xml, driver->xmlopt, NULL, parse_flags))) goto cleanup; @@ -1661,7 +1659,6 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, virDomainObjEndAPI(&vm); virObjectEventStateQueue(driver->domainEventState, event); virObjectEventStateQueue(driver->domainEventState, event2); - virNWFilterUnlockFilterUpdates(); return dom; }
@@ -5776,8 +5773,6 @@ qemuDomainRestoreFlags(virConnectPtr conn, if (flags & VIR_DOMAIN_SAVE_RESET_NVRAM) reset_nvram = true;
- virNWFilterReadLockFilterUpdates(); - fd = qemuSaveImageOpen(driver, NULL, path, &def, &data, (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, &wrapperFd, false, false); @@ -5849,7 +5844,6 @@ qemuDomainRestoreFlags(virConnectPtr conn, if (vm && ret < 0) qemuDomainRemoveInactiveJob(driver, vm); virDomainObjEndAPI(&vm); - virNWFilterUnlockFilterUpdates(); return ret; }
@@ -6403,8 +6397,6 @@ qemuDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) VIR_DOMAIN_START_FORCE_BOOT | VIR_DOMAIN_START_RESET_NVRAM, -1);
- virNWFilterReadLockFilterUpdates(); - if (!(vm = qemuDomainObjFromDomain(dom))) goto cleanup;
@@ -6433,7 +6425,6 @@ qemuDomainCreateWithFlags(virDomainPtr dom, unsigned int flags)
cleanup: virDomainObjEndAPI(&vm); - virNWFilterUnlockFilterUpdates(); return ret; }
@@ -7815,8 +7806,6 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom, virDomainObj *vm = NULL; int ret = -1;
- virNWFilterReadLockFilterUpdates(); - if (!(vm = qemuDomainObjFromDomain(dom))) goto cleanup;
@@ -7839,7 +7828,6 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom,
cleanup: virDomainObjEndAPI(&vm); - virNWFilterUnlockFilterUpdates(); return ret; }
@@ -7869,8 +7857,6 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1);
- virNWFilterReadLockFilterUpdates(); - cfg = virQEMUDriverGetConfig(driver);
if (!(vm = qemuDomainObjFromDomain(dom))) @@ -7947,7 +7933,6 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, if (dev != dev_copy) virDomainDeviceDefFree(dev_copy); virDomainObjEndAPI(&vm); - virNWFilterUnlockFilterUpdates(); return ret; }
@@ -13654,8 +13639,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, virDomainObj *vm = NULL; int ret = -1;
- virNWFilterReadLockFilterUpdates(); - if (!(vm = qemuDomObjFromSnapshot(snapshot))) goto cleanup;
@@ -13666,7 +13649,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
cleanup: virDomainObjEndAPI(&vm); - virNWFilterUnlockFilterUpdates(); return ret; }
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 5aecdddff0..43ee094486 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2782,8 +2782,6 @@ qemuMigrationDstPrepareAny(virQEMUDriver *driver, int rv; g_autofree char *tlsAlias = NULL;
- virNWFilterReadLockFilterUpdates(); - if (flags & VIR_MIGRATE_OFFLINE) { if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC)) { @@ -3103,7 +3101,6 @@ qemuMigrationDstPrepareAny(virQEMUDriver *driver, virDomainObjEndAPI(&vm); virObjectEventStateQueue(driver->domainEventState, event); qemuMigrationCookieFree(mig); - virNWFilterUnlockFilterUpdates(); virErrorRestore(&origErr); return ret;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 5af925e521..b19a6218d0 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -9004,7 +9004,6 @@ qemuProcessReconnect(void *opaque) qemuDomainRemoveInactiveJob(driver, obj); } virDomainObjEndAPI(&obj); - virNWFilterUnlockFilterUpdates(); virIdentitySetCurrent(NULL); return;
@@ -9056,8 +9055,6 @@ qemuProcessReconnectHelper(virDomainObj *obj, data->obj = obj; data->identity = virIdentityGetCurrent();
- virNWFilterReadLockFilterUpdates(); - /* this lock and reference will be eventually transferred to the thread * that handles the reconnect */ virObjectLock(obj); @@ -9080,7 +9077,6 @@ qemuProcessReconnectHelper(virDomainObj *obj, qemuDomainRemoveInactiveJobLocked(src->driver, obj);
virDomainObjEndAPI(&obj); - virNWFilterUnlockFilterUpdates(); g_clear_object(&data->identity); VIR_FREE(data); return -1;
On Mon, Feb 28, 2022 at 11:24:07AM -0500, Laine Stump wrote:
On 2/25/22 10:30 AM, Daniel P. Berrangé wrote:
Now that the virNWFilterBinding APIs are using the nwfilter update lock directly, there is no need for the virt drivers to do it themselves.
I'm always nervous when the ordering of locks is changed, and that's what is happening with the combination of this patch and the previous patch. Before it was always the NWFilterLock that was acquired first, and then the domain object is retrieved (which involves getting the domain-list lock while getting a ref to the domain object).
But since holding of the domain-list lock is localized to that short period (and certainly never across a call to the NWFilter binding API) I'm fairly certain this (along with the previous patch) is safe from creating any new deadlocks.
Note the reason for that ordering was that in the past the nwfilter driver would have ability to call back into the virt driver to ask the virt driver to reload all nwfilters for VMs. With this callback we would acquire the nwfilter update lock, and then iterate over each VM in turn. The nwfilter driver no longer has this ability. It is completely self contained when re-loadeding nwfilters. The only calls are from the virt driver into the nwfilter driver. Thus there can never be any scenario where a nwfilter driver lock is held when the virt driver tries to acquire a domain lock. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
This method doesn't exist since commit d1a7c08eb145d8b9d9c4a268f4ffff3b1590049a Author: Daniel P. Berrangé <berrange@redhat.com> Date: Thu Apr 26 12:26:51 2018 +0100 nwfilter: convert the gentech driver code to use virNWFilterBindingDefPtr Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/nwfilter/nwfilter_gentech_driver.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/nwfilter/nwfilter_gentech_driver.h b/src/nwfilter/nwfilter_gentech_driver.h index 56d1d7a80a..74f8a4496b 100644 --- a/src/nwfilter/nwfilter_gentech_driver.h +++ b/src/nwfilter/nwfilter_gentech_driver.h @@ -49,8 +49,5 @@ int virNWFilterInstantiateFilterLate(virNWFilterDriverState *driver, int virNWFilterTeardownFilter(virNWFilterBindingDef *binding); -GHashTable *virNWFilterCreateVarHashmap(const char *macaddr, - const virNWFilterVarValue *value); - int virNWFilterBuildAll(virNWFilterDriverState *driver, bool newFilters); -- 2.35.1
On 2/25/22 10:30 AM, Daniel P. Berrangé wrote:
This method doesn't exist since
commit d1a7c08eb145d8b9d9c4a268f4ffff3b1590049a Author: Daniel P. Berrangé <berrange@redhat.com> Date: Thu Apr 26 12:26:51 2018 +0100
nwfilter: convert the gentech driver code to use virNWFilterBindingDefPtr
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The proof is in the successful compile without it :-) Reviewed-by: Laine Stump <laine@redhat.com>
The virNWFilterTechDriverForName & virNWFilterUpdateInstantiateFilter methods are only used within the same source file, so don't need to be exported. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/nwfilter/nwfilter_gentech_driver.c | 4 ++-- src/nwfilter/nwfilter_gentech_driver.h | 5 ----- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 20ecd299bf..7bbf1e12fb 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -99,7 +99,7 @@ void virNWFilterTechDriversShutdown(void) } -virNWFilterTechDriver * +static virNWFilterTechDriver * virNWFilterTechDriverForName(const char *name) { size_t i = 0; @@ -791,7 +791,7 @@ virNWFilterInstantiateFilter(virNWFilterDriverState *driver, } -int +static int virNWFilterUpdateInstantiateFilter(virNWFilterDriverState *driver, virNWFilterBindingDef *binding, bool *skipIface) diff --git a/src/nwfilter/nwfilter_gentech_driver.h b/src/nwfilter/nwfilter_gentech_driver.h index 74f8a4496b..969ab76966 100644 --- a/src/nwfilter/nwfilter_gentech_driver.h +++ b/src/nwfilter/nwfilter_gentech_driver.h @@ -26,8 +26,6 @@ #include "virnwfilterbindingdef.h" #include "nwfilter_tech_driver.h" -virNWFilterTechDriver *virNWFilterTechDriverForName(const char *name); - int virNWFilterTechDriversInit(bool privileged); void virNWFilterTechDriversShutdown(void); @@ -39,9 +37,6 @@ enum instCase { int virNWFilterInstantiateFilter(virNWFilterDriverState *driver, virNWFilterBindingDef *binding); -int virNWFilterUpdateInstantiateFilter(virNWFilterDriverState *driver, - virNWFilterBindingDef *binding, - bool *skipIface); int virNWFilterInstantiateFilterLate(virNWFilterDriverState *driver, virNWFilterBindingDef *binding, -- 2.35.1
On 2/25/22 10:30 AM, Daniel P. Berrangé wrote:
The virNWFilterTechDriverForName & virNWFilterUpdateInstantiateFilter methods are only used within the same source file, so don't need to be exported.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com> (again, I didn't even look to the source to verify; the fact that it still compiles proves that you're right. Hooray for machines doing (part of) our work for us!)
participants (2)
-
Daniel P. Berrangé -
Laine Stump