From 07f569e2f60de0078a22a181021342f19c1b1c41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 26 Oct 2023 10:47:03 +0200 Subject: [PATCH 1/2] Replace mutex for listener->connections with TID check The controlconf channel runs single-threaded on the main thread. Replace the listener->connections locking with check that we are still running on the thread with TID 0. --- bin/named/controlconf.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/bin/named/controlconf.c b/bin/named/controlconf.c index 9044d4dce4..5dbc97106f 100644 --- a/bin/named/controlconf.c +++ b/bin/named/controlconf.c @@ -94,7 +94,6 @@ struct controllistener { bool exiting; isc_refcount_t references; controlkeylist_t keys; - isc_mutex_t connections_lock; controlconnectionlist_t connections; isc_socktype_t type; uint32_t perm; @@ -170,9 +169,9 @@ free_controlkeylist(controlkeylist_t *keylist, isc_mem_t *mctx) { static void free_listener(controllistener_t *listener) { - INSIST(listener->exiting); - INSIST(ISC_LIST_EMPTY(listener->connections)); - + REQUIRE(isc_tid() == 0); + REQUIRE(listener->exiting); + REQUIRE(ISC_LIST_EMPTY(listener->connections)); REQUIRE(listener->sock == NULL); free_controlkeylist(&listener->keys, listener->mctx); @@ -180,7 +179,6 @@ free_listener(controllistener_t *listener) { if (listener->acl != NULL) { dns_acl_detach(&listener->acl); } - isc_mutex_destroy(&listener->connections_lock); isc_mem_putanddetach(&listener->mctx, listener, sizeof(*listener)); } @@ -195,6 +193,7 @@ ISC_REFCOUNT_IMPL(controlconnection, conn_free); static void shutdown_listener(controllistener_t *listener) { + REQUIRE(isc_tid() == 0); if (!listener->exiting) { char socktext[ISC_SOCKADDR_FORMATSIZE]; @@ -267,6 +266,8 @@ cleanup_sendhandle: if (result != ISC_R_SUCCESS) { control_recvmessage(handle, result, conn); } + + REQUIRE(isc_tid() == 0); controlconnection_detach(&conn); } @@ -373,6 +374,7 @@ control_respond(controlconnection_t *conn) { r.base = conn->buffer->base; r.length = conn->buffer->used; + REQUIRE(isc_tid() == 0); controlconnection_ref(conn); isccc_ccmsg_sendmessage(&conn->ccmsg, &r, control_senddone, conn); @@ -390,6 +392,8 @@ control_command(void *arg) { conn->request, listener->readonly, &conn->text); control_respond(conn); } + + REQUIRE(isc_tid() == 0); controlconnection_detach(&conn); } @@ -528,6 +532,7 @@ control_recvmessage(isc_nmhandle_t *handle ISC_ATTR_UNUSED, isc_result_t result, /* * Trigger the command. */ + REQUIRE(isc_tid() == 0); controlconnection_ref(conn); isc_async_run(named_g_mainloop, control_command, conn); @@ -535,11 +540,15 @@ control_recvmessage(isc_nmhandle_t *handle ISC_ATTR_UNUSED, isc_result_t result, cleanup: conn->shuttingdown = true; + + REQUIRE(isc_tid() == 0); controlconnection_detach(&conn); } static void conn_free(controlconnection_t *conn) { + REQUIRE(isc_tid() == 0); + controllistener_t *listener = conn->listener; conn_cleanup(conn); @@ -548,9 +557,7 @@ conn_free(controlconnection_t *conn) { isc_buffer_free(&conn->buffer); } - LOCK(&listener->connections_lock); ISC_LIST_UNLINK(listener->connections, conn, link); - UNLOCK(&listener->connections_lock); #ifdef ENABLE_AFL if (named_g_fuzz_type == isc_fuzz_rndc) { named_fuzz_notify(); @@ -570,6 +577,8 @@ conn_free(controlconnection_t *conn) { static void newconnection(controllistener_t *listener, isc_nmhandle_t *handle) { + REQUIRE(isc_tid() == 0); + controlconnection_t *conn = isc_mem_get(listener->mctx, sizeof(*conn)); isc_log_write(named_g_lctx, NAMED_LOGCATEGORY_GENERAL, NAMED_LOGMODULE_CONTROL, ISC_LOG_DEBUG(3), @@ -586,9 +595,7 @@ newconnection(controllistener_t *listener, isc_nmhandle_t *handle) { /* Set a 32 KiB upper limit on incoming message. */ isccc_ccmsg_setmaxsize(&conn->ccmsg, 32768); - LOCK(&listener->connections_lock); ISC_LIST_INITANDAPPEND(listener->connections, conn, link); - UNLOCK(&listener->connections_lock); isccc_ccmsg_readmessage(&conn->ccmsg, control_recvmessage, conn); } @@ -1041,7 +1048,6 @@ add_listener(named_controls_t *cp, controllistener_t **listenerp, .address = *addr, .type = type }; isc_mem_attach(mctx, &listener->mctx); - isc_mutex_init(&listener->connections_lock); ISC_LINK_INIT(listener, link); ISC_LIST_INIT(listener->keys); ISC_LIST_INIT(listener->connections); From 2d2c249958d3f983e77bdecaf4bfc4553dacad62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Fri, 27 Oct 2023 10:16:13 +0200 Subject: [PATCH 2/2] Call isccc_ccmsg_invalidate() when shutting down the connection Previously, the isccc_ccmsg_invalidate() was called from conn_free() and this could lead to netmgr calling control_recvmessage() after we detached the reading controlconnection_t reference, but it wouldn't be the last reference because controlconnection_t is also attached/detached when sending response or running command asynchronously. Instead, move the isccc_ccmsg_invalidate() call to control_recvmessage() error handling path to make sure that control_recvmessage() won't be ever called again from the netmgr. --- bin/named/controlconf.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/bin/named/controlconf.c b/bin/named/controlconf.c index 5dbc97106f..3b8b3f5f2b 100644 --- a/bin/named/controlconf.c +++ b/bin/named/controlconf.c @@ -407,9 +407,7 @@ control_recvmessage(isc_nmhandle_t *handle ISC_ATTR_UNUSED, isc_result_t result, isccc_time_t exp; uint32_t nonce; - if (conn->shuttingdown) { - return; - } + INSIST(!conn->shuttingdown); /* Is the server shutting down? */ if (listener->controls->shuttingdown) { @@ -539,6 +537,9 @@ control_recvmessage(isc_nmhandle_t *handle ISC_ATTR_UNUSED, isc_result_t result, return; cleanup: + /* Make sure no read callbacks are called again */ + isccc_ccmsg_invalidate(&conn->ccmsg); + conn->shuttingdown = true; REQUIRE(isc_tid() == 0); @@ -564,8 +565,6 @@ conn_free(controlconnection_t *conn) { } #endif /* ifdef ENABLE_AFL */ - isccc_ccmsg_invalidate(&conn->ccmsg); - isc_log_write(named_g_lctx, NAMED_LOGCATEGORY_GENERAL, NAMED_LOGMODULE_CONTROL, ISC_LOG_DEBUG(3), "freeing control connection");