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 c40f40572c9ac67cd33d30fe7e6b82ac46969488..ebfcc77ba985e7dddbb72d623a9f8d61f7d9fa04 100644 (file)
--- a/lib/ns/client.c
+++ b/lib/ns/client.c
@@ -62,7 +62,6 @@
 #include <ns/interfacemgr.h>
 #include <ns/log.h>
 #include <ns/notify.h>
-#include <ns/query.h>
 #include <ns/server.h>
 #include <ns/stats.h>
 #include <ns/update.h>
@@ -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 bf3d85db0047ac9eace8213e4542f9d041af98e3..743578b5002df828109814e224ec1578ef343aba 100644 (file)
--- 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 18d1b5c9b604abc670fb6781482680842af8a937..a1e4a284df91b62e516f3e29de9026fb7cb396fe 100644 (file)
--- 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 0504bb021c0156e1c1c0f5a1e5ac230392398029..f1f004a6848ad669dc14c629e33bbc3fcab70c56 100644 (file)
--- 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 5636801b142b96d9c5afb67690434f7e814762e5..eabfec40bd2ea4326d66fb24365c2142efe28bdd 100644 (file)
--- 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);