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

[Maxim Nestratov]

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?

Maxim