From 5734d6c8266a6baecbd9907e38e9caa506250164 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Mon, 6 Nov 2023 20:19:20 +0100 Subject: [PATCH] Make sure we shutdown the controlconf listeners and connections once It was possible that controlconf connections could be shutdown twice when shutting down the server, because they would receive the signal (ISC_R_SHUTTINGDOWN result) from netmgr and then the shutdown procedure would be called second time via controls_shutdown(). Split the shutdown procedure from control_recvmessage(), so we can call it independently from netmgr callbacks and make sure it will be called only once. Do the similar thing for the listeners. --- bin/named/control.c | 2 + bin/named/controlconf.c | 223 +++++++++++++++++++++++----------------- 2 files changed, 132 insertions(+), 93 deletions(-) diff --git a/bin/named/control.c b/bin/named/control.c index 7f07db1240..a3e009799e 100644 --- a/bin/named/control.c +++ b/bin/named/control.c @@ -177,6 +177,7 @@ named_control_docommand(isccc_sexpr_t *message, bool readonly, /* Do not flush master files */ named_server_flushonshutdown(named_g_server, false); named_os_shutdownmsg(cmdline, *text); + isc_loopmgr_shutdown(named_g_loopmgr); result = ISC_R_SHUTTINGDOWN; } else if (command_compare(command, NAMED_COMMAND_STOP)) { /* @@ -194,6 +195,7 @@ named_control_docommand(isccc_sexpr_t *message, bool readonly, #endif /* ifdef HAVE_LIBSCF */ named_server_flushonshutdown(named_g_server, true); named_os_shutdownmsg(cmdline, *text); + isc_loopmgr_shutdown(named_g_loopmgr); result = ISC_R_SHUTTINGDOWN; } else if (command_compare(command, NAMED_COMMAND_ADDZONE) || command_compare(command, NAMED_COMMAND_MODZONE)) diff --git a/bin/named/controlconf.c b/bin/named/controlconf.c index 3b8b3f5f2b..d951073a7e 100644 --- a/bin/named/controlconf.c +++ b/bin/named/controlconf.c @@ -91,7 +91,7 @@ struct controllistener { isc_sockaddr_t address; isc_nmsocket_t *sock; dns_acl_t *acl; - bool exiting; + bool shuttingdown; isc_refcount_t references; controlkeylist_t keys; controlconnectionlist_t connections; @@ -119,6 +119,8 @@ static void conn_cleanup(controlconnection_t *conn); static void conn_free(controlconnection_t *conn); +static void +conn_shutdown(controlconnection_t *conn); #if NAMED_CONTROLCONF_TRACE #define controllistener_ref(ptr) \ @@ -147,6 +149,14 @@ ISC_REFCOUNT_DECL(controlconnection); #define CLOCKSKEW 300 +#define CHECK(x) \ + { \ + result = (x); \ + if (result != ISC_R_SUCCESS) { \ + goto cleanup; \ + } \ + } + static void free_controlkey(controlkey_t *key, isc_mem_t *mctx) { if (key->keyname != NULL) { @@ -169,8 +179,7 @@ free_controlkeylist(controlkeylist_t *keylist, isc_mem_t *mctx) { static void free_listener(controllistener_t *listener) { - REQUIRE(isc_tid() == 0); - REQUIRE(listener->exiting); + REQUIRE(listener->shuttingdown); REQUIRE(ISC_LIST_EMPTY(listener->connections)); REQUIRE(listener->sock == NULL); @@ -193,28 +202,25 @@ 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]; - - for (controlconnection_t *conn = - ISC_LIST_HEAD(listener->connections); - conn != NULL; conn = ISC_LIST_HEAD(listener->connections)) - { - control_recvmessage(conn->ccmsg.handle, - ISC_R_SHUTTINGDOWN, conn); - } - - ISC_LIST_UNLINK(listener->controls->listeners, listener, link); - - isc_sockaddr_format(&listener->address, socktext, - sizeof(socktext)); - isc_log_write(named_g_lctx, NAMED_LOGCATEGORY_GENERAL, - NAMED_LOGMODULE_CONTROL, ISC_LOG_NOTICE, - "stopping command channel on %s", socktext); - - listener->exiting = true; + /* Don't shutdown the same listener twice */ + if (listener->shuttingdown) { + return; } + listener->shuttingdown = true; + + for (controlconnection_t *conn = ISC_LIST_HEAD(listener->connections); + conn != NULL; conn = ISC_LIST_HEAD(listener->connections)) + { + conn_shutdown(conn); + } + + ISC_LIST_UNLINK(listener->controls->listeners, listener, link); + + char socktext[ISC_SOCKADDR_FORMATSIZE]; + isc_sockaddr_format(&listener->address, socktext, sizeof(socktext)); + isc_log_write(named_g_lctx, NAMED_LOGCATEGORY_GENERAL, + NAMED_LOGMODULE_CONTROL, ISC_LOG_NOTICE, + "stopping command channel on %s", socktext); isc_nm_stoplistening(listener->sock); isc_nmsocket_close(&listener->sock); @@ -239,35 +245,35 @@ address_ok(isc_sockaddr_t *sockaddr, controllistener_t *listener) { static void control_senddone(isc_nmhandle_t *handle, isc_result_t result, void *arg) { controlconnection_t *conn = (controlconnection_t *)arg; - controllistener_t *listener = conn->listener; - isc_sockaddr_t peeraddr = isc_nmhandle_peeraddr(handle); - if (conn->result == ISC_R_SHUTTINGDOWN) { - isc_loopmgr_shutdown(named_g_loopmgr); - goto cleanup_sendhandle; + if (conn->shuttingdown) { + /* The connection is shuttingdown */ + result = ISC_R_SHUTTINGDOWN; } - if (listener->controls->shuttingdown || result == ISC_R_SHUTTINGDOWN) { - goto cleanup_sendhandle; - } else if (result != ISC_R_SUCCESS) { + if (result == ISC_R_SUCCESS) { + /* Everything is peachy, continue reading from the socket */ + isccc_ccmsg_readmessage(&conn->ccmsg, control_recvmessage, + conn); + goto done; + } + + /* This is the error path */ + if (result != ISC_R_SHUTTINGDOWN) { char socktext[ISC_SOCKADDR_FORMATSIZE]; + isc_sockaddr_t peeraddr = isc_nmhandle_peeraddr(handle); isc_sockaddr_format(&peeraddr, socktext, sizeof(socktext)); isc_log_write(named_g_lctx, NAMED_LOGCATEGORY_GENERAL, NAMED_LOGMODULE_CONTROL, ISC_LOG_WARNING, "error sending command response to %s: %s", socktext, isc_result_totext(result)); - goto cleanup_sendhandle; } - isccc_ccmsg_readmessage(&conn->ccmsg, control_recvmessage, conn); + conn_shutdown(conn); -cleanup_sendhandle: - if (result != ISC_R_SUCCESS) { - control_recvmessage(handle, result, conn); - } - - REQUIRE(isc_tid() == 0); +done: + /* Detach the sending reference */ controlconnection_detach(&conn); } @@ -374,7 +380,7 @@ control_respond(controlconnection_t *conn) { r.base = conn->buffer->base; r.length = conn->buffer->used; - REQUIRE(isc_tid() == 0); + /* Attach the sending reference */ controlconnection_ref(conn); isccc_ccmsg_sendmessage(&conn->ccmsg, &r, control_senddone, conn); @@ -385,15 +391,33 @@ cleanup: static void control_command(void *arg) { controlconnection_t *conn = (controlconnection_t *)arg; - controllistener_t *listener = conn->listener; - if (!listener->controls->shuttingdown) { + /* Don't run the command if we already started the shutdown */ + if (!conn->shuttingdown) { conn->result = named_control_docommand( - conn->request, listener->readonly, &conn->text); + conn->request, conn->listener->readonly, &conn->text); control_respond(conn); } - REQUIRE(isc_tid() == 0); + /* Detach the control command reference */ + controlconnection_detach(&conn); +} + +static void +conn_shutdown(controlconnection_t *conn) { + /* Don't shutdown the same controlconnection twice */ + if (conn->shuttingdown) { + return; + } + conn->shuttingdown = true; + + /* + * Calling invalidate on ccmsg will shutdown the TCP connection, thus + * we are making sure that no read callback will be called ever again. + */ + isccc_ccmsg_invalidate(&conn->ccmsg); + + /* Detach the reading reference */ controlconnection_detach(&conn); } @@ -407,19 +431,7 @@ control_recvmessage(isc_nmhandle_t *handle ISC_ATTR_UNUSED, isc_result_t result, isccc_time_t exp; uint32_t nonce; - INSIST(!conn->shuttingdown); - - /* Is the server shutting down? */ - if (listener->controls->shuttingdown) { - result = ISC_R_SHUTTINGDOWN; - } - if (result != ISC_R_SUCCESS) { - if (result == ISC_R_SHUTTINGDOWN) { - listener->controls->shuttingdown = true; - } else if (result != ISC_R_EOF) { - log_invalid(&conn->ccmsg, result); - } goto cleanup; } @@ -445,13 +457,13 @@ control_recvmessage(isc_nmhandle_t *handle ISC_ATTR_UNUSED, isc_result_t result, } if (key == NULL) { - log_invalid(&conn->ccmsg, ISCCC_R_BADAUTH); + result = ISCCC_R_BADAUTH; goto cleanup; } /* We shouldn't be getting a reply. */ if (isccc_cc_isreply(conn->request)) { - log_invalid(&conn->ccmsg, ISC_R_FAILURE); + result = ISC_R_FAILURE; goto cleanup; } @@ -462,7 +474,7 @@ control_recvmessage(isc_nmhandle_t *handle ISC_ATTR_UNUSED, isc_result_t result, */ conn->ctrl = isccc_alist_lookup(conn->request, "_ctrl"); if (!isccc_alist_alistp(conn->ctrl)) { - log_invalid(&conn->ccmsg, ISC_R_FAILURE); + result = ISC_R_FAILURE; goto cleanup; } @@ -470,11 +482,11 @@ control_recvmessage(isc_nmhandle_t *handle ISC_ATTR_UNUSED, isc_result_t result, if ((sent + CLOCKSKEW) < conn->now || (sent - CLOCKSKEW) > conn->now) { - log_invalid(&conn->ccmsg, ISCCC_R_CLOCKSKEW); + result = ISCCC_R_CLOCKSKEW; goto cleanup; } } else { - log_invalid(&conn->ccmsg, ISC_R_FAILURE); + result = ISC_R_FAILURE; goto cleanup; } @@ -484,7 +496,7 @@ control_recvmessage(isc_nmhandle_t *handle ISC_ATTR_UNUSED, isc_result_t result, if (isccc_cc_lookupuint32(conn->ctrl, "_exp", &exp) == ISC_R_SUCCESS && conn->now > exp) { - log_invalid(&conn->ccmsg, ISCCC_R_EXPIRED); + result = ISCCC_R_EXPIRED; goto cleanup; } @@ -500,7 +512,6 @@ control_recvmessage(isc_nmhandle_t *handle ISC_ATTR_UNUSED, isc_result_t result, if (result == ISC_R_EXISTS) { result = ISCCC_R_DUPLICATE; } - log_invalid(&conn->ccmsg, result); goto cleanup; } @@ -509,7 +520,7 @@ control_recvmessage(isc_nmhandle_t *handle ISC_ATTR_UNUSED, isc_result_t result, ISC_R_SUCCESS || conn->nonce != nonce)) { - log_invalid(&conn->ccmsg, ISCCC_R_BADAUTH); + result = ISCCC_R_BADAUTH; goto cleanup; } @@ -527,28 +538,33 @@ control_recvmessage(isc_nmhandle_t *handle ISC_ATTR_UNUSED, isc_result_t result, return; } - /* - * Trigger the command. - */ - REQUIRE(isc_tid() == 0); + /* Attach the command reference */ controlconnection_ref(conn); + + /* Trigger the command asynchronously. */ isc_async_run(named_g_mainloop, control_command, conn); return; cleanup: - /* Make sure no read callbacks are called again */ - isccc_ccmsg_invalidate(&conn->ccmsg); + switch (result) { + case ISC_R_SHUTTINGDOWN: + case ISC_R_EOF: + break; + default: + /* We can't get here on normal path */ + INSIST(result != ISC_R_SUCCESS); - conn->shuttingdown = true; + log_invalid(&conn->ccmsg, result); + } - REQUIRE(isc_tid() == 0); - controlconnection_detach(&conn); + conn_shutdown(conn); } static void conn_free(controlconnection_t *conn) { - REQUIRE(isc_tid() == 0); + /* Make sure that the connection was shutdown first */ + REQUIRE(conn->shuttingdown); controllistener_t *listener = conn->listener; @@ -576,17 +592,26 @@ conn_free(controlconnection_t *conn) { static void newconnection(controllistener_t *listener, isc_nmhandle_t *handle) { - REQUIRE(isc_tid() == 0); + /* Don't create new connection if we are shutting down */ + if (listener->shuttingdown) { + isc_log_write(named_g_lctx, NAMED_LOGCATEGORY_GENERAL, + NAMED_LOGMODULE_CONTROL, ISC_LOG_DEBUG(3), + "rejected new control connection: %s", + isc_result_totext(ISC_R_SHUTTINGDOWN)); + return; + } 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), "allocate new control connection"); - *conn = (controlconnection_t){ .alg = DST_ALG_UNKNOWN }; - - isc_refcount_init(&conn->references, 1); - controllistener_attach(listener, &conn->listener); + *conn = (controlconnection_t){ + .alg = DST_ALG_UNKNOWN, + .references = ISC_REFCOUNT_INITIALIZER(1), + .listener = controllistener_ref(listener), + .link = ISC_LINK_INITIALIZER, + }; /* isccc_ccmsg_init() attaches to the handle */ isccc_ccmsg_init(listener->mctx, handle, &conn->ccmsg); @@ -594,8 +619,9 @@ newconnection(controllistener_t *listener, isc_nmhandle_t *handle) { /* Set a 32 KiB upper limit on incoming message. */ isccc_ccmsg_setmaxsize(&conn->ccmsg, 32768); - ISC_LIST_INITANDAPPEND(listener->connections, conn, link); + ISC_LIST_APPEND(listener->connections, conn, link); + /* The reading reference has been initialized in the initializer */ isccc_ccmsg_readmessage(&conn->ccmsg, control_recvmessage, conn); } @@ -635,8 +661,7 @@ controls_shutdown(named_controls_t *controls) { listener = next) { /* - * This is asynchronous. As listeners shut down, they will - * call their callbacks. + * As listeners shut down, they will call their callbacks. */ next = ISC_LIST_NEXT(listener, link); shutdown_listener(listener); @@ -645,8 +670,20 @@ controls_shutdown(named_controls_t *controls) { void named_controls_shutdown(named_controls_t *controls) { - controls_shutdown(controls); + /* + * Don't ever shutdown the controls twice. + * + * NOTE: This functions is called when the server is shutting down, but + * controls_shutdown() can and will be called multiple times - on each + * reconfiguration, the listeners will be torn down and recreated again, + * see named_controls_configure() for details. + */ + if (controls->shuttingdown) { + return; + } controls->shuttingdown = true; + + controls_shutdown(controls); } static isc_result_t @@ -775,14 +812,6 @@ register_keys(const cfg_obj_t *control, const cfg_obj_t *keylist, } } -#define CHECK(x) \ - do { \ - result = (x); \ - if (result != ISC_R_SUCCESS) { \ - goto cleanup; \ - } \ - } while (0) - static isc_result_t get_rndckey(isc_mem_t *mctx, controlkeylist_t *keyids) { isc_result_t result; @@ -1042,6 +1071,12 @@ add_listener(named_controls_t *cp, controllistener_t **listenerp, isc_result_t result = ISC_R_SUCCESS; int pf; + /* Don't create new listener if we are shutting down */ + if (cp->shuttingdown) { + result = ISC_R_SHUTTINGDOWN; + goto shuttingdown; + } + listener = isc_mem_get(mctx, sizeof(*listener)); *listener = (controllistener_t){ .controls = cp, .address = *addr, @@ -1112,9 +1147,10 @@ add_listener(named_controls_t *cp, controllistener_t **listenerp, cleanup: isc_refcount_decrement(&listener->references); - listener->exiting = true; + listener->shuttingdown = true; free_listener(listener); +shuttingdown: if (control != NULL) { cfg_obj_log(control, named_g_lctx, ISC_LOG_WARNING, "couldn't add command channel %s: %s", socktext, @@ -1332,6 +1368,7 @@ named_controls_destroy(named_controls_t **ctrlsp) { named_controls_t *controls = *ctrlsp; *ctrlsp = NULL; + REQUIRE(controls->shuttingdown); REQUIRE(ISC_LIST_EMPTY(controls->listeners)); LOCK(&controls->symtab_lock);