From: Denis V. Lunev <den@openvz.org> daemonStreamEvent() holds priv->lock for its whole body, and every error path in it calls virNetServerClientClose(). That runs the client close hook synchronously in the same thread, and remoteClientCloseFunc() -> remoteClientFreePrivateCallbacks() takes priv->lock again. The lock is a plain non-recursive mutex, so the thread blocks on itself and never returns. The thread is the daemon main loop, so the host stops answering RPC entirely while its guests keep running. Only a restart of the daemon recovers. Every route from daemonStreamEvent() to the close goes through a virNetServerProgramSend* failure, and those fail once the client is marked wantClose. It therefore needs a client marked for close while one of its streams is still registered and still delivering events, which is why the deadlock has gone unnoticed for so long. It was hit in the field by a stream whose client had just died, but a console or a volume transfer reaches the same line. Move the body into a helper that reports whether the client has to go, and close it in the caller once the lock guard is out of scope. daemonRemoveClientStream() keeps running under the lock, and virNetServerClientClose() only ever needed the client object lock. Fixes: 6386dd897df5 ("remote: add mutex when freeing private callbacks") Signed-off-by: Denis V. Lunev <den@openvz.org> --- src/remote/remote_daemon_stream.c | 58 +++++++++++++++++++------------ 1 file changed, 36 insertions(+), 22 deletions(-) diff --git a/src/remote/remote_daemon_stream.c b/src/remote/remote_daemon_stream.c index 4faaf99a90..0841bb0c78 100644 --- a/src/remote/remote_daemon_stream.c +++ b/src/remote/remote_daemon_stream.c @@ -110,15 +110,16 @@ daemonStreamMessageFinished(virNetMessage *msg, /* - * Callback that gets invoked when a stream becomes writable/readable + * Returns true if the client has to be closed, which the caller does + * after dropping priv->lock. */ -static void -daemonStreamEvent(virStreamPtr st, int events, void *opaque) +static bool +daemonStreamEventLocked(virNetServerClient *client, + virStreamPtr st, + int events) { - virNetServerClient *client = opaque; - daemonClientStream *stream; daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); - VIR_LOCK_GUARD lock = virLockGuardLock(&priv->lock); + daemonClientStream *stream; stream = priv->streams; while (stream) { @@ -130,7 +131,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque) if (!stream) { VIR_WARN("event for client=%p stream st=%p, but missing stream state", client, st); virStreamEventRemoveCallback(st); - return; + return false; } VIR_DEBUG("st=%p events=%d EOF=%d closed=%d", st, events, stream->recvEOF, stream->closed); @@ -139,8 +140,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque) (events & VIR_STREAM_EVENT_WRITABLE)) { if (daemonStreamHandleWrite(client, stream) < 0) { daemonRemoveClientStream(client, stream); - virNetServerClientClose(client); - return; + return true; } } @@ -149,8 +149,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque) events = events & ~(VIR_STREAM_EVENT_READABLE); if (daemonStreamHandleRead(client, stream) < 0) { daemonRemoveClientStream(client, stream); - virNetServerClientClose(client); - return; + return true; } /* If we detected EOF during read processing, * then clear hangup/error conditions, since @@ -174,8 +173,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque) if (daemonStreamHandleFinish(client, stream, msg) < 0) { virNetMessageFree(msg); daemonRemoveClientStream(client, stream); - virNetServerClientClose(client); - return; + return true; } break; case VIR_NET_ERROR: @@ -184,8 +182,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque) if (daemonStreamHandleAbort(client, stream, msg) < 0) { virNetMessageFree(msg); daemonRemoveClientStream(client, stream); - virNetServerClientClose(client); - return; + return true; } break; } @@ -203,8 +200,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque) stream->recvEOF = true; if (!(msg = virNetMessageNew(false))) { daemonRemoveClientStream(client, stream); - virNetServerClientClose(client); - return; + return true; } msg->cb = daemonStreamMessageFinished; msg->opaque = stream; @@ -217,8 +213,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque) "", 0) < 0) { virNetMessageFree(msg); daemonRemoveClientStream(client, stream); - virNetServerClientClose(client); - return; + return true; } } @@ -258,9 +253,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque) stream->serial); } daemonRemoveClientStream(client, stream); - if (ret < 0) - virNetServerClientClose(client); - return; + return ret < 0; } if (stream->closed) { @@ -268,6 +261,27 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque) } else { daemonStreamUpdateEvents(stream); } + + return false; +} + + +/* + * Callback that gets invoked when a stream becomes writable/readable + */ +static void +daemonStreamEvent(virStreamPtr st, int events, void *opaque) +{ + virNetServerClient *client = opaque; + daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); + bool needClose = false; + + VIR_WITH_MUTEX_LOCK_GUARD(&priv->lock) { + needClose = daemonStreamEventLocked(client, st, events); + } + + if (needClose) + virNetServerClientClose(client); } -- 2.53.0