[libvirt] [PATCH v2 4/4] Push nwfilter update locking up to top level

[Stefan Berger]

On 01/27/2014 12:18 PM, Daniel P. Berrange wrote:
> 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

Insert: 3. nwfilter callback drivers lock

This is then used in this order also by nwfilterStateReload

>    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<berrange at redhat.com>
> ---
>   src/conf/nwfilter_conf.c               | 23 +++++++++++------------
>   src/conf/nwfilter_conf.h               |  3 ++-
>   src/libvirt_private.syms               |  3 ++-
>   src/lxc/lxc_driver.c                   |  6 ++++++
>   src/nwfilter/nwfilter_driver.c         | 11 +++++++----
>   src/nwfilter/nwfilter_gentech_driver.c |  4 +---
>   src/qemu/qemu_driver.c                 |  6 ++++++
>   src/uml/uml_driver.c                   |  4 ++++
>   8 files changed, 39 insertions(+), 21 deletions(-)
>
> diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
> index 112e8cb..80030c8 100644
> --- a/src/nwfilter/nwfilter_driver.c
> +++ b/src/nwfilter/nwfilter_driver.c
> @@ -285,12 +285,15 @@ nwfilterStateReload(void)
>       virNWFilterLearnThreadsTerminate(true);
>
>       nwfilterDriverLock(driverState);
> +    virNWFilterWriteLockFilterUpdates();
>       virNWFilterCallbackDriversLock();
>
> +

stray newline here

>       virNWFilterLoadAllConfigs(&driverState->nwfilters,
>                                 driverState->configDir);
>
>       virNWFilterCallbackDriversUnlock();
> +    virNWFilterUnlockFilterUpdates();
>       nwfilterDriverUnlock(driverState);
>
>       virNWFilterInstFiltersOnAllVMs();

Modulo the above stray line and then comment in the introductory text, I 
think this patch is good. I am just a bit concerned about 3/4.

    Stefan