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

[Maxim Nestratov]

24.05.2016 13:56, Stefan Berger пишет:

> On 05/24/2016 03:22 AM, Maxim Nestratov wrote:
>> 20.04.2016 22:04, 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
>>>
>>>
>>
>> Shall I push it then?
>
> Yes.
>
>    Stefan
>

Pushed now. Thanks.

Maxim