From c62994e6a44416a5df4c7d06f39f233f77c9d4ac Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Fri, 14 Oct 2022 20:45:40 +0300 Subject: [PATCH 1/2] Synchronise stop listening operation for multi-layer transports This commit introduces a primitive isc__nmsocket_stop() which performs shutting down on a multilayered socket ensuring the proper order of the operations. The shared data within the socket object can be destroyed after the call completed, as it is guaranteed to not be used from within the context of other worker threads. (cherry picked from commit 5ab2c0ebb386f94384c1d16b44d61f71276fe59a) --- lib/isc/netmgr/http.c | 37 ++--------------- lib/isc/netmgr/netmgr-int.h | 35 +++++++++++++--- lib/isc/netmgr/netmgr.c | 79 ++++++++++++++++++++++++++++++++++++- lib/isc/netmgr/tlsstream.c | 29 ++++++-------- 4 files changed, 122 insertions(+), 58 deletions(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index e604ee7d63..614bada711 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -2527,6 +2527,9 @@ isc_nm_listenhttp(isc_nm_t *mgr, isc_sockaddr_t *iface, int backlog, sock->tid = 0; sock->fd = (uv_os_sock_t)-1; + isc__nmsocket_barrier_init(sock); + atomic_init(&sock->rchildren, sock->nchildren); + atomic_store(&sock->listening, true); *sockp = sock; return (ISC_R_SUCCESS); @@ -2694,39 +2697,7 @@ isc__nm_http_stoplistening(isc_nmsocket_t *sock) { REQUIRE(VALID_NMSOCK(sock)); REQUIRE(sock->type == isc_nm_httplistener); - if (!atomic_compare_exchange_strong(&sock->closing, &(bool){ false }, - true)) { - UNREACHABLE(); - } - - if (!isc__nm_in_netthread()) { - isc__netievent_httpstop_t *ievent = - isc__nm_get_netievent_httpstop(sock->mgr, sock); - isc__nm_enqueue_ievent(&sock->mgr->workers[sock->tid], - (isc__netievent_t *)ievent); - } else { - REQUIRE(isc_nm_tid() == sock->tid); - isc__netievent_httpstop_t ievent = { .sock = sock }; - isc__nm_async_httpstop(NULL, (isc__netievent_t *)&ievent); - } -} - -void -isc__nm_async_httpstop(isc__networker_t *worker, isc__netievent_t *ev0) { - isc__netievent_httpstop_t *ievent = (isc__netievent_httpstop_t *)ev0; - isc_nmsocket_t *sock = ievent->sock; - - UNUSED(worker); - - REQUIRE(VALID_NMSOCK(sock)); - - atomic_store(&sock->listening, false); - atomic_store(&sock->closing, false); - atomic_store(&sock->closed, true); - if (sock->outer != NULL) { - isc_nm_stoplistening(sock->outer); - isc_nmsocket_close(&sock->outer); - } + isc__nmsocket_stop(sock); } static void diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index 2a51fe35ca..d5269753ac 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -333,6 +333,7 @@ typedef enum isc__netievent_type { netievent_privilegedtask, netievent_settlsctx, + netievent_sockstop, /* for multilayer sockets */ /* * event type values higher than this will be treated @@ -349,7 +350,6 @@ typedef enum isc__netievent_type { netievent_tcpdnsstop, netievent_tlsdnslisten, netievent_tlsdnsstop, - netievent_httpstop, netievent_resume, netievent_detach, @@ -1221,6 +1221,8 @@ struct isc_nmsocket { atomic_int_fast32_t active_child_connections; + isc_barrier_t barrier; + bool barrier_initialised; #ifdef NETMGR_TRACE void *backtrace[TRACE_SIZE]; int backtrace_size; @@ -1854,9 +1856,6 @@ isc__nm_http_verify_tls_peer_result_string(const isc_nmhandle_t *handle); void isc__nm_async_httpsend(isc__networker_t *worker, isc__netievent_t *ev0); -void -isc__nm_async_httpstop(isc__networker_t *worker, isc__netievent_t *ev0); - void isc__nm_async_httpclose(isc__networker_t *worker, isc__netievent_t *ev0); @@ -1923,6 +1922,9 @@ isc__nm_acquire_interlocked_force(isc_nm_t *mgr); * Actively wait for interlocked state. */ +void +isc__nm_async_sockstop(isc__networker_t *worker, isc__netievent_t *ev0); + void isc__nm_incstats(isc_nmsocket_t *sock, isc__nm_statid_t id); /*%< @@ -2015,6 +2017,27 @@ isc__nm_set_network_buffers(isc_nm_t *nm, uv_handle_t *handle); * Sets the pre-configured network buffers size on the handle. */ +void +isc__nmsocket_barrier_init(isc_nmsocket_t *listener); +/*%> + * Initialise the socket synchronisation barrier according to the + * number of children. + */ + +void +isc__nmsocket_stop(isc_nmsocket_t *listener); +/*%> + * Broadcast "stop" event for a listener socket across all workers and + * wait its processing completion - then, stop and close the underlying + * transport listener socket. + * + * The primitive is used in multi-layer transport listener sockets to + * implement shutdown properly: after the broadcasted events has been + * processed it is safe to destroy the shared data within the listener + * socket (including shutting down the underlying transport listener + * socket). + */ + /* * typedef all the netievent types */ @@ -2057,7 +2080,6 @@ NETIEVENT_SOCKET_QUOTA_TYPE(tlsdnsaccept); NETIEVENT_SOCKET_TYPE(tlsdnscycle); #ifdef HAVE_LIBNGHTTP2 -NETIEVENT_SOCKET_TYPE(httpstop); NETIEVENT_SOCKET_REQ_TYPE(httpsend); NETIEVENT_SOCKET_TYPE(httpclose); NETIEVENT_SOCKET_HTTP_EPS_TYPE(httpendpoints); @@ -2090,6 +2112,7 @@ NETIEVENT_TASK_TYPE(task); NETIEVENT_TASK_TYPE(privilegedtask); NETIEVENT_SOCKET_TLSCTX_TYPE(settlsctx); +NETIEVENT_SOCKET_TYPE(sockstop); /* Now declared the helper functions */ @@ -2131,7 +2154,6 @@ NETIEVENT_SOCKET_QUOTA_DECL(tlsdnsaccept); NETIEVENT_SOCKET_DECL(tlsdnscycle); #ifdef HAVE_LIBNGHTTP2 -NETIEVENT_SOCKET_DECL(httpstop); NETIEVENT_SOCKET_REQ_DECL(httpsend); NETIEVENT_SOCKET_DECL(httpclose); NETIEVENT_SOCKET_HTTP_EPS_DECL(httpendpoints); @@ -2163,6 +2185,7 @@ NETIEVENT_TASK_DECL(task); NETIEVENT_TASK_DECL(privilegedtask); NETIEVENT_SOCKET_TLSCTX_DECL(settlsctx); +NETIEVENT_SOCKET_DECL(sockstop); void isc__nm_udp_failed_read_cb(isc_nmsocket_t *sock, isc_result_t result); diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index 57d5390c68..f2be97f6a3 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -964,12 +964,12 @@ process_netievent(isc__networker_t *worker, isc__netievent_t *ievent) { NETIEVENT_CASE(tlsdobio); NETIEVENT_CASE(tlscancel); - NETIEVENT_CASE(httpstop); NETIEVENT_CASE(httpsend); NETIEVENT_CASE(httpclose); NETIEVENT_CASE(httpendpoints); #endif NETIEVENT_CASE(settlsctx); + NETIEVENT_CASE(sockstop); NETIEVENT_CASE(connectcb); NETIEVENT_CASE(readcb); @@ -1082,7 +1082,6 @@ NETIEVENT_SOCKET_DEF(tlsdnscycle); NETIEVENT_SOCKET_DEF(tlsdnsshutdown); #ifdef HAVE_LIBNGHTTP2 -NETIEVENT_SOCKET_DEF(httpstop); NETIEVENT_SOCKET_REQ_DEF(httpsend); NETIEVENT_SOCKET_DEF(httpclose); NETIEVENT_SOCKET_HTTP_EPS_DEF(httpendpoints); @@ -1113,6 +1112,7 @@ NETIEVENT_TASK_DEF(task); NETIEVENT_TASK_DEF(privilegedtask); NETIEVENT_SOCKET_TLSCTX_DEF(settlsctx); +NETIEVENT_SOCKET_DEF(sockstop); void isc__nm_maybe_enqueue_ievent(isc__networker_t *worker, @@ -1302,6 +1302,11 @@ nmsocket_cleanup(isc_nmsocket_t *sock, bool dofree FLARG) { #endif INSIST(ISC_LIST_EMPTY(sock->tls.sendreqs)); + + if (sock->barrier_initialised) { + isc_barrier_destroy(&sock->barrier); + } + #ifdef NETMGR_TRACE LOCK(&sock->mgr->lock); ISC_LIST_UNLINK(sock->mgr->active_sockets, sock, active_link); @@ -2727,6 +2732,76 @@ isc_nm_stoplistening(isc_nmsocket_t *sock) { } } +void +isc__nmsocket_stop(isc_nmsocket_t *listener) { + isc__netievent_sockstop_t ievent = { .sock = listener }; + + REQUIRE(VALID_NMSOCK(listener)); + + if (!atomic_compare_exchange_strong(&listener->closing, + &(bool){ false }, true)) { + UNREACHABLE(); + } + + for (size_t i = 0; i < listener->nchildren; i++) { + isc__networker_t *worker = &listener->mgr->workers[i]; + isc__netievent_sockstop_t *ev; + + if (isc__nm_in_netthread() && i == (size_t)isc_nm_tid()) { + continue; + } + + ev = isc__nm_get_netievent_sockstop(listener->mgr, listener); + isc__nm_enqueue_ievent(worker, (isc__netievent_t *)ev); + } + + if (isc__nm_in_netthread()) { + isc__nm_async_sockstop(&listener->mgr->workers[0], + (isc__netievent_t *)&ievent); + } +} + +void +isc__nmsocket_barrier_init(isc_nmsocket_t *listener) { + REQUIRE(listener->nchildren > 0); + isc_barrier_init(&listener->barrier, listener->nchildren); + listener->barrier_initialised = true; +} + +void +isc__nm_async_sockstop(isc__networker_t *worker, isc__netievent_t *ev0) { + isc__netievent_sockstop_t *ievent = (isc__netievent_sockstop_t *)ev0; + isc_nmsocket_t *listener = ievent->sock; + UNUSED(worker); + + (void)atomic_fetch_sub(&listener->rchildren, 1); + isc_barrier_wait(&listener->barrier); + + if (listener->tid != isc_nm_tid()) { + return; + } + + if (!atomic_compare_exchange_strong(&listener->listening, + &(bool){ true }, false)) + { + UNREACHABLE(); + } + + INSIST(atomic_load(&listener->rchildren) == 0); + + listener->accept_cb = NULL; + listener->accept_cbarg = NULL; + listener->recv_cb = NULL; + listener->recv_cbarg = NULL; + + if (listener->outer != NULL) { + isc_nm_stoplistening(listener->outer); + isc__nmsocket_detach(&listener->outer); + } + + atomic_store(&listener->closed, true); +} + void isc__nm_connectcb(isc_nmsocket_t *sock, isc__nm_uvreq_t *uvreq, isc_result_t eresult, bool async) { diff --git a/lib/isc/netmgr/tlsstream.c b/lib/isc/netmgr/tlsstream.c index 9cb889f8fe..4746c6bb2f 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -674,6 +674,13 @@ tlslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { REQUIRE(VALID_NMSOCK(tlslistensock)); REQUIRE(tlslistensock->type == isc_nm_tlslistener); + if (isc__nmsocket_closing(handle->sock) || + isc__nmsocket_closing(tlslistensock) || + !atomic_load(&tlslistensock->listening)) + { + return (ISC_R_CANCELED); + } + /* * We need to create a 'wrapper' tlssocket for this connection. */ @@ -760,6 +767,10 @@ isc_nm_listentls(isc_nm_t *mgr, isc_sockaddr_t *iface, isc__nmsocket_attach(tlssock, &tlssock->outer->tlsstream.tlslistener); isc__nmsocket_detach(&tsock); INSIST(result != ISC_R_UNSET); + tlssock->nchildren = tlssock->outer->nchildren; + + isc__nmsocket_barrier_init(tlssock); + atomic_init(&tlssock->rchildren, tlssock->nchildren); if (result == ISC_R_SUCCESS) { atomic_store(&tlssock->listening, true); @@ -954,23 +965,7 @@ isc__nm_tls_stoplistening(isc_nmsocket_t *sock) { REQUIRE(VALID_NMSOCK(sock)); REQUIRE(sock->type == isc_nm_tlslistener); - if (!atomic_compare_exchange_strong(&sock->closing, &(bool){ false }, - true)) { - UNREACHABLE(); - } - - atomic_store(&sock->listening, false); - atomic_store(&sock->closed, true); - sock->recv_cb = NULL; - sock->recv_cbarg = NULL; - - INSIST(sock->tlsstream.tls == NULL); - INSIST(sock->tlsstream.ctx == NULL); - - if (sock->outer != NULL) { - isc_nm_stoplistening(sock->outer); - isc__nmsocket_detach(&sock->outer); - } + isc__nmsocket_stop(sock); } static void From a6f14565b47fee310b1fadc0a5603d8849566187 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Tue, 18 Oct 2022 14:42:10 +0300 Subject: [PATCH 2/2] TLS Stream: handle successful TLS handshake after listener shutdown It was possible that accept callback can be called after listener shutdown. In such a case the callback pointer equals NULL, leading to segmentation fault. This commit fixes that. --- lib/isc/netmgr/tlsstream.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/isc/netmgr/tlsstream.c b/lib/isc/netmgr/tlsstream.c index 4746c6bb2f..b09dd91838 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -342,9 +342,13 @@ tls_try_handshake(isc_nmsocket_t *sock, isc_result_t *presult) { isc__nmsocket_log_tls_session_reuse(sock, sock->tlsstream.tls); tlshandle = isc__nmhandle_get(sock, &sock->peer, &sock->iface); if (sock->tlsstream.server) { - result = sock->listener->accept_cb( - tlshandle, result, - sock->listener->accept_cbarg); + if (isc__nmsocket_closing(sock->listener)) { + result = ISC_R_CANCELED; + } else { + result = sock->listener->accept_cb( + tlshandle, result, + sock->listener->accept_cbarg); + } } else { tls_call_connect_cb(sock, tlshandle, result); }