From: Michał Kępień Date: Tue, 14 Jun 2022 11:13:32 +0000 (+0200) Subject: Attach to separate recursion quota pointers X-Git-Tag: v9.19.3~47^2~4 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=172e15f7adfe38ebd9eabc03b3385f0d6e835f42;p=thirdparty%2Fbind9.git Attach to separate recursion quota pointers Similarly to how different code paths reused common client handle pointers and fetch references despite being logically unrelated, they also reuse client->recursionquota, the field in which a reference to the recursion quota is stored. This unnecessarily forces all code using that field to be aware of the fact that it is overloaded by different features. Overloading client->recursionquota also causes inconsistent behavior. For example, if prefetch code triggers recursion and then delegation handling code also triggers recursion, only one of these code paths will be able to attach to the recursion quota, but both recursions will be started anyway. In other words, each code path only checks whether the recursion quota has not been exceeded if the quota has not yet been attached to by another code path. This behavior theoretically allows the configured recursion quota to be slightly exceeded; while it is not expected to be a real-world operational issue, it is still confusing and should therefore be fixed. Extend the structures comprising the 'recursions' array with a new field holding a pointer to the recursion quota that a given recursion process attached to. Update all code paths using client->recursionquota so that they use the appropriate slot in the 'recursions' array. Drop the 'recursionquota' field from ns_client_t. --- diff --git a/lib/ns/client.c b/lib/ns/client.c index c40f40572c9..ebfcc77ba98 100644 --- a/lib/ns/client.c +++ b/lib/ns/client.c @@ -62,7 +62,6 @@ #include #include #include -#include #include #include #include @@ -268,9 +267,9 @@ ns_client_endrequest(ns_client_t *client) { * fetch_callback(), but if we're shutting down and canceling then * it might not have happened. */ - if (client->recursionquota != NULL) { - isc_quota_detach(&client->recursionquota); - if (FETCH_RECTYPE_PREFETCH(client) == NULL) { + for (int i = 0; i < RECTYPE_COUNT; i++) { + if (client->query.recursions[i].quota != NULL) { + isc_quota_detach(&client->query.recursions[i].quota); ns_stats_decrement(client->manager->sctx->nsstats, ns_statscounter_recursclients); } @@ -1647,7 +1646,9 @@ ns__client_reset_cb(void *client0) { } client->state = NS_CLIENTSTATE_READY; - INSIST(client->recursionquota == NULL); + for (int i = 0; i < RECTYPE_COUNT; i++) { + INSIST(client->query.recursions[i].quota == NULL); + } #ifdef WANT_SINGLETRACE isc_log_setforcelog(false); @@ -1763,7 +1764,9 @@ ns__client_request(isc_nmhandle_t *handle, isc_result_t eresult, client->attributes |= NS_CLIENTATTR_TCP; } - INSIST(client->recursionquota == NULL); + for (int i = 0; i < RECTYPE_COUNT; i++) { + INSIST(client->query.recursions[i].quota == NULL); + } INSIST(client->state == NS_CLIENTSTATE_READY); diff --git a/lib/ns/include/ns/client.h b/lib/ns/include/ns/client.h index bf3d85db004..743578b5002 100644 --- a/lib/ns/include/ns/client.h +++ b/lib/ns/include/ns/client.h @@ -191,7 +191,6 @@ struct ns_client { isc_time_t tnow; dns_name_t signername; /*%< [T]SIG key name */ dns_name_t *signer; /*%< NULL if not valid sig */ - isc_quota_t *recursionquota; isc_sockaddr_t peeraddr; bool peeraddr_valid; diff --git a/lib/ns/include/ns/query.h b/lib/ns/include/ns/query.h index 18d1b5c9b60..a1e4a284df9 100644 --- a/lib/ns/include/ns/query.h +++ b/lib/ns/include/ns/query.h @@ -76,6 +76,19 @@ typedef enum { #define FETCH_RECTYPE_HOOK(client) \ ((client)->query.recursions[RECTYPE_HOOK].fetch) +/*% + * Helper macros for accessing isc_quota_t pointers for a specific recursion a + * given client is associated with. + */ +#define QUOTA_RECTYPE_NORMAL(client) \ + ((client)->query.recursions[RECTYPE_NORMAL].quota) +#define QUOTA_RECTYPE_PREFETCH(client) \ + ((client)->query.recursions[RECTYPE_PREFETCH].quota) +#define QUOTA_RECTYPE_RPZ(client) \ + ((client)->query.recursions[RECTYPE_RPZ].quota) +#define QUOTA_RECTYPE_HOOK(client) \ + ((client)->query.recursions[RECTYPE_HOOK].quota) + /*% * nameserver recursion parameters, to uniquely identify a recursion * query; this is used to detect a recursion loop @@ -133,6 +146,7 @@ struct ns_query { struct { isc_nmhandle_t *handle; dns_fetch_t *fetch; + isc_quota_t *quota; } recursions[RECTYPE_COUNT]; ns_query_recparam_t recparam; diff --git a/lib/ns/query.c b/lib/ns/query.c index 0504bb021c0..f1f004a6848 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -2458,15 +2458,17 @@ free_devent(ns_client_t *client, isc_event_t **eventp, } static isc_result_t -recursionquota_attach(ns_client_t *client, bool soft_limit) { +recursionquotatype_attach(ns_client_t *client, + ns_query_rectype_t recursion_type, bool soft_limit) { + isc_quota_t **quotap = &client->query.recursions[recursion_type].quota; isc_result_t result; - if (client->recursionquota != NULL) { + if (*quotap != NULL) { return (ISC_R_SUCCESS); } result = isc_quota_attach(&client->manager->sctx->recursionquota, - &client->recursionquota); + quotap); switch (result) { case ISC_R_SUCCESS: break; @@ -2480,7 +2482,7 @@ recursionquota_attach(ns_client_t *client, bool soft_limit) { break; } - isc_quota_detach(&client->recursionquota); + isc_quota_detach(quotap); FALLTHROUGH; default: return (result); @@ -2493,19 +2495,23 @@ recursionquota_attach(ns_client_t *client, bool soft_limit) { } static isc_result_t -recursionquota_attach_hard(ns_client_t *client) { - return (recursionquota_attach(client, false)); +recursionquotatype_attach_hard(ns_client_t *client, + ns_query_rectype_t recursion_type) { + return (recursionquotatype_attach(client, recursion_type, false)); } static isc_result_t -recursionquota_attach_soft(ns_client_t *client) { - return (recursionquota_attach(client, true)); +recursionquotatype_attach_soft(ns_client_t *client, + ns_query_rectype_t recursion_type) { + return (recursionquotatype_attach(client, recursion_type, true)); } static void -recursionquota_detach(ns_client_t *client) { - if (client->recursionquota != NULL) { - isc_quota_detach(&client->recursionquota); +recursionquotatype_detach(ns_client_t *client, + ns_query_rectype_t recursion_type) { + if (client->query.recursions[recursion_type].quota != NULL) { + isc_quota_detach( + &client->query.recursions[recursion_type].quota); } ns_stats_decrement(client->manager->sctx->nsstats, @@ -2539,7 +2545,7 @@ cleanup_after_fetch(isc_task_t *task, isc_event_t *event, } UNLOCK(&client->query.fetchlock); - recursionquota_detach(client); + recursionquotatype_detach(client, recursion_type); free_devent(client, &event, &devent); isc_nmhandle_detach(handlep); @@ -2574,7 +2580,7 @@ fetch_and_forget(ns_client_t *client, dns_name_t *qname, dns_rdatatype_t qtype, dns_fetch_t **fetchp; isc_result_t result; - result = recursionquota_attach_hard(client); + result = recursionquotatype_attach_hard(client, recursion_type); if (result != ISC_R_SUCCESS) { return; } @@ -6204,7 +6210,7 @@ fetch_callback(isc_task_t *task, isc_event_t *event) { * the manager's recursing-clients list. */ - recursionquota_detach(client); + recursionquotatype_detach(client, RECTYPE_NORMAL); LOCK(&client->manager->reclock); if (ISC_LINK_LINKED(client, rlink)) { @@ -6322,7 +6328,8 @@ static atomic_uint_fast32_t last_soft, last_hard; * Check recursion quota before making the current client "recursing". */ static isc_result_t -check_recursionquota(ns_client_t *client) { +check_recursionquota(ns_client_t *client, ns_query_rectype_t recursion_type) { + isc_quota_t **quotap = &client->query.recursions[recursion_type].quota; isc_result_t result = ISC_R_SUCCESS; /* @@ -6335,8 +6342,8 @@ check_recursionquota(ns_client_t *client) { * because those have already been replaced when the * connection was accepted (if allowed by the TCP quota). */ - if (client->recursionquota == NULL) { - result = recursionquota_attach_soft(client); + if (*quotap == NULL) { + result = recursionquotatype_attach_soft(client, recursion_type); if (result == ISC_R_SOFTQUOTA) { isc_stdtime_t now; isc_stdtime_get(&now); @@ -6348,12 +6355,9 @@ check_recursionquota(ns_client_t *client) { "recursive-clients soft limit " "exceeded (%u/%u/%u), " "aborting oldest query", - isc_quota_getused( - client->recursionquota), - isc_quota_getsoft( - client->recursionquota), - isc_quota_getmax( - client->recursionquota)); + isc_quota_getused(*quotap), + isc_quota_getsoft(*quotap), + isc_quota_getmax(*quotap)); } ns_client_killoldestquery(client); result = ISC_R_SUCCESS; @@ -6414,7 +6418,7 @@ ns_query_recurse(ns_client_t *client, dns_rdatatype_t qtype, dns_name_t *qname, inc_stats(client, ns_statscounter_recursion); } - result = check_recursionquota(client); + result = check_recursionquota(client, RECTYPE_NORMAL); if (result != ISC_R_SUCCESS) { return (result); } @@ -6687,7 +6691,7 @@ query_hookresume(isc_task_t *task, isc_event_t *event) { UNLOCK(&client->query.fetchlock); SAVE(hctx, rev->ctx); - recursionquota_detach(client); + recursionquotatype_detach(client, RECTYPE_HOOK); LOCK(&client->manager->reclock); if (ISC_LINK_LINKED(client, rlink)) { @@ -6818,7 +6822,7 @@ ns_query_hookasync(query_ctx_t *qctx, ns_query_starthookasync_t runasync, REQUIRE(client->query.hookactx == NULL); REQUIRE(FETCH_RECTYPE_NORMAL(client) == NULL); - result = check_recursionquota(client); + result = check_recursionquota(client, RECTYPE_HOOK); if (result != ISC_R_SUCCESS) { goto cleanup; } diff --git a/tests/ns/query_test.c b/tests/ns/query_test.c index 5636801b142..eabfec40bd2 100644 --- a/tests/ns/query_test.c +++ b/tests/ns/query_test.c @@ -968,7 +968,7 @@ run_hookasync_test(const ns__query_hookasync_test_params_t *test) { /* Confirm necessary cleanup has been performed. */ INSIST(qctx->client->query.hookactx == NULL); INSIST(qctx->client->state == NS_CLIENTSTATE_WORKING); - INSIST(qctx->client->recursionquota == NULL); + INSIST(QUOTA_RECTYPE_HOOK(qctx->client) == NULL); INSIST(ns_stats_get_counter( qctx->client->manager->sctx->nsstats, ns_statscounter_recursclients) == 0);