From: Denis V. Lunev <[email protected]>
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 <[email protected]>
---
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
