From af6fcf5641a1d32957f2ba62c4493139cb1c6e01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Tue, 14 Jun 2022 13:13:32 +0200 Subject: [PATCH 1/4] Make resolver glue code use the 'recursions' array With prefetch and RPZ code updated to use separate slots in the 'recursions' array, the code responsible for starting recursion in ns_query_recurse() and resuming query handling in fetch_callback() should follow suit, so that it does not need to explicitly cooperate with other code paths that may initiate recursion. Replace: - client->fetchhandle with HANDLE_RECTYPE_NORMAL(client) - client->query.fetch with FETCH_RECTYPE_NORMAL(client) Also update other functions using client->fetchhandle and client->query.fetch (ns_query_cancel(), query_usestale()) so that those two fields can shortly be dropped altogether. --- lib/ns/include/ns/query.h | 5 +++++ lib/ns/query.c | 40 ++++++++++++++++++++------------------- 2 files changed, 26 insertions(+), 19 deletions(-) diff --git a/lib/ns/include/ns/query.h b/lib/ns/include/ns/query.h index a2f77d35f0..67b008c7fa 100644 --- a/lib/ns/include/ns/query.h +++ b/lib/ns/include/ns/query.h @@ -43,6 +43,7 @@ typedef struct ns_dbversion { * allows common code paths to differentiate between them */ typedef enum { + RECTYPE_NORMAL, RECTYPE_PREFETCH, RECTYPE_RPZ, RECTYPE_COUNT, @@ -52,6 +53,8 @@ typedef enum { * Helper macros for accessing isc_nmhandle_t pointers for a specific recursion * a given client is associated with. */ +#define HANDLE_RECTYPE_NORMAL(client) \ + ((client)->query.recursions[RECTYPE_NORMAL].handle) #define HANDLE_RECTYPE_PREFETCH(client) \ ((client)->query.recursions[RECTYPE_PREFETCH].handle) #define HANDLE_RECTYPE_RPZ(client) \ @@ -61,6 +64,8 @@ typedef enum { * Helper macros for accessing dns_fetch_t pointers for a specific recursion a * given client is associated with. */ +#define FETCH_RECTYPE_NORMAL(client) \ + ((client)->query.recursions[RECTYPE_NORMAL].fetch) #define FETCH_RECTYPE_PREFETCH(client) \ ((client)->query.recursions[RECTYPE_PREFETCH].fetch) #define FETCH_RECTYPE_RPZ(client) \ diff --git a/lib/ns/query.c b/lib/ns/query.c index 0696eb35a2..e2e55a243f 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -660,10 +660,9 @@ ns_query_cancel(ns_client_t *client) { REQUIRE(NS_CLIENT_VALID(client)); LOCK(&client->query.fetchlock); - if (client->query.fetch != NULL) { - dns_resolver_cancelfetch(client->query.fetch); - - client->query.fetch = NULL; + if (FETCH_RECTYPE_NORMAL(client) != NULL) { + dns_resolver_cancelfetch(FETCH_RECTYPE_NORMAL(client)); + FETCH_RECTYPE_NORMAL(client) = NULL; } if (client->query.hookactx != NULL) { client->query.hookactx->cancel(client->query.hookactx); @@ -5961,9 +5960,11 @@ query_lookup(query_ctx_t *qctx) { qctx->client->query.dboptions &= ~DNS_DBFIND_STALETIMEOUT; qctx->options &= ~DNS_GETDB_STALEFIRST; - if (qctx->client->query.fetch != NULL) { + if (FETCH_RECTYPE_NORMAL(qctx->client) != NULL) + { dns_resolver_destroyfetch( - &qctx->client->query.fetch); + &FETCH_RECTYPE_NORMAL( + qctx->client)); } return (query_lookup(qctx)); } else { @@ -6163,22 +6164,22 @@ fetch_callback(isc_task_t *task, isc_event_t *event) { client->nodetach = false; LOCK(&client->query.fetchlock); - INSIST(client->query.fetch == devent->fetch || - client->query.fetch == NULL); + INSIST(FETCH_RECTYPE_NORMAL(client) == devent->fetch || + FETCH_RECTYPE_NORMAL(client) == NULL); if (QUERY_STALEPENDING(&client->query)) { /* * We've gotten an authoritative answer to a query that * was left pending after a stale timeout. We don't need * to do anything with it; free all the data and go home. */ - client->query.fetch = NULL; + FETCH_RECTYPE_NORMAL(client) = NULL; fetch_answered = true; - } else if (client->query.fetch != NULL) { + } else if (FETCH_RECTYPE_NORMAL(client) != NULL) { /* * This is the fetch we've been waiting for. */ - INSIST(devent->fetch == client->query.fetch); - client->query.fetch = NULL; + INSIST(FETCH_RECTYPE_NORMAL(client) == devent->fetch); + FETCH_RECTYPE_NORMAL(client) = NULL; /* * Update client->now. @@ -6208,7 +6209,7 @@ fetch_callback(isc_task_t *task, isc_event_t *event) { } UNLOCK(&client->manager->reclock); - isc_nmhandle_detach(&client->fetchhandle); + isc_nmhandle_detach(&HANDLE_RECTYPE_NORMAL(client)); client->query.attributes &= ~NS_QUERYATTR_RECURSING; client->state = NS_CLIENTSTATE_WORKING; @@ -6419,7 +6420,7 @@ ns_query_recurse(ns_client_t *client, dns_rdatatype_t qtype, dns_name_t *qname, * Invoke the resolver. */ REQUIRE(nameservers == NULL || nameservers->type == dns_rdatatype_ns); - REQUIRE(client->query.fetch == NULL); + REQUIRE(FETCH_RECTYPE_NORMAL(client) == NULL); rdataset = ns_client_newrdataset(client); @@ -6444,14 +6445,14 @@ ns_query_recurse(ns_client_t *client, dns_rdatatype_t qtype, dns_name_t *qname, client->query.fetchoptions |= DNS_FETCHOPT_TRYSTALE_ONTIMEOUT; } - isc_nmhandle_attach(client->handle, &client->fetchhandle); + isc_nmhandle_attach(client->handle, &HANDLE_RECTYPE_NORMAL(client)); result = dns_resolver_createfetch( client->view->resolver, qname, qtype, qdomain, nameservers, NULL, peeraddr, client->message->id, client->query.fetchoptions, 0, NULL, client->manager->task, fetch_callback, client, - rdataset, sigrdataset, &client->query.fetch); + rdataset, sigrdataset, &FETCH_RECTYPE_NORMAL(client)); if (result != ISC_R_SUCCESS) { - isc_nmhandle_detach(&client->fetchhandle); + isc_nmhandle_detach(&HANDLE_RECTYPE_NORMAL(client)); ns_client_putrdataset(client, &rdataset); if (sigrdataset != NULL) { ns_client_putrdataset(client, &sigrdataset); @@ -7495,8 +7496,9 @@ query_usestale(query_ctx_t *qctx, isc_result_t result) { dns_db_attach(qctx->client->view->cachedb, &qctx->db); qctx->version = NULL; qctx->client->query.dboptions |= DNS_DBFIND_STALEOK; - if (qctx->client->query.fetch != NULL) { - dns_resolver_destroyfetch(&qctx->client->query.fetch); + if (FETCH_RECTYPE_NORMAL(qctx->client) != NULL) { + dns_resolver_destroyfetch( + &FETCH_RECTYPE_NORMAL(qctx->client)); } /* From e0be643f5087fe3e84ef9991e90aa5abf2f18129 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Tue, 14 Jun 2022 13:13:32 +0200 Subject: [PATCH 2/4] Make async hooks code use the 'recursions' array Async hooks are the last feature using the client->fetchhandle and client->query.fetch pointers. Update ns_query_hookasync() and query_hookresume() so that they use a dedicated slot in the 'recursions' array. Note that async hooks are still not expected to initiate recursion if one was already started by a prior ns_query_recurse() call, so the REQUIRE assertion in ns_query_hookasync() needs to check the RECTYPE_NORMAL slot rather than the RECTYPE_HOOK one. --- lib/ns/include/ns/query.h | 5 +++++ lib/ns/query.c | 20 +++++++++----------- tests/ns/query_test.c | 8 ++++---- 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/lib/ns/include/ns/query.h b/lib/ns/include/ns/query.h index 67b008c7fa..321e83eeca 100644 --- a/lib/ns/include/ns/query.h +++ b/lib/ns/include/ns/query.h @@ -46,6 +46,7 @@ typedef enum { RECTYPE_NORMAL, RECTYPE_PREFETCH, RECTYPE_RPZ, + RECTYPE_HOOK, RECTYPE_COUNT, } ns_query_rectype_t; @@ -59,6 +60,8 @@ typedef enum { ((client)->query.recursions[RECTYPE_PREFETCH].handle) #define HANDLE_RECTYPE_RPZ(client) \ ((client)->query.recursions[RECTYPE_RPZ].handle) +#define HANDLE_RECTYPE_HOOK(client) \ + ((client)->query.recursions[RECTYPE_HOOK].handle) /*% * Helper macros for accessing dns_fetch_t pointers for a specific recursion a @@ -70,6 +73,8 @@ typedef enum { ((client)->query.recursions[RECTYPE_PREFETCH].fetch) #define FETCH_RECTYPE_RPZ(client) \ ((client)->query.recursions[RECTYPE_RPZ].fetch) +#define FETCH_RECTYPE_HOOK(client) \ + ((client)->query.recursions[RECTYPE_HOOK].fetch) /*% * nameserver recursion parameters, to uniquely identify a recursion diff --git a/lib/ns/query.c b/lib/ns/query.c index e2e55a243f..17bfcd3ee7 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -6693,12 +6693,11 @@ query_hookresume(isc_task_t *task, isc_event_t *event) { UNLOCK(&client->manager->reclock); /* - * This event is running under a client task, so it's safe to detach - * the fetch handle. And it should be done before resuming query - * processing below, since that may trigger another recursion or - * asynchronous hook event. + * The fetch handle should be detached before resuming query processing + * below, since that may trigger another recursion or asynchronous hook + * event. */ - isc_nmhandle_detach(&client->fetchhandle); + isc_nmhandle_detach(&HANDLE_RECTYPE_HOOK(client)); client->state = NS_CLIENTSTATE_WORKING; @@ -6814,7 +6813,7 @@ ns_query_hookasync(query_ctx_t *qctx, ns_query_starthookasync_t runasync, REQUIRE(NS_CLIENT_VALID(client)); REQUIRE(client->query.hookactx == NULL); - REQUIRE(client->query.fetch == NULL); + REQUIRE(FETCH_RECTYPE_NORMAL(client) == NULL); result = check_recursionquota(client); if (result != ISC_R_SUCCESS) { @@ -6837,12 +6836,11 @@ ns_query_hookasync(query_ctx_t *qctx, ns_query_starthookasync_t runasync, * attribute won't be checked anywhere. * * Hook-based asynchronous processing cannot coincide with normal - * recursion, so we can safely use fetchhandle here. Unlike in - * ns_query_recurse(), we attach to the handle only if 'runasync' - * succeeds. It should be safe since we're either in the client - * task or pausing it. + * recursion. Unlike in ns_query_recurse(), we attach to the handle + * only if 'runasync' succeeds. It should be safe since we're either in + * the client task or pausing it. */ - isc_nmhandle_attach(client->handle, &client->fetchhandle); + isc_nmhandle_attach(client->handle, &HANDLE_RECTYPE_HOOK(client)); return (ISC_R_SUCCESS); cleanup: diff --git a/tests/ns/query_test.c b/tests/ns/query_test.c index dda2c07150..5636801b14 100644 --- a/tests/ns/query_test.c +++ b/tests/ns/query_test.c @@ -709,11 +709,11 @@ hook_async_common(void *arg, void *data, isc_result_t *resultp, } } else { /* - * Resume from the completion of async event. - * fetchhandle should have been detached so that we can start - * another async event or DNS recursive resolution. + * Resume from the completion of async event. The fetch handle + * should have been detached so that we can start another async + * event or DNS recursive resolution. */ - INSIST(qctx->client->fetchhandle == NULL); + INSIST(HANDLE_RECTYPE_HOOK(qctx->client) == NULL); asdata->async = false; switch (hookpoint) { case NS_QUERY_GOT_ANSWER_BEGIN: From 9e187b893d03f6d3dbba0962a3280d3d9722b1e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Tue, 14 Jun 2022 13:13:32 +0200 Subject: [PATCH 3/4] Drop the 'fetchhandle' and 'fetch' fields Drop the 'fetchhandle' field from ns_client_t as all code using it has been migrated to use the recursion-type-specific HANDLE_RECTYPE_*() macros. Drop the 'fetch' field from ns_query_t as all code using it has been migrated to use the recursion-type-specific FETCH_RECTYPE_*() macros. --- lib/ns/include/ns/client.h | 1 - lib/ns/include/ns/query.h | 1 - 2 files changed, 2 deletions(-) diff --git a/lib/ns/include/ns/client.h b/lib/ns/include/ns/client.h index 67c641b7cc..bf3d85db00 100644 --- a/lib/ns/include/ns/client.h +++ b/lib/ns/include/ns/client.h @@ -174,7 +174,6 @@ struct ns_client { isc_nmhandle_t *sendhandle; /* Waiting for send callback */ isc_nmhandle_t *reqhandle; /* Waiting for request callback (query, update, notify) */ - isc_nmhandle_t *fetchhandle; /* Waiting for recursive fetch */ isc_nmhandle_t *updatehandle; /* Waiting for update callback */ unsigned char *tcpbuf; dns_message_t *message; diff --git a/lib/ns/include/ns/query.h b/lib/ns/include/ns/query.h index 321e83eeca..18d1b5c9b6 100644 --- a/lib/ns/include/ns/query.h +++ b/lib/ns/include/ns/query.h @@ -104,7 +104,6 @@ struct ns_query { bool authdbset; bool isreferral; isc_mutex_t fetchlock; - dns_fetch_t *fetch; ns_hookasync_t *hookactx; dns_rpz_st_t *rpz_st; isc_bufferlist_t namebufs; From 95e703121db434eb6d7c5bb3611712b4f8f4f029 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Tue, 14 Jun 2022 13:13:32 +0200 Subject: [PATCH 4/4] Ensure ns_query_cancel() handles all recursions Previously, multiple code paths reused client->query.fetch, so it was enough for ns_query_cancel() to issue a single call to dns_resolver_cancelfetch() with that fetch as an argument. Now, since each slot in the 'recursions' array can hold a reference to a separate resolver fetch, ns_query_cancel() needs to handle all of them, so that all recursion callbacks get a chance to clean up the associated resources when a query is canceled. --- lib/ns/query.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/ns/query.c b/lib/ns/query.c index 17bfcd3ee7..0504bb021c 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -660,9 +660,12 @@ ns_query_cancel(ns_client_t *client) { REQUIRE(NS_CLIENT_VALID(client)); LOCK(&client->query.fetchlock); - if (FETCH_RECTYPE_NORMAL(client) != NULL) { - dns_resolver_cancelfetch(FETCH_RECTYPE_NORMAL(client)); - FETCH_RECTYPE_NORMAL(client) = NULL; + for (int i = 0; i < RECTYPE_COUNT; i++) { + dns_fetch_t **fetchp = &client->query.recursions[i].fetch; + if (*fetchp != NULL) { + dns_resolver_cancelfetch(*fetchp); + *fetchp = NULL; + } } if (client->query.hookactx != NULL) { client->query.hookactx->cancel(client->query.hookactx);