From 4ca64c1799e8a382404c7002c38122dd282d1ab9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 25 Jul 2023 10:30:09 +0200 Subject: [PATCH] Pin dns_request to the associated loop When dns_request was canceled via dns_requestmgr_shutdown() the cancel event would be propagated on different loop (loop 0) than the loop where request was created on. In turn this would propagate down to isc_netmgr where we require all the events to be called from the matching isc_loop. Pin the dns_requests to the loops and ensure that all the events are called on the associated loop. This in turn allows us to remove the hashed locks on the requests and change the single .requests list to be a per-loop list for the request accounting. Additionally, do some extra cleanup because some race condititions are now not possible as all events on the dns_request are serialized. --- bin/delv/delv.c | 3 +- bin/nsupdate/nsupdate.c | 2 +- bin/tests/system/pipelined/pipequeries.c | 43 +- bin/tools/mdig.c | 40 +- lib/dns/include/dns/request.h | 59 +- lib/dns/request.c | 673 ++++++++++------------- lib/dns/view.c | 2 +- lib/dns/zone.c | 24 +- lib/isc/loop.c | 3 + 9 files changed, 412 insertions(+), 437 deletions(-) diff --git a/bin/delv/delv.c b/bin/delv/delv.c index d045bfaa8c..2e6f82c7e5 100644 --- a/bin/delv/delv.c +++ b/bin/delv/delv.c @@ -1927,6 +1927,7 @@ cleanup: static void shutdown_server(void) { if (requestmgr != NULL) { + dns_requestmgr_shutdown(requestmgr); dns_requestmgr_detach(&requestmgr); } if (interfacemgr != NULL) { @@ -2095,7 +2096,7 @@ sendquery(void *arg) { NULL, 0)); CHECK(dns_message_setopt(message, opt)); - CHECK(dns_requestmgr_create(mctx, dispatchmgr, NULL, NULL, + CHECK(dns_requestmgr_create(mctx, loopmgr, dispatchmgr, NULL, NULL, &requestmgr)); dns_view_attach(view, &(dns_view_t *){ NULL }); diff --git a/bin/nsupdate/nsupdate.c b/bin/nsupdate/nsupdate.c index bedab93704..d90473b039 100644 --- a/bin/nsupdate/nsupdate.c +++ b/bin/nsupdate/nsupdate.c @@ -967,7 +967,7 @@ setup_system(void) { dns_transport_set_always_verify_remote(transport, tls_always_verify_remote); - result = dns_requestmgr_create(gmctx, dispatchmgr, dispatchv4, + result = dns_requestmgr_create(gmctx, loopmgr, dispatchmgr, dispatchv4, dispatchv6, &requestmgr); check_result(result, "dns_requestmgr_create"); diff --git a/bin/tests/system/pipelined/pipequeries.c b/bin/tests/system/pipelined/pipequeries.c index 805e3c5344..b242118fc2 100644 --- a/bin/tests/system/pipelined/pipequeries.c +++ b/bin/tests/system/pipelined/pipequeries.c @@ -183,6 +183,32 @@ sendqueries(void *arg) { return; } +static void +teardown_view(void *arg) { + dns_view_t *view = arg; + dns_view_detach(&view); +} + +static void +teardown_requestmgr(void *arg) { + dns_requestmgr_t *mgr = arg; + + dns_requestmgr_shutdown(mgr); + dns_requestmgr_detach(&mgr); +} + +static void +teardown_dispatchv4(void *arg) { + dns_dispatch_t *dispatchv4 = arg; + dns_dispatch_detach(&dispatchv4); +} + +static void +teardown_dispatchmgr(void *arg) { + dns_dispatchmgr_t *dispatchmgr = arg; + dns_dispatchmgr_detach(&dispatchmgr); +} + int main(int argc, char *argv[]) { isc_sockaddr_t bind_any; @@ -253,22 +279,19 @@ main(int argc, char *argv[]) { RUNCHECK(dns_dispatch_createudp( dispatchmgr, have_src ? &srcaddr : &bind_any, &dispatchv4)); - RUNCHECK(dns_requestmgr_create(mctx, dispatchmgr, dispatchv4, NULL, - &requestmgr)); + RUNCHECK(dns_requestmgr_create(mctx, loopmgr, dispatchmgr, dispatchv4, + NULL, &requestmgr)); RUNCHECK(dns_view_create(mctx, 0, "_test", &view)); isc_loopmgr_setup(loopmgr, sendqueries, NULL); + isc_loopmgr_teardown(loopmgr, teardown_view, view); + isc_loopmgr_teardown(loopmgr, teardown_requestmgr, requestmgr); + isc_loopmgr_teardown(loopmgr, teardown_dispatchv4, dispatchv4); + isc_loopmgr_teardown(loopmgr, teardown_dispatchmgr, dispatchmgr); + isc_loopmgr_run(loopmgr); - dns_view_detach(&view); - - dns_requestmgr_shutdown(requestmgr); - dns_requestmgr_detach(&requestmgr); - - dns_dispatch_detach(&dispatchv4); - dns_dispatchmgr_detach(&dispatchmgr); - dst_lib_destroy(); isc_log_destroy(&lctx); diff --git a/bin/tools/mdig.c b/bin/tools/mdig.c index 7a366c0e85..24ac710a56 100644 --- a/bin/tools/mdig.c +++ b/bin/tools/mdig.c @@ -2068,6 +2068,32 @@ set_source_ports(dns_dispatchmgr_t *manager) { isc_portset_destroy(mctx, &v6portset); } +static void +teardown_view(void *arg) { + dns_view_t *view = arg; + dns_view_detach(&view); +} + +static void +teardown_requestmgr(void *arg) { + dns_requestmgr_t *mgr = arg; + + dns_requestmgr_shutdown(mgr); + dns_requestmgr_detach(&mgr); +} + +static void +teardown_dispatch(void *arg) { + dns_dispatch_t *dispatchv4 = arg; + dns_dispatch_detach(&dispatchv4); +} + +static void +teardown_dispatchmgr(void *arg) { + dns_dispatchmgr_t *dispatchmgr = arg; + dns_dispatchmgr_detach(&dispatchmgr); +} + /*% Main processing routine for mdig */ int main(int argc, char *argv[]) { @@ -2138,13 +2164,17 @@ main(int argc, char *argv[]) { dispatchmgr, have_src ? &srcaddr : &bind_any, &dispatchvx)); RUNCHECK(dns_requestmgr_create( - mctx, dispatchmgr, have_ipv4 ? dispatchvx : NULL, + mctx, loopmgr, dispatchmgr, have_ipv4 ? dispatchvx : NULL, have_ipv6 ? dispatchvx : NULL, &requestmgr)); RUNCHECK(dns_view_create(mctx, 0, "_mdig", &view)); query = ISC_LIST_HEAD(queries); isc_loopmgr_setup(loopmgr, sendqueries, query); + isc_loopmgr_teardown(loopmgr, teardown_view, view); + isc_loopmgr_teardown(loopmgr, teardown_requestmgr, requestmgr); + isc_loopmgr_teardown(loopmgr, teardown_dispatch, dispatchvx); + isc_loopmgr_teardown(loopmgr, teardown_dispatchmgr, dispatchmgr); /* * Stall to the start of a new second. @@ -2174,14 +2204,6 @@ main(int argc, char *argv[]) { isc_loopmgr_run(loopmgr); - dns_view_detach(&view); - - dns_requestmgr_shutdown(requestmgr); - dns_requestmgr_detach(&requestmgr); - - dns_dispatch_detach(&dispatchvx); - dns_dispatchmgr_detach(&dispatchmgr); - dst_lib_destroy(); isc_log_destroy(&lctx); diff --git a/lib/dns/include/dns/request.h b/lib/dns/include/dns/request.h index e33119742b..5e112cb437 100644 --- a/lib/dns/include/dns/request.h +++ b/lib/dns/include/dns/request.h @@ -36,10 +36,14 @@ #include +#include #include +#include #include +#undef DNS_REQUEST_TRACE + #define DNS_REQUESTOPT_TCP 0x00000001U #define DNS_REQUESTOPT_CASE 0x00000002U #define DNS_REQUESTOPT_FIXEDID 0x00000004U @@ -48,7 +52,8 @@ ISC_LANG_BEGINDECLS isc_result_t -dns_requestmgr_create(isc_mem_t *mctx, dns_dispatchmgr_t *dispatchmgr, +dns_requestmgr_create(isc_mem_t *mctx, isc_loopmgr_t *loopmgr, + dns_dispatchmgr_t *dispatchmgr, dns_dispatch_t *dispatchv4, dns_dispatch_t *dispatchv6, dns_requestmgr_t **requestmgrp); /*%< @@ -90,33 +95,19 @@ dns_requestmgr_shutdown(dns_requestmgr_t *requestmgr); *\li 'requestmgr' is a valid requestmgr. */ -void -dns_requestmgr_attach(dns_requestmgr_t *source, dns_requestmgr_t **targetp); -/*%< - * Attach to the request manager. dns_requestmgr_shutdown() must not - * have been called on 'source' prior to calling dns_requestmgr_attach(). - * - * Requires: - * - *\li 'source' is a valid requestmgr. - * - *\li 'targetp' to be non NULL and '*targetp' to be NULL. - */ - -void -dns_requestmgr_detach(dns_requestmgr_t **requestmgrp); -/*%< - * Detach from the given requestmgr. If this is the final detach - * requestmgr will be destroyed. dns_requestmgr_shutdown() must - * be called before the final detach. - * - * Requires: - * - *\li '*requestmgrp' is a valid requestmgr. - * - * Ensures: - *\li '*requestmgrp' is NULL. - */ +#if DNS_REQUEST_TRACE +#define dns_requestmgr_ref(ptr) \ + dns_requestmgr__ref(ptr, __func__, __FILE__, __LINE__) +#define dns_requestmgr_unref(ptr) \ + dns_requestmgr__unref(ptr, __func__, __FILE__, __LINE__) +#define dns_requestmgr_attach(ptr, ptrp) \ + dns_requestmgr__attach(ptr, ptrp, __func__, __FILE__, __LINE__) +#define dns_requestmgr_detach(ptrp) \ + dns_requestmgr__detach(ptrp, __func__, __FILE__, __LINE__) +ISC_REFCOUNT_TRACE_DECL(dns_requestmgr); +#else +ISC_REFCOUNT_DECL(dns_requestmgr); +#endif isc_result_t dns_request_create(dns_requestmgr_t *requestmgr, dns_message_t *message, @@ -302,5 +293,17 @@ dns_request_getresult(dns_request_t *request); * completion handler.) */ +#if DNS_REQUEST_TRACE +#define dns_request_ref(ptr) dns_request__ref(ptr, __func__, __FILE__, __LINE__) +#define dns_request_unref(ptr) \ + dns_request__unref(ptr, __func__, __FILE__, __LINE__) +#define dns_request_attach(ptr, ptrp) \ + dns_request__attach(ptr, ptrp, __func__, __FILE__, __LINE__) +#define dns_request_detach(ptrp) \ + dns_request__detach(ptrp, __func__, __FILE__, __LINE__) +ISC_REFCOUNT_TRACE_DECL(dns_request); +#else ISC_REFCOUNT_DECL(dns_request); +#endif + ISC_LANG_ENDDECLS diff --git a/lib/dns/request.c b/lib/dns/request.c index f5d4ed25cf..f675fb495f 100644 --- a/lib/dns/request.c +++ b/lib/dns/request.c @@ -45,37 +45,33 @@ typedef ISC_LIST(dns_request_t) dns_requestlist_t; -#define DNS_REQUEST_NLOCKS 7 - struct dns_requestmgr { unsigned int magic; - isc_refcount_t references; - - isc_mutex_t lock; isc_mem_t *mctx; + isc_refcount_t references; + isc_loopmgr_t *loopmgr; + + atomic_bool shuttingdown; - /* locked */ dns_dispatchmgr_t *dispatchmgr; dns_dispatch_t *dispatchv4; dns_dispatch_t *dispatchv6; - atomic_bool exiting; - unsigned int hash; - isc_mutex_t locks[DNS_REQUEST_NLOCKS]; - dns_requestlist_t requests; + dns_requestlist_t *requests; }; struct dns_request { unsigned int magic; isc_refcount_t references; - unsigned int hash; isc_mem_t *mctx; int32_t flags; + isc_loop_t *loop; + unsigned int tid; + isc_result_t result; isc_job_cb cb; void *arg; - bool complete; ISC_LINK(dns_request_t) link; isc_buffer_t *query; isc_buffer_t *answer; @@ -89,23 +85,19 @@ struct dns_request { unsigned int udpcount; }; -#define DNS_REQUEST_F_CONNECTING 0x0001 -#define DNS_REQUEST_F_SENDING 0x0002 -#define DNS_REQUEST_F_CANCELED 0x0004 -#define DNS_REQUEST_F_TCP 0x0010 +#define DNS_REQUEST_F_CONNECTING (1 << 0) +#define DNS_REQUEST_F_SENDING (1 << 1) +#define DNS_REQUEST_F_COMPLETE (1 << 2) +#define DNS_REQUEST_F_TCP (1 << 3) -#define DNS_REQUEST_CANCELED(r) (((r)->flags & DNS_REQUEST_F_CANCELED) != 0) #define DNS_REQUEST_CONNECTING(r) (((r)->flags & DNS_REQUEST_F_CONNECTING) != 0) #define DNS_REQUEST_SENDING(r) (((r)->flags & DNS_REQUEST_F_SENDING) != 0) +#define DNS_REQUEST_COMPLETE(r) (((r)->flags & DNS_REQUEST_F_COMPLETE) != 0) /*** *** Forward ***/ -static void -mgr_destroy(dns_requestmgr_t *requestmgr); -static unsigned int -mgr_gethash(dns_requestmgr_t *requestmgr); static isc_result_t req_render(dns_message_t *message, isc_buffer_t **buffer, unsigned int options, isc_mem_t *mctx); @@ -114,6 +106,8 @@ req_response(isc_result_t result, isc_region_t *region, void *arg); static void req_senddone(isc_result_t eresult, isc_region_t *region, void *arg); static void +req_cleanup(dns_request_t *request); +static void req_sendevent(dns_request_t *request, isc_result_t result); static void req_connected(isc_result_t eresult, isc_region_t *region, void *arg); @@ -121,131 +115,124 @@ static void req_destroy(dns_request_t *request); static void req_log(int level, const char *fmt, ...) ISC_FORMAT_PRINTF(2, 3); -void -request_cancel(dns_request_t *request); /*** *** Public ***/ isc_result_t -dns_requestmgr_create(isc_mem_t *mctx, dns_dispatchmgr_t *dispatchmgr, +dns_requestmgr_create(isc_mem_t *mctx, isc_loopmgr_t *loopmgr, + dns_dispatchmgr_t *dispatchmgr, dns_dispatch_t *dispatchv4, dns_dispatch_t *dispatchv6, dns_requestmgr_t **requestmgrp) { - dns_requestmgr_t *requestmgr; - int i; - - req_log(ISC_LOG_DEBUG(3), "dns_requestmgr_create"); - REQUIRE(requestmgrp != NULL && *requestmgrp == NULL); REQUIRE(dispatchmgr != NULL); - requestmgr = isc_mem_get(mctx, sizeof(*requestmgr)); - *requestmgr = (dns_requestmgr_t){ 0 }; + req_log(ISC_LOG_DEBUG(3), "%s", __func__); + + dns_requestmgr_t *requestmgr = isc_mem_get(mctx, sizeof(*requestmgr)); + *requestmgr = (dns_requestmgr_t){ + .magic = REQUESTMGR_MAGIC, + .loopmgr = loopmgr, + }; + isc_mem_attach(mctx, &requestmgr->mctx); + + uint32_t nloops = isc_loopmgr_nloops(requestmgr->loopmgr); + requestmgr->requests = isc_mem_getx( + requestmgr->mctx, nloops * sizeof(requestmgr->requests[0]), + ISC_MEM_ZERO); + for (size_t i = 0; i < nloops; i++) { + ISC_LIST_INIT(requestmgr->requests[i]); + + /* unreferenced in requests_shutdown() */ + isc_loop_ref(isc_loop_get(requestmgr->loopmgr, i)); + } dns_dispatchmgr_attach(dispatchmgr, &requestmgr->dispatchmgr); - isc_mutex_init(&requestmgr->lock); - for (i = 0; i < DNS_REQUEST_NLOCKS; i++) { - isc_mutex_init(&requestmgr->locks[i]); - } if (dispatchv4 != NULL) { dns_dispatch_attach(dispatchv4, &requestmgr->dispatchv4); } if (dispatchv6 != NULL) { dns_dispatch_attach(dispatchv6, &requestmgr->dispatchv6); } - isc_mem_attach(mctx, &requestmgr->mctx); isc_refcount_init(&requestmgr->references, 1); - ISC_LIST_INIT(requestmgr->requests); - - atomic_init(&requestmgr->exiting, false); - - requestmgr->magic = REQUESTMGR_MAGIC; - - req_log(ISC_LOG_DEBUG(3), "dns_requestmgr_create: %p", requestmgr); + req_log(ISC_LOG_DEBUG(3), "%s: %p", __func__, requestmgr); *requestmgrp = requestmgr; return (ISC_R_SUCCESS); } +static void +requests_shutdown(void *arg) { + dns_requestmgr_t *requestmgr = arg; + dns_request_t *request = NULL, *next = NULL; + uint32_t tid = isc_tid(); + + ISC_LIST_FOREACH_SAFE (requestmgr->requests[tid], request, link, next) { + req_log(ISC_LOG_DEBUG(3), "%s(%" PRIu32 ": request %p", + __func__, tid, request); + if (DNS_REQUEST_COMPLETE(request)) { + /* The callback has been already scheduled */ + continue; + } + req_sendevent(request, ISC_R_SHUTTINGDOWN); + } + + isc_loop_unref(isc_loop_get(requestmgr->loopmgr, tid)); + dns_requestmgr_detach(&requestmgr); +} + void dns_requestmgr_shutdown(dns_requestmgr_t *requestmgr) { - dns_request_t *request; - REQUIRE(VALID_REQUESTMGR(requestmgr)); - req_log(ISC_LOG_DEBUG(3), "dns_requestmgr_shutdown: %p", requestmgr); + req_log(ISC_LOG_DEBUG(3), "%s: %p", __func__, requestmgr); - if (!atomic_compare_exchange_strong(&requestmgr->exiting, - &(bool){ false }, true)) - { - return; - } + rcu_read_lock(); + INSIST(atomic_compare_exchange_strong(&requestmgr->shuttingdown, + &(bool){ false }, true)); + rcu_read_unlock(); - LOCK(&requestmgr->lock); - for (request = ISC_LIST_HEAD(requestmgr->requests); request != NULL; - request = ISC_LIST_NEXT(request, link)) - { - dns_request_cancel(request); - } - UNLOCK(&requestmgr->lock); -} + /* + * Wait until all dns_request_create{raw}() are finished, so + * there will be no new requests added to the lists. + */ + synchronize_rcu(); -void -dns_requestmgr_attach(dns_requestmgr_t *source, dns_requestmgr_t **targetp) { - uint_fast32_t ref; + uint32_t tid = isc_tid(); + uint32_t nloops = isc_loopmgr_nloops(requestmgr->loopmgr); + for (size_t i = 0; i < nloops; i++) { + dns_requestmgr_ref(requestmgr); - REQUIRE(VALID_REQUESTMGR(source)); - REQUIRE(targetp != NULL && *targetp == NULL); + if (i == tid) { + /* Run the current loop synchronously */ + requests_shutdown(requestmgr); + continue; + } - REQUIRE(!atomic_load_acquire(&source->exiting)); - - ref = isc_refcount_increment(&source->references); - - req_log(ISC_LOG_DEBUG(3), - "dns_requestmgr_attach: %p: references = %" PRIuFAST32, source, - ref + 1); - - *targetp = source; -} - -void -dns_requestmgr_detach(dns_requestmgr_t **requestmgrp) { - dns_requestmgr_t *requestmgr = NULL; - uint_fast32_t ref; - - REQUIRE(requestmgrp != NULL && VALID_REQUESTMGR(*requestmgrp)); - - requestmgr = *requestmgrp; - *requestmgrp = NULL; - - ref = isc_refcount_decrement(&requestmgr->references); - - req_log(ISC_LOG_DEBUG(3), - "dns_requestmgr_detach: %p: references = %" PRIuFAST32, - requestmgr, ref - 1); - - if (ref == 1) { - INSIST(ISC_LIST_EMPTY(requestmgr->requests)); - mgr_destroy(requestmgr); + isc_loop_t *loop = isc_loop_get(requestmgr->loopmgr, i); + isc_async_run(loop, requests_shutdown, requestmgr); } } static void -mgr_destroy(dns_requestmgr_t *requestmgr) { - int i; +requestmgr_destroy(dns_requestmgr_t *requestmgr) { + req_log(ISC_LOG_DEBUG(3), "%s", __func__); - req_log(ISC_LOG_DEBUG(3), "mgr_destroy"); + INSIST(atomic_load(&requestmgr->shuttingdown)); isc_refcount_destroy(&requestmgr->references); - isc_mutex_destroy(&requestmgr->lock); - for (i = 0; i < DNS_REQUEST_NLOCKS; i++) { - isc_mutex_destroy(&requestmgr->locks[i]); + size_t nloops = isc_loopmgr_nloops(requestmgr->loopmgr); + for (size_t i = 0; i < nloops; i++) { + INSIST(ISC_LIST_EMPTY(requestmgr->requests[i])); } + isc_mem_put(requestmgr->mctx, requestmgr->requests, + nloops * sizeof(requestmgr->requests[0])); + if (requestmgr->dispatchv4 != NULL) { dns_dispatch_detach(&requestmgr->dispatchv4); } @@ -260,21 +247,17 @@ mgr_destroy(dns_requestmgr_t *requestmgr) { sizeof(*requestmgr)); } -static unsigned int -mgr_gethash(dns_requestmgr_t *requestmgr) { - req_log(ISC_LOG_DEBUG(3), "mgr_gethash"); - /* - * Locked by caller. - */ - requestmgr->hash++; - return (requestmgr->hash % DNS_REQUEST_NLOCKS); -} +#if DNS_REQUEST_TRACE +ISC_REFCOUNT_TRACE_IMPL(dns_requestmgr, requestmgr_destroy); +#else +ISC_REFCOUNT_IMPL(dns_requestmgr, requestmgr_destroy); +#endif static void req_send(dns_request_t *request) { isc_region_t r; - req_log(ISC_LOG_DEBUG(3), "req_send: request %p", request); + req_log(ISC_LOG_DEBUG(3), "%s: request %p", __func__, request); REQUIRE(VALID_REQUEST(request)); @@ -287,20 +270,38 @@ req_send(dns_request_t *request) { dns_dispatch_send(request->dispentry, &r); } -static isc_result_t -new_request(isc_mem_t *mctx, dns_request_t **requestp) { - dns_request_t *request = NULL; - - request = isc_mem_get(mctx, sizeof(*request)); - *request = (dns_request_t){ 0 }; - ISC_LINK_INIT(request, link); +static dns_request_t * +new_request(isc_mem_t *mctx, isc_loop_t *loop, isc_job_cb cb, void *arg, + bool tcp, unsigned int timeout, unsigned int udptimeout, + unsigned int udpretries) { + dns_request_t *request = isc_mem_get(mctx, sizeof(*request)); + *request = (dns_request_t){ + .magic = REQUEST_MAGIC, + .loop = loop, + .tid = isc_tid(), + .cb = cb, + .arg = arg, + .link = ISC_LINK_INITIALIZER, + .result = ISC_R_FAILURE, + .udpcount = udpretries + 1, + }; isc_refcount_init(&request->references, 1); isc_mem_attach(mctx, &request->mctx); - request->magic = REQUEST_MAGIC; - *requestp = request; - return (ISC_R_SUCCESS); + if (tcp) { + request->timeout = timeout * 1000; + } else { + if (udptimeout == 0) { + udptimeout = timeout / request->udpcount; + } + if (udptimeout == 0) { + udptimeout = 1; + } + request->timeout = udptimeout * 1000; + } + + return (request); } static bool @@ -429,62 +430,44 @@ dns_request_createraw(dns_requestmgr_t *requestmgr, isc_buffer_t *msgbuf, mctx = requestmgr->mctx; - req_log(ISC_LOG_DEBUG(3), "dns_request_createraw"); + req_log(ISC_LOG_DEBUG(3), "%s", __func__); - if (atomic_load_acquire(&requestmgr->exiting)) { - return (ISC_R_SHUTTINGDOWN); + rcu_read_lock(); + + if (atomic_load_acquire(&requestmgr->shuttingdown)) { + result = ISC_R_SHUTTINGDOWN; + goto done; } if (isblackholed(requestmgr->dispatchmgr, destaddr)) { - return (DNS_R_BLACKHOLED); + result = DNS_R_BLACKHOLED; + goto done; } - /* detached in dns_request_destroy() */ - result = new_request(mctx, &request); - if (result != ISC_R_SUCCESS) { - return (result); - } - - request->loop = loop; - request->cb = cb; - request->arg = arg; - request->result = ISC_R_FAILURE; - request->udpcount = udpretries + 1; - isc_buffer_usedregion(msgbuf, &r); if (r.length < DNS_MESSAGE_HEADERLEN || r.length > 65535) { result = DNS_R_FORMERR; - goto cleanup; + goto done; } if ((options & DNS_REQUESTOPT_TCP) != 0 || r.length > 512) { tcp = true; - request->timeout = timeout * 1000; - } else { - if (udptimeout == 0) { - udptimeout = timeout / request->udpcount; - } - if (udptimeout == 0) { - udptimeout = 1; - } - request->timeout = udptimeout * 1000; } + request = new_request(mctx, loop, cb, arg, tcp, timeout, udptimeout, + udpretries); + isc_buffer_allocate(mctx, &request->query, r.length + (tcp ? 2 : 0)); result = isc_buffer_copyregion(request->query, &r); if (result != ISC_R_SUCCESS) { goto cleanup; } - /* detached in req_connected() */ - dns_request_ref(request); - again: - result = get_dispatch(tcp, newtcp, requestmgr, srcaddr, destaddr, &request->dispatch); if (result != ISC_R_SUCCESS) { - goto detach; + goto cleanup; } if ((options & DNS_REQUESTOPT_FIXEDID) != 0) { @@ -498,12 +481,12 @@ again: req_response, request, &id, &request->dispentry); if (result != ISC_R_SUCCESS) { if ((options & DNS_REQUESTOPT_FIXEDID) != 0 && !newtcp) { - newtcp = true; dns_dispatch_detach(&request->dispatch); + newtcp = true; goto again; } - goto detach; + goto cleanup; } /* Add message ID. */ @@ -511,41 +494,35 @@ again: r.base[0] = (id >> 8) & 0xff; r.base[1] = id & 0xff; - LOCK(&requestmgr->lock); - dns_requestmgr_attach(requestmgr, &request->requestmgr); - request->hash = mgr_gethash(requestmgr); - ISC_LIST_APPEND(requestmgr->requests, request, link); - UNLOCK(&requestmgr->lock); - request->destaddr = *destaddr; - request->flags |= DNS_REQUEST_F_CONNECTING; if (tcp) { request->flags |= DNS_REQUEST_F_TCP; } + dns_requestmgr_attach(requestmgr, &request->requestmgr); + ISC_LIST_APPEND(requestmgr->requests[request->tid], request, link); + + dns_request_ref(request); /* detached in req_connected() */ result = dns_dispatch_connect(request->dispentry); if (result != ISC_R_SUCCESS) { - goto unlink; + dns_request_unref(request); + goto cleanup; } - req_log(ISC_LOG_DEBUG(3), "dns_request_createraw: request %p", request); + req_log(ISC_LOG_DEBUG(3), "%s: request %p", __func__, request); *requestp = request; - return (ISC_R_SUCCESS); - -unlink: - LOCK(&requestmgr->lock); - ISC_LIST_UNLINK(requestmgr->requests, request, link); - UNLOCK(&requestmgr->lock); - -detach: - /* connect failed, unref here */ - dns_request_unref(request); cleanup: - dns_request_detach(&request); - req_log(ISC_LOG_DEBUG(3), "dns_request_createraw: failed %s", - isc_result_totext(result)); + if (result != ISC_R_SUCCESS) { + req_cleanup(request); + dns_request_detach(&request); + req_log(ISC_LOG_DEBUG(3), "%s: failed %s", __func__, + isc_result_totext(result)); + } + +done: + rcu_read_unlock(); return (result); } @@ -573,35 +550,34 @@ dns_request_create(dns_requestmgr_t *requestmgr, dns_message_t *message, REQUIRE(timeout > 0); REQUIRE(udpretries != UINT_MAX); - mctx = requestmgr->mctx; - - req_log(ISC_LOG_DEBUG(3), "dns_request_create"); - - if (atomic_load_acquire(&requestmgr->exiting)) { - return (ISC_R_SHUTTINGDOWN); - } - if (srcaddr != NULL && isc_sockaddr_pf(srcaddr) != isc_sockaddr_pf(destaddr)) { return (ISC_R_FAMILYMISMATCH); } + mctx = requestmgr->mctx; + + req_log(ISC_LOG_DEBUG(3), "%s", __func__); + + rcu_read_lock(); + + if (atomic_load_acquire(&requestmgr->shuttingdown)) { + result = ISC_R_SHUTTINGDOWN; + goto done; + } + if (isblackholed(requestmgr->dispatchmgr, destaddr)) { - return (DNS_R_BLACKHOLED); + result = DNS_R_BLACKHOLED; + goto done; } - /* detached in dns_request_destroy() */ - result = new_request(mctx, &request); - if (result != ISC_R_SUCCESS) { - return (result); + if ((options & DNS_REQUESTOPT_TCP) != 0) { + tcp = true; } - request->loop = loop; - request->cb = cb; - request->arg = arg; - request->result = ISC_R_FAILURE; - request->udpcount = udpretries + 1; + request = new_request(mctx, loop, cb, arg, tcp, timeout, udptimeout, + udpretries); if (key != NULL) { dns_tsigkey_attach(key, &request->tsigkey); @@ -612,27 +588,11 @@ dns_request_create(dns_requestmgr_t *requestmgr, dns_message_t *message, goto cleanup; } - if ((options & DNS_REQUESTOPT_TCP) != 0) { - tcp = true; - request->timeout = timeout * 1000; - } else { - if (udptimeout == 0) { - udptimeout = timeout / request->udpcount; - } - if (udptimeout == 0) { - udptimeout = 1; - } - request->timeout = udptimeout * 1000; - } - - /* detached in req_connected() */ - dns_request_ref(request); - again: result = get_dispatch(tcp, false, requestmgr, srcaddr, destaddr, &request->dispatch); if (result != ISC_R_SUCCESS) { - goto detach; + goto cleanup; } result = dns_dispatch_add(request->dispatch, loop, 0, request->timeout, @@ -640,65 +600,57 @@ again: req_connected, req_senddone, req_response, request, &id, &request->dispentry); if (result != ISC_R_SUCCESS) { - goto detach; + goto cleanup; } message->id = id; result = req_render(message, &request->query, options, mctx); if (result == DNS_R_USETCP && !tcp) { - /* - * Try again using TCP. - */ + /* Try again using TCP. */ dns_message_renderreset(message); dns_dispatch_done(&request->dispentry); dns_dispatch_detach(&request->dispatch); options |= DNS_REQUESTOPT_TCP; tcp = true; goto again; - } - if (result != ISC_R_SUCCESS) { - goto detach; + } else if (result != ISC_R_SUCCESS) { + goto cleanup; } result = dns_message_getquerytsig(message, mctx, &request->tsig); if (result != ISC_R_SUCCESS) { - goto detach; + goto cleanup; } - LOCK(&requestmgr->lock); - dns_requestmgr_attach(requestmgr, &request->requestmgr); - request->hash = mgr_gethash(requestmgr); - ISC_LIST_APPEND(requestmgr->requests, request, link); - UNLOCK(&requestmgr->lock); - request->destaddr = *destaddr; request->flags |= DNS_REQUEST_F_CONNECTING; if (tcp) { request->flags |= DNS_REQUEST_F_TCP; } + dns_requestmgr_attach(requestmgr, &request->requestmgr); + ISC_LIST_APPEND(requestmgr->requests[request->tid], request, link); + + dns_request_ref(request); /* detached in req_connected() */ result = dns_dispatch_connect(request->dispentry); if (result != ISC_R_SUCCESS) { - goto unlink; + dns_request_unref(request); + goto cleanup; } - req_log(ISC_LOG_DEBUG(3), "dns_request_create: request %p", request); + req_log(ISC_LOG_DEBUG(3), "%s: request %p", __func__, request); *requestp = request; - return (ISC_R_SUCCESS); - -unlink: - LOCK(&requestmgr->lock); - ISC_LIST_UNLINK(requestmgr->requests, request, link); - UNLOCK(&requestmgr->lock); - -detach: - /* connect failed, unref here */ - dns_request_unref(request); cleanup: - dns_request_detach(&request); - req_log(ISC_LOG_DEBUG(3), "dns_request_create: failed %s", - isc_result_totext(result)); + if (result != ISC_R_SUCCESS) { + req_cleanup(request); + dns_request_detach(&request); + req_log(ISC_LOG_DEBUG(3), "%s: failed %s", __func__, + isc_result_totext(result)); + } +done: + rcu_read_unlock(); + return (result); } @@ -714,7 +666,7 @@ req_render(dns_message_t *message, isc_buffer_t **bufferp, unsigned int options, REQUIRE(bufferp != NULL && *bufferp == NULL); - req_log(ISC_LOG_DEBUG(3), "request_render"); + req_log(ISC_LOG_DEBUG(3), "%s", __func__); /* * Create buffer able to hold largest possible message. @@ -792,32 +744,18 @@ cleanup: return (result); } -void -request_cancel(dns_request_t *request) { - if (!DNS_REQUEST_CANCELED(request)) { - req_log(ISC_LOG_DEBUG(3), "request_cancel: request %p", - request); - - request->flags |= DNS_REQUEST_F_CANCELED; - request->flags &= ~DNS_REQUEST_F_CONNECTING; - - if (request->dispentry != NULL) { - dns_dispatch_done(&request->dispentry); - } - - dns_dispatch_detach(&request->dispatch); - } -} - void dns_request_cancel(dns_request_t *request) { REQUIRE(VALID_REQUEST(request)); + REQUIRE(request->tid == isc_tid()); - req_log(ISC_LOG_DEBUG(3), "dns_request_cancel: request %p", request); - LOCK(&request->requestmgr->locks[request->hash]); - request_cancel(request); - req_sendevent(request, ISC_R_CANCELED); - UNLOCK(&request->requestmgr->locks[request->hash]); + if (DNS_REQUEST_COMPLETE(request)) { + /* The request callback was already called */ + return; + } + + req_log(ISC_LOG_DEBUG(3), "%s: request %p", __func__, request); + req_sendevent(request, ISC_R_CANCELED); /* call asynchronously */ } isc_result_t @@ -826,10 +764,10 @@ dns_request_getresponse(dns_request_t *request, dns_message_t *message, isc_result_t result; REQUIRE(VALID_REQUEST(request)); + REQUIRE(request->tid == isc_tid()); REQUIRE(request->answer != NULL); - req_log(ISC_LOG_DEBUG(3), "dns_request_getresponse: request %p", - request); + req_log(ISC_LOG_DEBUG(3), "%s: request %p", __func__, request); dns_message_setquerytsig(message, request->tsig); result = dns_message_settsigkey(message, request->tsigkey); @@ -849,6 +787,7 @@ dns_request_getresponse(dns_request_t *request, dns_message_t *message, isc_buffer_t * dns_request_getanswer(dns_request_t *request) { REQUIRE(VALID_REQUEST(request)); + REQUIRE(request->tid == isc_tid()); return (request->answer); } @@ -856,40 +795,22 @@ dns_request_getanswer(dns_request_t *request) { bool dns_request_usedtcp(dns_request_t *request) { REQUIRE(VALID_REQUEST(request)); + REQUIRE(request->tid == isc_tid()); return ((request->flags & DNS_REQUEST_F_TCP) != 0); } void dns_request_destroy(dns_request_t **requestp) { - dns_request_t *request = NULL; - REQUIRE(requestp != NULL && VALID_REQUEST(*requestp)); - request = *requestp; + dns_request_t *request = *requestp; *requestp = NULL; - req_log(ISC_LOG_DEBUG(3), "dns_request_destroy: request %p", request); + req_log(ISC_LOG_DEBUG(3), "%s: request %p", __func__, request); - LOCK(&request->requestmgr->lock); - LOCK(&request->requestmgr->locks[request->hash]); - ISC_LIST_UNLINK(request->requestmgr->requests, request, link); - UNLOCK(&request->requestmgr->locks[request->hash]); - UNLOCK(&request->requestmgr->lock); - - /* - * These should have been cleaned up before the completion - * event was sent. - */ - INSIST(request->dispentry == NULL); - INSIST(request->dispatch == NULL); - - /* - * if we've called the completion handler, there's - * another ref to detach - */ - if (request->complete) { - dns_request_unref(request); + if (DNS_REQUEST_COMPLETE(request)) { + dns_request_cancel(request); } /* final detach to shut down request */ @@ -897,160 +818,168 @@ dns_request_destroy(dns_request_t **requestp) { } static void -req_connected(isc_result_t eresult, isc_region_t *region, void *arg) { +req_connected(isc_result_t eresult, isc_region_t *region ISC_ATTR_UNUSED, + void *arg) { dns_request_t *request = (dns_request_t *)arg; - UNUSED(region); + REQUIRE(VALID_REQUEST(request)); + REQUIRE(request->tid == isc_tid()); + REQUIRE(DNS_REQUEST_CONNECTING(request)); - req_log(ISC_LOG_DEBUG(3), "req_connected: request %p: %s", request, + req_log(ISC_LOG_DEBUG(3), "%s: request %p: %s", __func__, request, isc_result_totext(eresult)); - REQUIRE(VALID_REQUEST(request)); - REQUIRE(DNS_REQUEST_CONNECTING(request) || - DNS_REQUEST_CANCELED(request)); - - LOCK(&request->requestmgr->locks[request->hash]); request->flags &= ~DNS_REQUEST_F_CONNECTING; - if (eresult == ISC_R_TIMEDOUT) { - dns_dispatch_done(&request->dispentry); - dns_dispatch_detach(&request->dispatch); - req_sendevent(request, eresult); - } else if (DNS_REQUEST_CANCELED(request)) { - req_sendevent(request, ISC_R_CANCELED); - } else if (eresult == ISC_R_SUCCESS) { + if (DNS_REQUEST_COMPLETE(request)) { + /* The request callback was already called */ + goto detach; + } + + if (eresult == ISC_R_SUCCESS) { req_send(request); } else { - request_cancel(request); req_sendevent(request, eresult); } - UNLOCK(&request->requestmgr->locks[request->hash]); +detach: /* attached in dns_request_create/_createraw() */ dns_request_unref(request); } static void -req_senddone(isc_result_t eresult, isc_region_t *region, void *arg) { +req_senddone(isc_result_t eresult, isc_region_t *region ISC_ATTR_UNUSED, + void *arg) { dns_request_t *request = (dns_request_t *)arg; REQUIRE(VALID_REQUEST(request)); + REQUIRE(request->tid == isc_tid()); REQUIRE(DNS_REQUEST_SENDING(request)); - UNUSED(region); + req_log(ISC_LOG_DEBUG(3), "%s: request %p", __func__, request); - req_log(ISC_LOG_DEBUG(3), "req_senddone: request %p", request); - - LOCK(&request->requestmgr->locks[request->hash]); request->flags &= ~DNS_REQUEST_F_SENDING; - if (DNS_REQUEST_CANCELED(request)) { - if (eresult == ISC_R_TIMEDOUT) { - req_sendevent(request, eresult); - } else { - req_sendevent(request, ISC_R_CANCELED); - } - } else if (eresult != ISC_R_SUCCESS) { - request_cancel(request); - req_sendevent(request, ISC_R_CANCELED); + if (DNS_REQUEST_COMPLETE(request)) { + /* The request callback was already called */ + goto detach; } - UNLOCK(&request->requestmgr->locks[request->hash]); + if (eresult != ISC_R_SUCCESS) { + req_sendevent(request, eresult); + } +detach: /* attached in req_send() */ - dns_request_detach(&request); + dns_request_unref(request); } static void -req_response(isc_result_t result, isc_region_t *region, void *arg) { +req_response(isc_result_t eresult, isc_region_t *region, void *arg) { dns_request_t *request = (dns_request_t *)arg; - if (result == ISC_R_CANCELED) { + if (eresult == ISC_R_CANCELED) { return; } - req_log(ISC_LOG_DEBUG(3), "req_response: request %p: %s", request, - isc_result_totext(result)); - REQUIRE(VALID_REQUEST(request)); + REQUIRE(request->tid == isc_tid()); - if (result == ISC_R_TIMEDOUT) { - LOCK(&request->requestmgr->locks[request->hash]); - if (request->udpcount > 1 && - (request->flags & DNS_REQUEST_F_TCP) == 0) - { + req_log(ISC_LOG_DEBUG(3), "%s: request %p: %s", __func__, request, + isc_result_totext(eresult)); + + if (DNS_REQUEST_COMPLETE(request)) { + /* The request callback was already called */ + return; + } + + switch (eresult) { + case ISC_R_TIMEDOUT: + if (request->udpcount > 1 && !dns_request_usedtcp(request)) { request->udpcount -= 1; dns_dispatch_resume(request->dispentry, request->timeout); if (!DNS_REQUEST_SENDING(request)) { req_send(request); } - UNLOCK(&request->requestmgr->locks[request->hash]); return; } - - /* The lock is unlocked below */ - goto done; + break; + case ISC_R_SUCCESS: + /* Copy region to request. */ + isc_buffer_allocate(request->mctx, &request->answer, + region->length); + eresult = isc_buffer_copyregion(request->answer, region); + if (eresult != ISC_R_SUCCESS) { + isc_buffer_free(&request->answer); + } + break; + default: + break; } - LOCK(&request->requestmgr->locks[request->hash]); + req_sendevent(request, eresult); +} - if (result != ISC_R_SUCCESS) { - goto done; +static void +req_sendevent_cb(void *arg) { + dns_request_t *request = arg; + + request->cb(request); + dns_request_unref(request); +} + +static void +req_cleanup(dns_request_t *request) { + if (ISC_LINK_LINKED(request, link)) { + ISC_LIST_UNLINK(request->requestmgr->requests[request->tid], + request, link); } - - /* - * Copy region to request. - */ - isc_buffer_allocate(request->mctx, &request->answer, region->length); - result = isc_buffer_copyregion(request->answer, region); - if (result != ISC_R_SUCCESS) { - isc_buffer_free(&request->answer); - } - -done: - /* - * Cleanup. - */ if (request->dispentry != NULL) { dns_dispatch_done(&request->dispentry); } - request_cancel(request); - - /* - * Send completion event. - */ - req_sendevent(request, result); - UNLOCK(&request->requestmgr->locks[request->hash]); + if (request->dispatch != NULL) { + dns_dispatch_detach(&request->dispatch); + } } static void req_sendevent(dns_request_t *request, isc_result_t result) { REQUIRE(VALID_REQUEST(request)); + REQUIRE(request->tid == isc_tid()); + REQUIRE(!DNS_REQUEST_COMPLETE(request)); - if (request->complete) { - return; - } + request->flags |= DNS_REQUEST_F_COMPLETE; - req_log(ISC_LOG_DEBUG(3), "req_sendevent: request %p", request); + req_cleanup(request); - dns_request_ref(request); + req_log(ISC_LOG_DEBUG(3), "%s: request %p: %s", __func__, request, + isc_result_totext(result)); request->result = result; - request->complete = true; - isc_async_run(request->loop, request->cb, request); + dns_request_ref(request); + isc_async_run(request->loop, req_sendevent_cb, request); } static void req_destroy(dns_request_t *request) { REQUIRE(VALID_REQUEST(request)); + REQUIRE(request->tid == isc_tid()); REQUIRE(!ISC_LINK_LINKED(request, link)); - req_log(ISC_LOG_DEBUG(3), "req_destroy: request %p", request); + req_log(ISC_LOG_DEBUG(3), "%s: request %p", __func__, request); isc_refcount_destroy(&request->references); + /* + * These should have been cleaned up before the + * completion event was sent. + */ + INSIST(!ISC_LINK_LINKED(request, link)); + INSIST(request->dispentry == NULL); + INSIST(request->dispatch == NULL); + request->magic = 0; if (request->query != NULL) { isc_buffer_free(&request->query); @@ -1058,12 +987,6 @@ req_destroy(dns_request_t *request) { if (request->answer != NULL) { isc_buffer_free(&request->answer); } - if (request->dispentry != NULL) { - dns_dispatch_done(&request->dispentry); - } - if (request->dispatch != NULL) { - dns_dispatch_detach(&request->dispatch); - } if (request->tsig != NULL) { isc_buffer_free(&request->tsig); } @@ -1079,6 +1002,7 @@ req_destroy(dns_request_t *request) { void * dns_request_getarg(dns_request_t *request) { REQUIRE(VALID_REQUEST(request)); + REQUIRE(request->tid == isc_tid()); return (request->arg); } @@ -1086,11 +1010,16 @@ dns_request_getarg(dns_request_t *request) { isc_result_t dns_request_getresult(dns_request_t *request) { REQUIRE(VALID_REQUEST(request)); + REQUIRE(request->tid == isc_tid()); return (request->result); } +#if DNS_REQUEST_TRACE +ISC_REFCOUNT_TRACE_IMPL(dns_request, req_destroy); +#else ISC_REFCOUNT_IMPL(dns_request, req_destroy); +#endif static void req_log(int level, const char *fmt, ...) { diff --git a/lib/dns/view.c b/lib/dns/view.c index d133c805aa..0399ad73f8 100644 --- a/lib/dns/view.c +++ b/lib/dns/view.c @@ -622,7 +622,7 @@ dns_view_createresolver(dns_view_t *view, isc_loopmgr_t *loopmgr, dns_adb_create(mctx, view, loopmgr, &view->adb); isc_mem_detach(&mctx); - result = dns_requestmgr_create(view->mctx, view->dispatchmgr, + result = dns_requestmgr_create(view->mctx, loopmgr, view->dispatchmgr, dispatchv4, dispatchv6, &view->requestmgr); if (result != ISC_R_SUCCESS) { diff --git a/lib/dns/zone.c b/lib/dns/zone.c index e499444a93..b7dfbc97bf 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -11730,13 +11730,6 @@ checkds_cancel(dns_zone_t *zone) { } } -static void -forward_cancel_cb(void *arg) { - dns_request_t *request = arg; - dns_request_cancel(request); - dns_request_detach(&request); -} - static void forward_cancel(dns_zone_t *zone) { dns_forward_t *forward; @@ -11751,9 +11744,7 @@ forward_cancel(dns_zone_t *zone) { forward = ISC_LIST_NEXT(forward, link)) { if (forward->request != NULL) { - dns_request_t *request = NULL; - dns_request_attach(forward->request, &request); - isc_async_run(zone->loop, forward_cancel_cb, request); + dns_request_cancel(forward->request); } } } @@ -12878,8 +12869,9 @@ cleanup: if (msg != NULL) { dns_message_detach(&msg); } + dns_name_free(&sgr->name, zone->mctx); - dns_request_destroy(&request); + dns_request_destroy(&sgr->request); isc_mem_put(zone->mctx, sgr, sizeof(*sgr)); /* If last request, release all related resources */ @@ -12912,10 +12904,12 @@ stub_request_nameserver_address(struct stub_cb_args *args, bool ipv4, zone = args->stub->zone; sgr = isc_mem_get(zone->mctx, sizeof(*sgr)); - sgr->request = NULL; - sgr->args = args; - sgr->name = (dns_name_t)DNS_NAME_INITEMPTY; - sgr->ipv4 = ipv4; + *sgr = (struct stub_glue_request){ + .args = args, + .name = (dns_name_t)DNS_NAME_INITEMPTY, + .ipv4 = ipv4, + }; + dns_name_dup(name, zone->mctx, &sgr->name); create_query(zone, ipv4 ? dns_rdatatype_a : dns_rdatatype_aaaa, diff --git a/lib/isc/loop.c b/lib/isc/loop.c index 5ee9daa8eb..88c7fd3487 100644 --- a/lib/isc/loop.c +++ b/lib/isc/loop.c @@ -282,6 +282,9 @@ loop_thread(void *arg) { r = uv_run(&loop->loop, UV_RUN_DEFAULT); UV_RUNTIME_CHECK(uv_run, r); + /* Invalidate the loop early */ + loop->magic = 0; + isc_barrier_wait(&loop->loopmgr->stopping); return (NULL);