From f7482b68b9623cad01f21fc8816d84a29183f2d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Wed, 23 Feb 2022 14:39:11 +0100 Subject: [PATCH] Fix more ns_statscounter_recursclients underflows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit aab691d51266f552a7923db32686fb9398b1d255 did not fix all possible scenarios in which the ns_statscounter_recursclients counter underflows. The solution implemented therein can be ineffective e.g. when CNAME chaining happens with prefetching enabled. Here is an example recursive resolution scenario in which the ns_statscounter_recursclients counter can underflow with the current logic in effect: 1. Query processing starts, the answer is not found in the cache, so recursion is started. The NS_CLIENTATTR_RECURSING attribute is set. ns_statscounter_recursclients is incremented (Δ = +1). 2. Recursion completes, returning a CNAME. client->recursionquota is non-NULL, so the NS_CLIENTATTR_RECURSING attribute remains set. ns_statscounter_recursclients is decremented (Δ = 0). 3. Query processing restarts. 4. The current QNAME (the target of the CNAME from step 2) is found in the cache, with a TTL low enough to trigger a prefetch. 5. query_prefetch() attaches to client->recursionquota. ns_statscounter_recursclients is not incremented because query_prefetch() does not do that (Δ = 0). 6. Query processing restarts. 7. The current QNAME (the target of the CNAME from step 4) is not found in the cache, so recursion is started. client->recursionquota is already attached to (since step 5) and the NS_CLIENTATTR_RECURSING attribute is set (since step 1), so ns_statscounter_recursclients is not incremented (Δ = 0). 8. The prefetch from step 5 completes. client->recursionquota is detached from in prefetch_done(). ns_statscounter_recursclients is not decremented because prefetch_done() does not do that (Δ = 0). 9. Recursion for the current QNAME completes. client->recursionquota is already detached from, i.e. set to NULL (since step 8), and the NS_CLIENTATTR_RECURSING attribute is set (since step 1), so ns_statscounter_recursclients is decremented (Δ = -1). Another possible scenario is that after step 7, recursion for the target of the CNAME from step 4 completes before the prefetch for the CNAME itself. fetch_callback() then notices that client->recursionquota is non-NULL and decrements ns_statscounter_recursclients, even though client->recursionquota was attached to by query_prefetch() and therefore not accompanied by an incrementation of ns_statscounter_recursclients. The net result is also an underflow. Instead of trying to properly handle all possible orderings of events set into motion by normal recursion and prefetch-triggered recursion, adjust ns_statscounter_recursclients whenever the recursive clients quota is successfully attached to or detached from. Remove the NS_CLIENTATTR_RECURSING attribute altogether as its only purpose is made obsolete by this change. --- lib/ns/include/ns/client.h | 3 +-- lib/ns/query.c | 46 +++++++++++++++----------------------- 2 files changed, 19 insertions(+), 30 deletions(-) diff --git a/lib/ns/include/ns/client.h b/lib/ns/include/ns/client.h index a5c1f0072e..c5d8e86cf7 100644 --- a/lib/ns/include/ns/client.h +++ b/lib/ns/include/ns/client.h @@ -267,8 +267,7 @@ struct ns_client { #define NS_CLIENTATTR_WANTPAD 0x08000 /*%< pad reply */ #define NS_CLIENTATTR_USEKEEPALIVE 0x10000 /*%< use TCP keepalive */ -#define NS_CLIENTATTR_NOSETFC 0x20000 /*%< don't set servfail cache */ -#define NS_CLIENTATTR_RECURSING 0x40000 /*%< client is recursing */ +#define NS_CLIENTATTR_NOSETFC 0x20000 /*%< don't set servfail cache */ /* * Flag to use with the SERVFAIL cache to indicate diff --git a/lib/ns/query.c b/lib/ns/query.c index d87d1f8ec9..4ef38965ed 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -2530,6 +2530,8 @@ prefetch_done(isc_task_t *task, isc_event_t *event) { */ if (client->recursionquota != NULL) { isc_quota_detach(&client->recursionquota); + ns_stats_decrement(client->sctx->nsstats, + ns_statscounter_recursclients); } free_devent(client, &event, &devent); @@ -2557,10 +2559,15 @@ query_prefetch(ns_client_t *client, dns_name_t *qname, if (client->recursionquota == NULL) { result = isc_quota_attach(&client->sctx->recursionquota, &client->recursionquota); - if (result == ISC_R_SOFTQUOTA) { + switch (result) { + case ISC_R_SUCCESS: + ns_stats_increment(client->sctx->nsstats, + ns_statscounter_recursclients); + break; + case ISC_R_SOFTQUOTA: isc_quota_detach(&client->recursionquota); - } - if (result != ISC_R_SUCCESS) { + /* FALLTHROUGH */ + default: return; } } @@ -2770,10 +2777,15 @@ query_rpzfetch(ns_client_t *client, dns_name_t *qname, dns_rdatatype_t type) { if (client->recursionquota == NULL) { result = isc_quota_attach(&client->sctx->recursionquota, &client->recursionquota); - if (result == ISC_R_SOFTQUOTA) { + switch (result) { + case ISC_R_SUCCESS: + ns_stats_increment(client->sctx->nsstats, + ns_statscounter_recursclients); + break; + case ISC_R_SOFTQUOTA: isc_quota_detach(&client->recursionquota); - } - if (result != ISC_R_SUCCESS) { + /* FALLTHROUGH */ + default: return; } } @@ -6185,15 +6197,6 @@ fetch_callback(isc_task_t *task, isc_event_t *event) { isc_quota_detach(&client->recursionquota); ns_stats_decrement(client->sctx->nsstats, ns_statscounter_recursclients); - } else if (client->attributes & NS_CLIENTATTR_RECURSING) { - client->attributes &= ~NS_CLIENTATTR_RECURSING; - /* - * Detached from recursionquota via prefetch_done(), - * but need to decrement recursive client stats here anyway, - * since it was incremented in ns_query_recurse(). - */ - ns_stats_decrement(client->sctx->nsstats, - ns_statscounter_recursclients); } LOCK(&client->manager->reclock); @@ -6332,7 +6335,6 @@ check_recursionquota(ns_client_t *client) { if (result == ISC_R_SUCCESS || result == ISC_R_SOFTQUOTA) { ns_stats_increment(client->sctx->nsstats, ns_statscounter_recursclients); - client->attributes |= NS_CLIENTATTR_RECURSING; } if (result == ISC_R_SOFTQUOTA) { @@ -6381,18 +6383,6 @@ check_recursionquota(ns_client_t *client) { dns_message_clonebuffer(client->message); ns_client_recursing(client); - } else if ((client->attributes & NS_CLIENTATTR_RECURSING) == 0) { - client->attributes |= NS_CLIENTATTR_RECURSING; - /* - * query_prefetch() attached first to client->recursionquota, - * but we must check if NS_CLIENTATTR_RECURSING attribute is - * on, if not then turn it on and increment recursing clients - * stats counter only once. The attribute is also checked in - * fetch_callback() to know if a matching decrement to this - * counter should be applied. - */ - ns_stats_increment(client->sctx->nsstats, - ns_statscounter_recursclients); } return (result);