From: Timo Sirainen Date: Wed, 2 Feb 2022 19:15:13 +0000 (+0100) Subject: lib-master: Add support for for race-free kicking of users with SIGTERM X-Git-Tag: 2.4.0~4417 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=14da525deeb16d3e81b2b9cf24fc52092b33f88d;p=thirdparty%2Fdovecot%2Fcore.git lib-master: Add support for for race-free kicking of users with SIGTERM This works by anvil connecting to the per-process admin socket and sending KICK-USER-SIGNAL command, followed by SIGTERM. The receiving process usually gets the SIGTERM and attempts to accept() the admin socket and read() the KICK-USER-SIGNAL command. If the command is for a different user than is currently running, the signal is ignored. If the SIGTERM is delayed, the generic admin connection needs to support handling the KICK-USER-SIGNAL. This is done by blocking the SIGTERM early while accessing the admin connection. If the SIGTERM handler finds out that the admin socket was accepted but a different command than KICK-USER-SIGNAL was found, this means a rather race condition with another admin command. The process was most likely going to be killed anyway, so just ignore the command and kill the process. --- diff --git a/src/lib-master/master-admin-client.c b/src/lib-master/master-admin-client.c index f11acaf3f3..4625cf0a41 100644 --- a/src/lib-master/master-admin-client.c +++ b/src/lib-master/master-admin-client.c @@ -10,6 +10,7 @@ struct master_admin_client { struct connection conn; + struct ioloop *wait_ioloop; bool reply_pending; }; @@ -40,6 +41,23 @@ cmd_kick_user(struct master_admin_client *client, const char *const *args) master_admin_client_callbacks.cmd_kick_user(user, conn_guid))); } +static void +cmd_kick_user_signal(struct master_admin_client *client, + const char *const *args) +{ + const char *user = args[0]; + if (user == NULL) { + master_admin_client_send_reply(client, "-Missing parameter"); + return; + } + + /* This command is usually handled by the signal handler, but looks + like the signal handling was delayed. Remember the username for + the signal. */ + master_service_set_last_kick_signal_user(master_service, user); + /* Don't send a response back, just like the signal handler won't. */ +} + static int master_admin_client_input_args(struct connection *conn, const char *const *args) { @@ -55,12 +73,22 @@ master_admin_client_input_args(struct connection *conn, const char *const *args) const char *cmd = args[0]; args++; + if (client->wait_ioloop != NULL) { + /* A command was received while waiting in + master_admin_client_initial_read(). Now that we've seen it, + stop the wait ioloop after the command is finished. */ + io_loop_stop(client->wait_ioloop); + } + client->reply_pending = TRUE; if (strcmp(cmd, "KICK-USER") == 0 && master_admin_client_callbacks.cmd_kick_user != NULL) cmd_kick_user(client, args); - else if (master_admin_client_callbacks.cmd == NULL || - !master_admin_client_callbacks.cmd(client, cmd, args)) { + else if (strcmp(cmd, "KICK-USER-SIGNAL") == 0) { + cmd_kick_user_signal(client, args); + return -1; + } else if (master_admin_client_callbacks.cmd == NULL || + !master_admin_client_callbacks.cmd(client, cmd, args)) { client->reply_pending = FALSE; o_stream_nsend_str(conn->output, "-Unknown command\n"); } @@ -113,6 +141,21 @@ static const struct connection_vfuncs master_admin_conn_vfuncs = { .input_args = master_admin_client_input_args }; +static void master_admin_client_initial_read(struct master_admin_client *client) +{ + struct ioloop *prev_ioloop = current_ioloop; + client->wait_ioloop = io_loop_create(); + connection_switch_ioloop(&client->conn); + struct timeout *to = + timeout_add_short(100, io_loop_stop, client->wait_ioloop); + + io_loop_run(client->wait_ioloop); + + timeout_remove(&to); + connection_switch_ioloop_to(&client->conn, prev_ioloop); + io_loop_destroy(&client->wait_ioloop); +} + void master_admin_client_create(struct master_service_connection *master_conn) { struct master_admin_client *client; @@ -126,6 +169,14 @@ void master_admin_client_create(struct master_service_connection *master_conn) client = i_new(struct master_admin_client, 1); connection_init_server(master_admin_clients, &client->conn, master_conn->name, master_conn->fd, master_conn->fd); + if (master_service_get_client_limit(master_service) == 1) { + /* client_limit=1 for this process, so this connection is + likely to be for KICK-USER-SIGNAL command. We're currently + blocking the SIGTERM, so try for a while to read the command + here. This way the command can be handled more reliably + instead of SIGTERM interrupting its handling too early. */ + master_admin_client_initial_read(client); + } } bool master_admin_client_can_accept(const char *name) diff --git a/src/lib-master/master-service-private.h b/src/lib-master/master-service-private.h index bf28ef7407..2349c76b7e 100644 --- a/src/lib-master/master-service-private.h +++ b/src/lib-master/master-service-private.h @@ -4,6 +4,8 @@ #include "master-interface.h" #include "master-service.h" +#include + struct master_service_haproxy_conn; struct master_service_listener { @@ -72,6 +74,8 @@ struct master_service { time_t ssl_params_last_refresh; char *current_user; + char *last_kick_signal_user; + volatile sig_atomic_t last_kick_signal_user_accessed; struct stats_client *stats_client; struct master_service_haproxy_conn *haproxy_conns; @@ -85,6 +89,7 @@ struct master_service { bool die_with_master:1; bool call_avail_overflow:1; bool config_path_changed_with_param:1; + bool have_admin_sockets:1; bool want_ssl_server:1; bool ssl_ctx_initialized:1; bool config_path_from_master:1; diff --git a/src/lib-master/master-service.c b/src/lib-master/master-service.c index a1210d0d4d..6b222ce0f9 100644 --- a/src/lib-master/master-service.c +++ b/src/lib-master/master-service.c @@ -7,6 +7,7 @@ #include "ioloop.h" #include "hostpid.h" #include "path-util.h" +#include "net.h" #include "array.h" #include "str.h" #include "strescape.h" @@ -65,6 +66,21 @@ const char *master_service_getopt_string(void) return "c:i:ko:OL"; } +static int block_sigterm(sigset_t *oldmask_r) +{ + sigset_t sigmask; + + if (sigemptyset(&sigmask) < 0) + i_error("sigemptyset() failed: %m"); + else if (sigaddset(&sigmask, SIGTERM) < 0) + i_error("sigaddset(SIGTERM) failed: %m"); + else if (sigprocmask(SIG_BLOCK, &sigmask, oldmask_r) < 0) + i_error("sigprocmask(SIG_BLOCK, SIGTERM) failed: %m"); + else + return 0; + return -1; +} + static void sig_die(const siginfo_t *si, void *context) { struct master_service *service = context; @@ -99,6 +115,151 @@ static void sig_die(const siginfo_t *si, void *context) io_loop_stop(service->ioloop); } +static bool sig_term_buf_get_kick_user(char *buf, const char **user_r) +{ + /* WARNING: We are in a (non-delayed) signal handler context. + Be VERY careful what functions you call. */ + if (strncmp(buf, "VERSION\tmaster-admin-client\t1\t", 30) != 0) + return FALSE; + buf += 30; + /* skip over minor version */ + while (*buf >= '0' && *buf <= '0') buf++; + if (*buf != '\n') + return FALSE; + buf++; + + if (strncmp(buf, "KICK-USER-SIGNAL\t", 17) != 0) + return FALSE; + buf += 17; + + /* [] - Handling the conn-guid is too much effort, + it should normally be enough to just check the user. */ + char *p = strpbrk(buf, "\t\n"); + if (p == NULL) + return FALSE; + *p = '\0'; + + *user_r = buf; + return TRUE; +} + +static bool +sig_service_kick_user_match(struct master_service *service, const char *user) +{ + /* WARNING: We are in a (non-delayed) signal handler context. + Be VERY careful what functions you call. */ + if (service->current_user != NULL) + return strcmp(user, service->current_user) == 0 ? 1 : 0; + else { + /* There is no currently accessed user. Most likely it + means that the process already stopped handling the + requested user. */ + return 0; + } +} + +static int sig_term_try_kick_user(struct master_service *service, int fd_listen) +{ + /* WARNING: We are in a (non-delayed) signal handler context. + Be VERY careful what functions you call. */ + struct sockaddr sa; + int fd, ret = -1; + char buf[256]; + ssize_t bytes; + socklen_t addrlen; + + if (service->last_kick_signal_user != NULL && + service->last_kick_signal_user_accessed == 0) { + /* The signal came a bit late. The KICK-USER-SIGNAL command + was already handled. */ + service->last_kick_signal_user_accessed = 1; + return sig_service_kick_user_match(service, + service->last_kick_signal_user) ? 1 : 0; + } + + fd = accept(fd_listen, &sa, &addrlen); + if (fd < 0) { + if (errno == EAGAIN || errno == ECONNABORTED) + return -1; + lib_signals_syscall_error("SIGTERM: accept() failed: "); + return -1; + } + alarm(1); + bytes = read(fd, buf, sizeof(buf)-1); + alarm(0); + if (bytes >= 0) { + const char *user; + buf[bytes] = '\0'; + if (!sig_term_buf_get_kick_user(buf, &user)) { + /* This wasn't a KICK-USER-SIGNAL command at all. The + process will be soon killed with a delayed SIGTERM, + so we can simply close the connection and ignore the + command. */ + } else { + ret = sig_service_kick_user_match(service, user) ? 1 : 0; + } + } else if (errno != EINTR) { + lib_signals_syscall_error("SIGTERM: read() failed: "); + } + if (close(fd) < 0) + lib_signals_syscall_error("SIGTERM: close() failed: "); + return ret; +} + +static bool sig_term_try_kick(struct master_service *service) +{ + /* WARNING: We are in a (non-delayed) signal handler context. + Be VERY careful what functions you call. */ + int ret; + + /* see if there's a admin-socket connection waiting */ + for (unsigned int i = 0; i < service->socket_count; i++) { + struct master_service_listener *l = &service->listeners[i]; + + if (master_admin_client_can_accept(l->name)) { + ret = sig_term_try_kick_user(service, l->fd); + if (ret > 0) { + /* USER-KICK matched */ + return TRUE; + } + if (ret == 0) { + /* USER-KICK mismatch - ignore */ + return FALSE; + } + /* no connection or not a USER-KICK command */ + } + } + /* no. just handle the signal normally as a delayed signal. */ + return TRUE; +} + +static void sig_term(const siginfo_t *si, void *context) +{ + /* WARNING: We are in a (non-delayed) signal handler context. + Be VERY careful what functions you call. */ + struct master_service *service = context; + sigset_t sigmask, oldmask; + int saved_errno = errno; + bool call_delayed = TRUE; + + /* Block SIGTERM so that we don't get back here recursively. */ + if (sigemptyset(&sigmask) < 0) + lib_signals_syscall_error("SIGTERM: sigemptyset() failed: "); + else if (sigaddset(&sigmask, SIGTERM) < 0) + lib_signals_syscall_error("SIGTERM: sigaddset() failed: "); + else if (sigprocmask(SIG_BLOCK, &sigmask, &oldmask) < 0) + lib_signals_syscall_error("SIGTERM: sigprocmask(SIG_BLOCK) failed: "); + else { + call_delayed = sig_term_try_kick(service); + if (sigprocmask(SIG_SETMASK, &oldmask, NULL) < 0) + lib_signals_syscall_error("SIGTERM: sigprocmask(SIG_SETMASK) failed: "); + } + + if (call_delayed) + lib_signal_delayed(si); + errno = saved_errno; +} + static void sig_close_listeners(const siginfo_t *si ATTR_UNUSED, void *context) { struct master_service *service = context; @@ -189,6 +350,8 @@ static void master_service_init_socket_listeners(struct master_service *service) if (*settings != NULL) { l->name = i_strdup_empty(*settings); + if (master_admin_client_can_accept(l->name)) + service->have_admin_sockets = TRUE; settings++; } while (*settings != NULL) { @@ -678,7 +841,10 @@ void master_service_init_finish(struct master_service *service) if ((service->flags & MASTER_SERVICE_FLAG_STANDALONE) == 0) sigint_flags |= LIBSIG_FLAG_RESTART; lib_signals_set_handler(SIGINT, sigint_flags, sig_die, service); - lib_signals_set_handler(SIGTERM, LIBSIG_FLAG_DELAYED, sig_die, service); + if (!service->have_admin_sockets) + lib_signals_set_handler(SIGTERM, LIBSIG_FLAG_DELAYED, sig_die, service); + else + lib_signals_set_handler2(SIGTERM, 0, sig_term, sig_die, service); if ((service->flags & MASTER_SERVICE_FLAG_TRACK_LOGIN_STATE) != 0) { lib_signals_set_handler(SIGUSR1, LIBSIG_FLAGS_SAFE, sig_state_changed, service); @@ -974,10 +1140,12 @@ bool master_service_anvil_connect(struct master_service *service, str_append_c(cmd, '\t'); if (!kick_supported) str_append_c(cmd, 'N'); - else if (master_service_get_client_limit(service) == 1) - str_append_c(cmd, 'S'); - else + else if (master_service_get_client_limit(service) > 1) str_append_c(cmd, 'A'); + else if (service->have_admin_sockets) + str_append_c(cmd, 'W'); + else + str_append_c(cmd, 'S'); str_append_c(cmd, '\t'); if (session->dest_ip.family != 0) str_append(cmd, net_ip2addr(&session->dest_ip)); @@ -1432,7 +1600,20 @@ static void master_service_listen(struct master_service_listener *l) i_assert(service->master_status.available_count > 0); } + sigset_t oldmask; + bool sigterm_blocked = FALSE; + if (master_admin_conn) { + /* Keep SIGTERM blocked while handling a master-admin + connection. This prevents race conditions with the SIGTERM + being received while handling the KICK-USER-SIGNAL + command. */ + sigterm_blocked = block_sigterm(&oldmask) == 0; + } master_service_accept(l, conn_name, master_admin_conn); + if (sigterm_blocked) { + if (sigprocmask(SIG_SETMASK, &oldmask, NULL) < 0) + i_error("sigprocmask(SIG_SETMASK) failed: %m"); + } } void master_service_io_listeners_add(struct master_service *service) @@ -1645,7 +1826,33 @@ void master_service_unset_process_shutdown_filter(struct master_service *service void master_service_set_current_user(struct master_service *service, const char *user) { + /* block the signal to avoid races accessing current_user */ + sigset_t oldmask; + bool sigterm_blocked = block_sigterm(&oldmask) == 0; + char *old_user = service->current_user; service->current_user = i_strdup(user); i_free(old_user); + + if (sigterm_blocked) { + if (sigprocmask(SIG_SETMASK, &oldmask, NULL) < 0) + i_error("sigprocmask(SIG_SETMASK) failed: %m"); + } +} + +void master_service_set_last_kick_signal_user(struct master_service *service, + const char *user) +{ + /* block the signal to avoid races accessing last_kick_signal_user */ + sigset_t oldmask; + bool sigterm_blocked = block_sigterm(&oldmask) == 0; + + i_free(service->last_kick_signal_user); + service->last_kick_signal_user = i_strdup(user); + service->last_kick_signal_user_accessed = 0; + + if (sigterm_blocked) { + if (sigprocmask(SIG_SETMASK, &oldmask, NULL) < 0) + i_error("sigprocmask(SIG_SETMASK) failed: %m"); + } } diff --git a/src/lib-master/master-service.h b/src/lib-master/master-service.h index 6a2e9b8071..49317cbe01 100644 --- a/src/lib-master/master-service.h +++ b/src/lib-master/master-service.h @@ -284,5 +284,9 @@ void master_service_unset_process_shutdown_filter(struct master_service *service users due to race conditions. */ void master_service_set_current_user(struct master_service *service, const char *user); +/* Set the user that is expected to be kicked by the next SIGTERM. If it + doesn't match the current_user, the SIGTERM will be ignored. */ +void master_service_set_last_kick_signal_user(struct master_service *service, + const char *user); #endif