9:30 a.m.
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(a)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
9:30 a.m.
New subject: [libvirt PATCH 1/5] nwfilter: stop using recursive mutex for IP learning
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(a)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
10:05 a.m.
New subject: [libvirt PATCH 1/5] nwfilter: stop using recursive mutex for IP learning
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.
10:15 a.m.
New subject: [libvirt PATCH 1/5] nwfilter: stop using recursive mutex for IP learning
On Mon, Feb 28, 2022 at 11:05:09AM -0500, Laine Stump wrote:
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 :|
9:30 a.m.
New subject: [libvirt PATCH 2/5] nwfilter: hold filter update lock when creating/deleting bindings
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(a)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
10:22 a.m.
New subject: [libvirt PATCH 2/5] nwfilter: hold filter update lock when creating/deleting bindings
On 2/25/22 10:30 AM, Daniel P. Berrangé wrote:
Reviewed-by: Laine Stump <laine(a)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)
9:30 a.m.
New subject: [libvirt PATCH 3/5] qemu,lxc: remove use to nwfilter update lock
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(a)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
10:24 a.m.
New subject: [libvirt PATCH 3/5] qemu,lxc: remove use to nwfilter update lock
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(a)redhat.com>
Reviewed-by: Laine Stump <laine(a)redhat.com>
10:28 a.m.
New subject: [libvirt PATCH 3/5] qemu,lxc: remove use to nwfilter update lock
On Mon, Feb 28, 2022 at 11:24:07AM -0500, Laine Stump wrote:
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 :|
9:30 a.m.
New subject: [libvirt PATCH 4/5] nwfilter: remove decl of virNWFilterCreateVarHashmap
This method doesn't exist since
commit d1a7c08eb145d8b9d9c4a268f4ffff3b1590049a
Author: Daniel P. Berrangé <berrange(a)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(a)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
9:30 a.m.
New subject: [libvirt PATCH 5/5] nwfilter: make some gentech driver methods static
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(a)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
10:27 a.m.
New subject: [libvirt PATCH 5/5] nwfilter: make some gentech driver methods static
On 2/25/22 10:30 AM, Daniel P. Berrangé wrote:
Reviewed-by: Laine Stump <laine(a)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!)
1133
days inactive
1136
days old
12 comments
2 participants
participants (2)
-
Daniel P. Berrangé -
Laine Stump