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

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.