Re: [libvirt] deadlock in remoteDispatchDomainUndefine vs daemonStreamHandleAbort

On Tue, Apr 2, 2019 at 3:30 PM Michal Privoznik <mprivozn(a)redhat.com&gt; wrote: I tried the suggestion that drops the fdst lock in virFDStreamCloseInt. But that leads to corruption as the devs structure (as it is cleaned up elsewhere in the meantime). It hangs again on the currently "Not acquired" lock (gdb) p priv->devs->lock $1 = {lock = pthread_mutex_t = {Type = Normal, Status = Not acquired, Robust = Yes, Shared = No, Protocol = Priority protect, Priority ceiling = 0}} But more interestingly hash seems clobbered already p priv->devs->hash $6 = (virHashTablePtr) 0x25 Code would usually access hash and 0x25 is not a valid address. The code after the lock would have failed in virHashRemoveEntry(priv->devs->hash, priv->path); This will access 0x25 and would segfault nextptr = table->table + virHashComputeKey(table, name); I analyzed the current deadlock again, what I see is this: I'll call the stack of undefine "U" and the stack of daemonStreamHandleAbort "A": 1A: already has FDST locked in virFDStreamCloseInt 2A: calls virChrdevFDStreamCloseCb which blocks on locks priv->devs->lock 3U: virFDStreamSetInternalCloseCb blocks on locking FDST 4U: virChrdevFree -> ... -> virFDStreamSetInternalCloseCb 5U: virChrdevFree does virMutexLock(&devs->lock); So it seems that: - 5U (holds) and 2A (blocks) collide on devs->lock - 1A (holds) and 3U (blocks) collide on virObjectLock(fdst) Seems like a classic entangled deadlock :-/ Next I wondered if we should put ordering on these locks with FDST>>devs->lock. Checking the callbacks that are actually registered I only found virChrdevFDStreamCloseCbFree which frees priv->path and priv, but does not access dev. So I tried: --- a/src/conf/virchrdev.c +++ b/src/conf/virchrdev.c @@ -310,8 +310,8 @@ void virChrdevFree(virChrdevsPtr devs) if (!devs || !devs->hash) return; - virMutexLock(&devs->lock); virHashForEach(devs->hash, virChrdevFreeClearCallbacks, NULL); + virMutexLock(&devs->lock); virHashFree(devs->hash); virMutexUnlock(&devs->lock); virMutexDestroy(&devs->lock); But that breaks virFDStreamSetInternalCloseCb which due to the stream being cleaned up in the background is called with fdst being zero. Again some other thread in the background cleaned up the structs and that breaks things. I quickly tried a band aid of: --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -1435,6 +1435,8 @@ int virFDStreamSetInternalCloseCb(virStr virFDStreamInternalCloseCbFreeOpaque fcb) { virFDStreamDataPtr fdst = st->privateData; + if (!fdst) + return -1; virObjectLock(fdst); But that didn't help (I still can get double frees later on). Overall I now wonder if we'd maybe want all entry paths for those cleanups to lock at a higher level to be mutually exclusive. virObjectLock(stream) maybe? The problem with that is the path from Undefine seems to work with a POV on devs, it locks devs first and only after virHashForEach knows the actual streams on which later the FDST is locked. OTOH the path starting at daemonStreamHandleAbort starts with streams, locks FDST and later tries to get devs->lock in virChrdevFDStreamCloseCb. Therefore I'm usure about locking on streams would be doable/helpful, I wanted to hear your further opinions/suggestions first. -- Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd