[libvirt] [PATCH] nwfilter: fix lock order deadlock

[Cole Robinson]

On 04/09/2016 12:14 PM, Maxim Nestratov wrote:
> Below is backtraces of two deadlocked threads:
> 
> thread #1:
>  virDomainConfVMNWFilterTeardown
>    virNWFilterTeardownFilter
>        lock updateMutex <------------
>        _virNWFilterTeardownFilter
>             try to lock interface <----------
> 
> thread #2:
>  learnIPAddressThread
>     lock interface <-------
>     virNWFilterInstantiateFilterLate
>         try to lock updateMutex <----------
> 
> The problem is fixed by unlocking interface before calling
> virNWFilterInstantiateFilterLate to avoid updateMutex and interface ordering
> deadlocks. Otherwise we are going to instantiate the filter while holding
> interface lock, which will try to lock updateMutex, and if some other thread
> instantiating a filter in parallel is holding updateMutex and is trying to
> lock interface, both will deadlock.
> Also it is safe to unlock interface before virNWFilterInstantiateFilterLate
> because learnIPAddressThread stopped capturing packets and applied necessary
> rules on the interface, while instantiating a new filter doesn't require a
> locked interface.
> 
> Signed-off-by: Maxim Nestratov <mnestratov at virtuozzo.com>
> ---
>  src/nwfilter/nwfilter_learnipaddr.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c
> index 1adbadb..cfd92d9 100644
> --- a/src/nwfilter/nwfilter_learnipaddr.c
> +++ b/src/nwfilter/nwfilter_learnipaddr.c
> @@ -611,6 +611,16 @@ learnIPAddressThread(void *arg)
>          sa.data.inet4.sin_addr.s_addr = vmaddr;
>          char *inetaddr;
>  
> +        /* It is necessary to unlock interface here to avoid updateMutex and
> +         * interface ordering deadlocks. Otherwise we are going to
> +         * instantiate the filter, which will try to lock updateMutex, and
> +         * some other thread instantiating a filter in parallel is holding
> +         * updateMutex and is trying to lock interface, both will deadlock.
> +         * Also it is safe to unlock interface here because we stopped
> +         * capturing and applied necessary rules on the interface, while
> +         * instantiating a new filter doesn't require a locked interface.*/
> +        virNWFilterUnlockIface(req->ifname);
> +
>          if ((inetaddr = virSocketAddrFormat(&sa)) != NULL) {
>              if (virNWFilterIPAddrMapAddIPAddr(req->ifname, inetaddr) < 0) {
>                  VIR_ERROR(_("Failed to add IP address %s to IP address "
> @@ -636,11 +646,11 @@ learnIPAddressThread(void *arg)
>                                   req->ifname, req->ifindex);
>  
>          techdriver->applyDropAllRules(req->ifname);
> +        virNWFilterUnlockIface(req->ifname);
>      }
>  
>      VIR_DEBUG("pcap thread terminating for interface %s", req->ifname);
>  
> -    virNWFilterUnlockIface(req->ifname);
>  
>   err_no_lock:
>      virNWFilterDeregisterLearnReq(req->ifindex);
> 

This looks sensible to me... the virNWFilterInstantiateFilterLate call tree
certainly expects the iface to be unlocked at a certain point. This patch just
moves the unlock a bit earlier.

ACK, but maybe wait another day to see if anyone else wants to jump in, I'm
not really familiar with this code (then again I doubt anyone watching the
list is deeply familiar with it either)

- Cole