packages/bind
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity
bind/backport-0015-Fix-more-ns_statscounter_recursclients-underflows.patch
jiangheng e9c081b514 backport bugfix patches
2023-01-09 16:44:23 +08:00

186 lines
7.2 KiB
Diff
Raw Blame History

From 60e82835ecc0791b2de5131eeb7c636b000577f2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= <michal@isc.org>
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.
(cherry picked from commit f7482b68b9623cad01f21fc8816d84a29183f2d1)
Conflict: NA
Reference: https://gitlab.isc.org/isc-projects/bind9/-/commit/60e82835ecc0791b2de5131eeb7c636b000577f2
---
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 1fd08633d3..91e8ccf8dc 100644
--- a/lib/ns/include/ns/client.h
+++ b/lib/ns/include/ns/client.h
@@ -270,8 +270,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 335d877d48..176c552b41 100644
--- a/lib/ns/query.c
+++ b/lib/ns/query.c
@@ -2486,6 +2486,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);
@@ -2513,10 +2515,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;
}
}
@@ -2726,10 +2733,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;
}
}
@@ -6094,15 +6106,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);
@@ -6268,7 +6271,6 @@ ns_query_recurse(ns_client_t *client, dns_rdatatype_t qtype, dns_name_t *qname,
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) {
@@ -6323,18 +6325,6 @@ ns_query_recurse(ns_client_t *client, dns_rdatatype_t qtype, dns_name_t *qname,
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);
}
/*
--
2.23.0
Reference in New Issue View Git Blame Copy Permalink