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.

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);
 

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;

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;

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;
 	}

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);