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

Stefan Berger stefanb at linux.vnet.ibm.com
Tue May 24 10:56:57 UTC 2016


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




More information about the libvir-list mailing list