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

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(a)virtuozzo.com&gt; >>>> --- >>>> 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?