From aab691d51266f552a7923db32686fb9398b1d255 Mon Sep 17 00:00:00 2001 From: Diego Fronza Date: Wed, 8 Jul 2020 11:42:32 -0300 Subject: [PATCH 1/2] 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); } /* From a22e61d554b16dd19f3cb8bc76be61296263e2b0 Mon Sep 17 00:00:00 2001 From: Diego Fronza Date: Mon, 13 Jul 2020 11:43:36 -0300 Subject: [PATCH 2/2] Add CHANGES and release note for #1719 --- CHANGES | 3 +++ doc/notes/notes-current.rst | 8 ++++++++ 2 files changed, 11 insertions(+) diff --git a/CHANGES b/CHANGES index 10f2d0fcc8..f8df942d7d 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +5466. [bug] Addressed an error in recursive clients stats reporting. + [GL #1719] + 5465. [func] Fallback to built in trust-anchors, managed-keys, or trusted-keys if the bindkeys-file (bind.keys) cannot be parsed. [GL #1235] diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index b5bf70b039..8aab454039 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -49,6 +49,14 @@ Feature Changes Bug Fixes ~~~~~~~~~ +- Addressed an error in recursive clients stats reporting. + There were occasions when an incoming query could trigger a prefetch for + some eligible rrset, and if the prefetch code were executed before recursion, + no increment in recursive clients stats would take place. Conversely, + when processing the answers, if the recursion code were executed before the + prefetch, the same counter would be decremented without a matching increment. + [GL #1719] + - The DS set returned by ``dns_keynode_dsset()`` was not thread-safe. This could result in an INSIST being triggered. [GL #1926]