[libvirt PATCH 2/5] nwfilter: hold filter update lock when creating/deleting bindings

[Laine Stump]

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 at redhat.com>


Reviewed-by: Laine Stump <laine at 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);
>