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

[Laine Stump]

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.