[libvirt] [PATCH 0/4] fix some libvirtd cleanup leaks and crashes
Crash situation of last 2 patches can easily simulated, just patch libvirt: diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 30a2830..f6b71d6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4515,6 +4515,8 @@ processMonitorEOFEvent(virQEMUDriverPtr driver, unsigned int stopFlags = 0; virObjectEventPtr event = NULL; + sleep(3); + if (qemuProcessBeginStopJob(driver, vm, QEMU_JOB_DESTROY, true) < 0) return; then when it is up and running and there is also a qemu domain running: kill -9 $QEMU_DOMAIN && kill libvirtd If first 2 patches are not applied yet then make sure there is no admin server clients present. Nikolay Shirokovskiy (4): daemon: break cyclic deps between admin clients and daemon daemon: call free callbacks synchronously in event loop impl daemon: keep daemon until all hypervisors drivers are cleaned up qemu: first wait all workers finish on state cleanup daemon/libvirtd.c | 7 ++++- src/qemu/qemu_driver.c | 2 +- src/rpc/virnetdaemon.c | 1 + src/rpc/virnetsocket.c | 70 ++++++++++++++++++++++--------------------------- src/util/vireventpoll.c | 24 +++++++---------- 5 files changed, 50 insertions(+), 54 deletions(-) -- 1.8.3.1
Libvirtd daemon cleanup is not correct in case there are admin server clients connected at the moment libvirtd exits. The reason is that there is cyclic dependency: daemon holds references to its network servers, networks servers hold references to its clients, admin server clients hold refs to daemon. These refs are kept until holder object dispose thus the above objects are never get disposed. Let's drop references to severs in daemon object at appropriate moment on cleanup procedure to break the cycle. The caller should keep references to servers by itself. Otherwise dropping refences can cause synchronous servers cleanup which can cause deadlock (server cleanup blocks until workers are finished and workers can call daemon functions which will try to get daemon lock). Other option is to make list cleanup via temporary list copy. --- daemon/libvirtd.c | 3 +++ src/rpc/virnetdaemon.c | 1 + 2 files changed, 4 insertions(+) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 95c1b1c..7fac7b2 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1628,6 +1628,9 @@ int main(int argc, char **argv) { virObjectUnref(qemuProgram); virObjectUnref(adminProgram); virNetDaemonClose(dmn); + /* we need to keep servers references up to here + so that above function will not cause servers cleanup + which can deadlock */ virObjectUnref(dmn); virObjectUnref(srv); virObjectUnref(srvAdm); diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c index dcc89fa..c6c6522 100644 --- a/src/rpc/virnetdaemon.c +++ b/src/rpc/virnetdaemon.c @@ -861,6 +861,7 @@ virNetDaemonClose(virNetDaemonPtr dmn) virObjectLock(dmn); virHashForEach(dmn->servers, daemonServerClose, NULL); + virHashRemoveAll(dmn->servers); virObjectUnlock(dmn); } -- 1.8.3.1
Default event loop impl delays deleting registered handles and timeouts so that deleting is safe from event handlers. But this means deletions after event loop is finished will never occur and particularly assosiated callback objects will not be freed. For this reason network clients that exist at the moment of libvirtd daemon exit are never get disposed and daemon object itself too if there are admin server clients. Let's call free callbacks immediately we don't need to delay this operation, only removing handles from the list. This breaks virnetsocket.c immediately as socket is locked in freeing callback and callback is called synchronously from virNetSocketRemoveIOCallback which holds the lock already. So fix it to pass intermediate callback object to the loop instead of socket itself. I've checked other users of virEventAddHandle, looks like they don't have such problems. --- src/rpc/virnetsocket.c | 70 ++++++++++++++++++++++--------------------------- src/util/vireventpoll.c | 24 +++++++---------- 2 files changed, 42 insertions(+), 52 deletions(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 405f5ba..732a993 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -69,6 +69,16 @@ VIR_LOG_INIT("rpc.netsocket"); +struct _virNetSocketCallbackObject { + virNetSocketPtr sock; + virNetSocketIOFunc func; + void *opaque; + virFreeCallback ff; +}; + +typedef struct _virNetSocketCallbackObject virNetSocketCallbackObject; +typedef virNetSocketCallbackObject *virNetSocketCallbackObjectPtr; + struct _virNetSocket { virObjectLockable parent; @@ -78,11 +88,6 @@ struct _virNetSocket { int errfd; bool client; - /* Event callback fields */ - virNetSocketIOFunc func; - void *opaque; - virFreeCallback ff; - virSocketAddr localAddr; virSocketAddr remoteAddr; char *localAddrStrSASL; @@ -1928,38 +1933,19 @@ static void virNetSocketEventHandle(int watch ATTRIBUTE_UNUSED, int events, void *opaque) { - virNetSocketPtr sock = opaque; - virNetSocketIOFunc func; - void *eopaque; - - virObjectLock(sock); - func = sock->func; - eopaque = sock->opaque; - virObjectUnlock(sock); - - if (func) - func(sock, events, eopaque); + virNetSocketCallbackObjectPtr o = opaque; + if (o->func) + o->func(o->sock, events, o->opaque); } static void virNetSocketEventFree(void *opaque) { - virNetSocketPtr sock = opaque; - virFreeCallback ff; - void *eopaque; - - virObjectLock(sock); - ff = sock->ff; - eopaque = sock->opaque; - sock->func = NULL; - sock->ff = NULL; - sock->opaque = NULL; - virObjectUnlock(sock); - - if (ff) - ff(eopaque); - - virObjectUnref(sock); + virNetSocketCallbackObjectPtr o = opaque; + if (o->ff) + o->ff(o->opaque); + virObjectUnref(o->sock); + VIR_FREE(o); } int virNetSocketAddIOCallback(virNetSocketPtr sock, @@ -1969,32 +1955,40 @@ int virNetSocketAddIOCallback(virNetSocketPtr sock, virFreeCallback ff) { int ret = -1; + virNetSocketCallbackObjectPtr cbobj = NULL; - virObjectRef(sock); virObjectLock(sock); if (sock->watch >= 0) { VIR_DEBUG("Watch already registered on socket %p", sock); goto cleanup; } + if (VIR_ALLOC(cbobj) < 0) + goto cleanup; + + cbobj->sock = virObjectRef(sock); + cbobj->func = func; + cbobj->opaque = opaque; + cbobj->ff = ff; + if ((sock->watch = virEventAddHandle(sock->fd, events, virNetSocketEventHandle, - sock, + cbobj, virNetSocketEventFree)) < 0) { VIR_DEBUG("Failed to register watch on socket %p", sock); goto cleanup; } - sock->func = func; - sock->opaque = opaque; - sock->ff = ff; ret = 0; cleanup: virObjectUnlock(sock); - if (ret != 0) + if (ret != 0) { + VIR_FREE(cbobj); virObjectUnref(sock); + } + return ret; } diff --git a/src/util/vireventpoll.c b/src/util/vireventpoll.c index 81ecab4..e152d23 100644 --- a/src/util/vireventpoll.c +++ b/src/util/vireventpoll.c @@ -198,6 +198,11 @@ int virEventPollRemoveHandle(int watch) if (eventLoop.handles[i].watch == watch) { EVENT_DEBUG("mark delete %zu %d", i, eventLoop.handles[i].fd); eventLoop.handles[i].deleted = 1; + if (eventLoop.handles[i].ff) { + virMutexUnlock(&eventLoop.lock); + eventLoop.handles[i].ff(eventLoop.handles[i].opaque); + virMutexLock(&eventLoop.lock); + } virEventPollInterruptLocked(); virMutexUnlock(&eventLoop.lock); return 0; @@ -315,6 +320,11 @@ int virEventPollRemoveTimeout(int timer) continue; if (eventLoop.timeouts[i].timer == timer) { + if (eventLoop.timeouts[i].ff) { + virMutexUnlock(&eventLoop.lock); + eventLoop.timeouts[i].ff(eventLoop.timeouts[i].opaque); + virMutexLock(&eventLoop.lock); + } eventLoop.timeouts[i].deleted = 1; virEventPollInterruptLocked(); virMutexUnlock(&eventLoop.lock); @@ -536,13 +546,6 @@ static void virEventPollCleanupTimeouts(void) PROBE(EVENT_POLL_PURGE_TIMEOUT, "timer=%d", eventLoop.timeouts[i].timer); - if (eventLoop.timeouts[i].ff) { - virFreeCallback ff = eventLoop.timeouts[i].ff; - void *opaque = eventLoop.timeouts[i].opaque; - virMutexUnlock(&eventLoop.lock); - ff(opaque); - virMutexLock(&eventLoop.lock); - } if ((i+1) < eventLoop.timeoutsCount) { memmove(eventLoop.timeouts+i, @@ -585,13 +588,6 @@ static void virEventPollCleanupHandles(void) PROBE(EVENT_POLL_PURGE_HANDLE, "watch=%d", eventLoop.handles[i].watch); - if (eventLoop.handles[i].ff) { - virFreeCallback ff = eventLoop.handles[i].ff; - void *opaque = eventLoop.handles[i].opaque; - virMutexUnlock(&eventLoop.lock); - ff(opaque); - virMutexLock(&eventLoop.lock); - } if ((i+1) < eventLoop.handlesCount) { memmove(eventLoop.handles+i, -- 1.8.3.1
On Thu, Sep 22, 2016 at 11:38:50AM +0300, Nikolay Shirokovskiy wrote:
Default event loop impl delays deleting registered handles and timeouts so that deleting is safe from event handlers. But this means deletions after event loop is finished will never occur and particularly assosiated callback objects will not be freed. For this reason network clients that exist at the moment of libvirtd daemon exit are never get disposed and daemon object itself too if there are admin server clients.
Let's call free callbacks immediately we don't need to delay this operation, only removing handles from the list.
This breaks virnetsocket.c immediately as socket is locked in freeing callback and callback is called synchronously from virNetSocketRemoveIOCallback which holds the lock already. So fix it to pass intermediate callback object to the loop instead of socket itself. I've checked other users of virEventAddHandle, looks like they don't have such problems.
That is impossible to guarantee. The event loop impl is exposed via the public API, so arbitrary external applications get to call it. So I don't think it is safe to make this change. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 22.09.2016 11:44, Daniel P. Berrange wrote:
On Thu, Sep 22, 2016 at 11:38:50AM +0300, Nikolay Shirokovskiy wrote:
Default event loop impl delays deleting registered handles and timeouts so that deleting is safe from event handlers. But this means deletions after event loop is finished will never occur and particularly assosiated callback objects will not be freed. For this reason network clients that exist at the moment of libvirtd daemon exit are never get disposed and daemon object itself too if there are admin server clients.
Let's call free callbacks immediately we don't need to delay this operation, only removing handles from the list.
This breaks virnetsocket.c immediately as socket is locked in freeing callback and callback is called synchronously from virNetSocketRemoveIOCallback which holds the lock already. So fix it to pass intermediate callback object to the loop instead of socket itself. I've checked other users of virEventAddHandle, looks like they don't have such problems.
That is impossible to guarantee. The event loop impl is exposed via the public API, so arbitrary external applications get to call it.
So I don't think it is safe to make this change.
Ok, the other I option I think of is to add call to indicate event loop is finished so that there is no need to defer cleanups. Nikolay
This fix SEGV with next backtrace (shortened a bit): 1 0x00007fd3a791b2e6 in virCondWait (c=<optimized out>, m=<optimized out>) at util/virthread.c:154 2 0x00007fd3a791bcb0 in virThreadPoolFree (pool=0x7fd38024ee00) at util/virthreadpool.c:266 3 0x00007fd38edaa00e in qemuStateCleanup () at qemu/qemu_driver.c:1116 4 0x00007fd3a79abfeb in virStateCleanup () at libvirt.c:808 5 0x00007fd3a85f2c9e in main (argc=<optimized out>, argv=<optimized out>) at libvirtd.c:1660 Thread 1 (Thread 0x7fd38722d700 (LWP 32256)): 0 0x00007fd3a7900910 in virClassIsDerivedFrom (klass=0xdfd36058d4853, parent=0x7fd3a8f394d0) at util/virobject.c:169 1 0x00007fd3a7900c4e in virObjectIsClass (anyobj=anyobj@entry=0x7fd3a8f2f850, klass=<optimized out>) at util/virobject.c:365 2 0x00007fd3a7900c74 in virObjectLock (anyobj=0x7fd3a8f2f850) at util/virobject.c:317 3 0x00007fd3a7a24d5d in virNetDaemonRemoveShutdownInhibition (dmn=0x7fd3a8f2f850) at rpc/virnetdaemon.c:547 4 0x00007fd38ed722cf in qemuProcessStop (driver=driver@entry=0x7fd380103810, vm=vm@entry=0x7fd38025b6d0, reason=reason@entry=VIR_DOMAIN_SHUTOFF_SHUTDOWN, asyncJob=asyncJob@entry=QEMU_ASYNC_JOB_NONE, flags=flags@entry=0) at qemu/qemu_process.c:5786 5 0x00007fd38edd9428 in processMonitorEOFEvent (vm=0x7fd38025b6d0, driver=0x7fd380103810) at qemu/qemu_driver.c:4588 6 qemuProcessEventHandler (data=<optimized out>, opaque=0x7fd380103810) at qemu/qemu_driver.c:4632 7 0x00007fd3a791bb55 in virThreadPoolWorker (opaque=opaque@entry=0x7fd3a8f1e4c0) at util/virthreadpool.c:145 This happens due to race on simultaneous finishing of libvirtd and qemu process. We need to keep daemon object until all hypervisor drivers are cleaned up. The other option is to take reference to daemon in every hypervisor driver but this will take more work and we really don't need to. Drivers cleanup procedure is synchronous thus let's just remove last reference to daemon after drivers cleanup. --- daemon/libvirtd.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 7fac7b2..97a7587 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1631,7 +1631,6 @@ int main(int argc, char **argv) { /* we need to keep servers references up to here so that above function will not cause servers cleanup which can deadlock */ - virObjectUnref(dmn); virObjectUnref(srv); virObjectUnref(srvAdm); virNetlinkShutdown(); @@ -1661,6 +1660,9 @@ int main(int argc, char **argv) { driversInitialized = false; virStateCleanup(); } + /* unref daemon only here as hypervisor drivers can + call shutdown inhibition functions on cleanup */ + virObjectUnref(dmn); return ret; } -- 1.8.3.1
This fix next crash (bt is shortened a bit): 0 0x00007ffff7282f2b in virClassIsDerivedFrom (klass=0xdeadbeef, parent=0x55555581d650) at util/virobject.c:169 1 0x00007ffff72835fd in virObjectIsClass (anyobj=0x7fffd024f580, klass=0x55555581d650) at util/virobject.c:365 2 0x00007ffff7283498 in virObjectLock (anyobj=0x7fffd024f580) at util/virobject.c:317 3 0x00007ffff722f0a3 in virCloseCallbacksUnset (closeCallbacks=0x7fffd024f580, vm=0x7fffd0194db0, cb=0x7fffdf1af765 <qemuProcessAutoDestroy>) at util/virclosecallbacks.c:164 4 0x00007fffdf1afa7b in qemuProcessAutoDestroyRemove (driver=0x7fffd00f3a60, vm=0x7fffd0194db0) at qemu/qemu_process.c:6365 5 0x00007fffdf1adff1 in qemuProcessStop (driver=0x7fffd00f3a60, vm=0x7fffd0194db0, reason=VIR_DOMAIN_SHUTOFF_CRASHED, asyncJob=QEMU_ASYNC_JOB_NONE, flags=0) at qemu/qemu_process.c:5877 6 0x00007fffdf1f711c in processMonitorEOFEvent (driver=0x7fffd00f3a60, vm=0x7fffd0194db0) at qemu/qemu_driver.c:4545 7 0x00007fffdf1f7313 in qemuProcessEventHandler (data=0x555555832710, opaque=0x7fffd00f3a60) at qemu/qemu_driver.c:4589 8 0x00007ffff72a84c4 in virThreadPoolWorker (opaque=0x555555805da0) at util/virthreadpool.c:167 Thread 1 (Thread 0x7ffff7fb1880 (LWP 494472)): 1 0x00007ffff72a7898 in virCondWait (c=0x7fffd01c21f8, m=0x7fffd01c21a0) at util/virthread.c:154 2 0x00007ffff72a8a22 in virThreadPoolFree (pool=0x7fffd01c2160) at util/virthreadpool.c:290 3 0x00007fffdf1edd44 in qemuStateCleanup () at qemu/qemu_driver.c:1102 4 0x00007ffff736570a in virStateCleanup () at libvirt.c:807 5 0x000055555556f991 in main (argc=1, argv=0x7fffffffe458) at libvirtd.c:1660 This happens on race conditions of simultaneous exiting libvirtd and qemu process. Let's just finish all workers that keep driver pointer before disposing driver object or any of its members. --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e29180d..30a2830 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1068,6 +1068,7 @@ qemuStateCleanup(void) if (!qemu_driver) return -1; + virThreadPoolFree(qemu_driver->workerPool); virNWFilterUnRegisterCallbackDriver(&qemuCallbackDriver); virObjectUnref(qemu_driver->config); virObjectUnref(qemu_driver->hostdevMgr); @@ -1099,7 +1100,6 @@ qemuStateCleanup(void) virLockManagerPluginUnref(qemu_driver->lockManager); virMutexDestroy(&qemu_driver->lock); - virThreadPoolFree(qemu_driver->workerPool); VIR_FREE(qemu_driver); return 0; -- 1.8.3.1
participants (2)
-
Daniel P. Berrange -
Nikolay Shirokovskiy