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);
>