Fix ns_statscounter_recursclients underflow

The basic scenario for the problem was that in the process of
resolving a query, if any rrset was eligible for prefetching, then it
would trigger a call to query_prefetch(), this call would run in
parallel to the normal query processing.

The problem arises due to the fact that both query_prefetch(), and,
in the original thread, a call to ns_query_recurse(), try to attach
to the recursionquota, but recursing client stats counter is only
incremented if ns_query_recurse() attachs to it first.

Conversely, if fetch_callback() is called before prefetch_done(),
it would not only detach from recursionquota, but also decrement
the stats counter, if query_prefetch() attached to te quota first
that would result in a decrement not matched by an increment, as
expected.

To solve this issue an atomic bool was added, it is set once in
ns_query_recurse(), allowing fetch_callback() to check for it
and decrement stats accordingly.

For a more compreensive explanation check the thread comment below:
https://gitlab.isc.org/isc-projects/bind9/-/issues/1719#note_145857

diff --git a/lib/ns/include/ns/client.h b/lib/ns/include/ns/client.h
index 7fa285745b0728da4676103d29809cfdf9a2435b..735c286cf73cb04bed47584ffedd452966cc6c41 100644 (file)
--- a/lib/ns/include/ns/client.h
+++ b/lib/ns/include/ns/client.h
@@ -263,7 +263,8 @@ 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_NOSETFC  0x20000 /*%< don't set servfail cache */
+#define NS_CLIENTATTR_RECURSING 0x40000 /*%< client is recursing */
 
 /*
  * Flag to use with the SERVFAIL cache to indicate

diff --git a/lib/ns/query.c b/lib/ns/query.c
index 70277b9727c609b52f271f62113e10f9bd99cb1c..c4c15488f1067370fe177441f50a7270213f2731 100644 (file)
--- a/lib/ns/query.c
+++ b/lib/ns/query.c
@@ -5679,6 +5679,15 @@ 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);
@@ -5827,6 +5836,7 @@ 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) {
@@ -5880,6 +5890,18 @@ ns_query_recurse(ns_client_t *client, dns_rdatatype_t qtype, dns_name_t *qname,
                }
 
                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);
        }
 
        /*