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

[Stefan Berger]

On 04/20/2016 12:06 PM, Laine Stump wrote:
> On 04/19/2016 05:45 PM, Cole Robinson wrote:
>> 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)
>
> Stefan Berger is, but I don't think he watches the list closely these 
> days. I just found the original message (somehow it had been tossed 
> into my spam folder) and Replied-All to it with Stefan Cc'ed.

ACK. I agree to the change. It seems to be possible to release the lock 
earlier.

    Stefan