[PATCH 0/2] remote: fix a daemon self-deadlock on the stream error path
daemonStreamEvent() holds priv->lock across its whole body and closes the client from every error path. The close hook runs in the same thread and takes priv->lock again, so the daemon main loop deadlocks against itself and the host stops answering RPC entirely while its guests keep running. Only a restart of the daemon recovers. We hit this in the field on 9.5.0, and current master is affected the same way. Getting there needs a client already marked wantClose while one of its streams still delivers events, which is presumably why it has gone unnoticed since 9.2.0. Patch 1 closes the client after the lock guard has gone out of scope. Patch 2 adds a test for it, which needs remote_daemon_stream.c compiled into a test binary; that is the only build change and it pulls in the generated protocol headers via remote_daemon.h. The test deadlocks without patch 1 and passes with it. Signed-off-by: Denis V. Lunev <den@openvz.org> Denis V. Lunev (2): remote: fix daemon deadlock when a stream event closes its client tests: add daemonstreamtest covering the stream close deadlock src/remote/meson.build | 7 +- src/remote/remote_daemon_stream.c | 58 ++++---- tests/daemonstreamtest.c | 212 ++++++++++++++++++++++++++++++ tests/meson.build | 9 ++ 4 files changed, 262 insertions(+), 24 deletions(-) create mode 100644 tests/daemonstreamtest.c base-commit: 91d4618734bd6d02174b909aadb7d84a99560ebe -- 2.53.0
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
From: Denis V. Lunev <den@openvz.org> daemonStreamEvent() must not close the client while holding priv->lock: the close hook takes that lock too and runs in the same thread. A regression there hangs the daemon rather than failing anything. It tests deterministically. virNetServerClientImmediateClose() sets wantClose without closing anything, which is the state every route to the close needs, and an fd stream over a pipe fires as soon as the write end goes away. So the test gives a client a close hook that takes priv->lock, as remoteClientFreePrivateCallbacks() does, registers a stream, marks the client for close and hangs up the pipe. A close under the lock blocks the event loop and the alarm reports it. Building remote_daemon_stream.c into a test pulls in the generated protocol headers through remote_daemon.h, hence the extra sources and the new variable that keeps the daemon listing the file once. Signed-off-by: Denis V. Lunev <den@openvz.org> --- src/remote/meson.build | 7 +- tests/daemonstreamtest.c | 212 +++++++++++++++++++++++++++++++++++++++ tests/meson.build | 9 ++ 3 files changed, 226 insertions(+), 2 deletions(-) create mode 100644 tests/daemonstreamtest.c diff --git a/src/remote/meson.build b/src/remote/meson.build index 3525bc34b2..a1d858779b 100644 --- a/src/remote/meson.build +++ b/src/remote/meson.build @@ -52,12 +52,15 @@ foreach name : [ 'remote', 'qemu', 'lxc' ] rpc_probe_files += files(protocol_x) endforeach +remote_daemon_stream_sources = files( + 'remote_daemon_stream.c', +) + remote_daemon_sources = files( 'remote_daemon.c', 'remote_daemon_config.c', 'remote_daemon_dispatch.c', - 'remote_daemon_stream.c', -) +) + remote_daemon_stream_sources remote_daemon_generated = [] diff --git a/tests/daemonstreamtest.c b/tests/daemonstreamtest.c new file mode 100644 index 0000000000..586bdcc1e9 --- /dev/null +++ b/tests/daemonstreamtest.c @@ -0,0 +1,212 @@ +/* + * daemonstreamtest.c: test the daemon side of client streams + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#include <signal.h> +#include <sys/socket.h> +#include <unistd.h> + +#include "testutils.h" +#include "virerror.h" +#include "virevent.h" +#include "virfdstream.h" +#include "virfile.h" +#include "virthread.h" +#include "virutil.h" +#include "remote/remote_daemon_stream.h" +#include "rpc/virnetserverclient.h" +#include "rpc/virnetserverprogram.h" + +#define VIR_FROM_THIS VIR_FROM_RPC + +#ifndef WIN32 + +#define TEST_PROGRAM 0x11111111 +#define TEST_TIMEOUT 5 + +static bool closeHookDone; + +static void * +testClientPrivNew(virNetServerClient *client G_GNUC_UNUSED, + void *opaque G_GNUC_UNUSED) +{ + daemonClientPrivate *priv = g_new0(daemonClientPrivate, 1); + + if (virMutexInit(&priv->lock) < 0) { + g_free(priv); + return NULL; + } + + return priv; +} + + +static void +testClientPrivFree(void *opaque) +{ + daemonClientPrivate *priv = opaque; + + virMutexDestroy(&priv->lock); + g_free(priv); +} + + +/* Same locking as remoteClientFreePrivateCallbacks(), which is what the + * daemon installs here. */ +static void +testClientClose(virNetServerClient *client) +{ + daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); + VIR_LOCK_GUARD lock = virLockGuardLock(&priv->lock); + + closeHookDone = true; +} + + +static void +testAlarm(int sig G_GNUC_UNUSED) +{ + static const char msg[] = "daemonStreamEvent() deadlocked closing the client\n"; + + ignore_value(safewrite(STDERR_FILENO, msg, sizeof(msg) - 1)); + _exit(EXIT_FAILURE); +} + + +/* A stream event delivered for a client that is already marked for close + * must not deadlock the thread running the event loop. The error reply + * cannot be queued to such a client, so the event handler closes it, and + * the close hook takes the same lock the handler holds. */ +static int +testStreamEventClose(const void *opaque G_GNUC_UNUSED) +{ + struct virNetMessageHeader header = { .proc = 1, .serial = 1 }; + virConnectPtr conn = NULL; + virNetServerClient *client = NULL; + virNetServerProgram *prog = NULL; + daemonClientStream *stream = NULL; + virNetSocket *sock = NULL; + virStreamPtr st = NULL; + int pipeFD[2] = { -1, -1 }; + int sv[2] = { -1, -1 }; + int ret = -1; + size_t i; + + closeHookDone = false; + + if (!(conn = virConnectOpen("test:///default"))) + return -1; + + if (socketpair(PF_UNIX, SOCK_STREAM, 0, sv) < 0) { + virReportSystemError(errno, "%s", "Cannot create socket pair"); + return -1; + } + + if (virNetSocketNewConnectSockFD(sv[0], &sock) < 0) + goto cleanup; + sv[0] = -1; + + if (!(client = virNetServerClientNew(1, sock, 0, false, 1, NULL, + testClientPrivNew, NULL, + testClientPrivFree, NULL))) + goto cleanup; + + virNetServerClientSetCloseHook(client, testClientClose); + + if (!(prog = virNetServerProgramNew(TEST_PROGRAM, 1, NULL, 0))) + goto cleanup; + + if (virPipe(pipeFD) < 0) + goto cleanup; + + if (!(st = virStreamNew(conn, VIR_STREAM_NONBLOCK))) + goto cleanup; + + if (virFDStreamOpen(st, pipeFD[0]) < 0) + goto cleanup; + pipeFD[0] = -1; + + if (!(stream = daemonCreateClientStream(client, st, prog, &header, false))) + goto cleanup; + + if (daemonAddClientStream(client, stream, true) < 0) + goto cleanup; + st = NULL; + + virNetServerClientImmediateClose(client); + + /* Hangs up the read end, so the stream reports EOF */ + VIR_FORCE_CLOSE(pipeFD[1]); + + signal(SIGALRM, testAlarm); + alarm(TEST_TIMEOUT); + + for (i = 0; i < 100 && !closeHookDone; i++) { + if (virEventRunDefaultImpl() < 0) + break; + } + + alarm(0); + + if (!closeHookDone) { + fprintf(stderr, "Client was not closed by the stream event\n"); + goto cleanup; + } + + ret = 0; + + cleanup: + virObjectUnref(st); + if (conn) + virConnectClose(conn); + virObjectUnref(prog); + virObjectUnref(sock); + if (client) + virNetServerClientClose(client); + virObjectUnref(client); + VIR_FORCE_CLOSE(pipeFD[0]); + VIR_FORCE_CLOSE(pipeFD[1]); + VIR_FORCE_CLOSE(sv[0]); + VIR_FORCE_CLOSE(sv[1]); + return ret; +} + + +static int +mymain(void) +{ + int ret = 0; + + virEventRegisterDefaultImpl(); + + if (virTestRun("Stream event on a client marked for close", + testStreamEventClose, NULL) < 0) + ret = -1; + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} +VIR_TEST_MAIN(mymain); +#else +static int +mymain(void) +{ + return EXIT_AM_SKIP; +} +VIR_TEST_MAIN(mymain); +#endif diff --git a/tests/meson.build b/tests/meson.build index 9d300de41d..2a97a18a52 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -492,6 +492,15 @@ endif if conf.has('WITH_REMOTE') tests += [ + { + 'name': 'daemonstreamtest', + 'sources': [ + 'daemonstreamtest.c', + remote_daemon_stream_sources, + remote_protocol_generated, + ], + 'include': [ remote_inc_dir ], + }, { 'name': 'virnetdaemontest' }, { 'name': 'virnetmessagetest' }, { 'name': 'virnetserverclienttest' }, -- 2.53.0
On 9/21/26 17:25, Denis V. Lunev via Devel wrote:
daemonStreamEvent() holds priv->lock across its whole body and closes the client from every error path. The close hook runs in the same thread and takes priv->lock again, so the daemon main loop deadlocks against itself and the host stops answering RPC entirely while its guests keep running. Only a restart of the daemon recovers.
We hit this in the field on 9.5.0, and current master is affected the same way. Getting there needs a client already marked wantClose while one of its streams still delivers events, which is presumably why it has gone unnoticed since 9.2.0.
Patch 1 closes the client after the lock guard has gone out of scope. Patch 2 adds a test for it, which needs remote_daemon_stream.c compiled into a test binary; that is the only build change and it pulls in the generated protocol headers via remote_daemon.h. The test deadlocks without patch 1 and passes with it.
Signed-off-by: Denis V. Lunev <den@openvz.org>
Denis V. Lunev (2): remote: fix daemon deadlock when a stream event closes its client tests: add daemonstreamtest covering the stream close deadlock
src/remote/meson.build | 7 +- src/remote/remote_daemon_stream.c | 58 ++++---- tests/daemonstreamtest.c | 212 ++++++++++++++++++++++++++++++ tests/meson.build | 9 ++ 4 files changed, 262 insertions(+), 24 deletions(-) create mode 100644 tests/daemonstreamtest.c
base-commit: 91d4618734bd6d02174b909aadb7d84a99560ebe
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> and merged. Michal
participants (2)
-
Denis V. Lunev -
Michal Prívozník