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

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