From aab691d51266f552a7923db32686fb9398b1d255 Mon Sep 17 00:00:00 2001 From: Diego Fronza Date: Wed, 8 Jul 2020 11:42:32 -0300 Subject: [PATCH] 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 --- lib/ns/include/ns/client.h | 3 ++- lib/ns/query.c | 22 ++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/lib/ns/include/ns/client.h b/lib/ns/include/ns/client.h index ee5dcef8bc..bf386e869e 100644 --- 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 d1fe194707..6147ff3705 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -5686,6 +5686,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); @@ -5834,6 +5843,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) { @@ -5887,6 +5897,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); } /*