mir/bind
2
0
Fork 0
mirror of https://gitlab.isc.org/isc-projects/bind9 synced 2025-08-30 14:07:59 +00:00
Code Issues Releases Wiki Activity

Fix more ns_statscounter_recursclients underflows

Browse Source 
Commit aab691d512 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.
This commit is contained in:
Michał Kępień
2022-02-23 14:39:11 +01:00
parent df0bc2b3b6
commit f7482b68b9
2 changed files with 19 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
lib/ns/include/ns/client.h
View File

@@ -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

46
lib/ns/query.c
View File

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