From 667d81b52bf502ed12c86d36b1c1c0fedeec17e6 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Wed, 9 Jul 2025 12:40:34 +0200 Subject: [PATCH] Make serve-stale refresh behave as prefetch A serve-stale refresh is similar to a prefetch, the only difference is when it triggers. Where a prefetch is done when an RRset is about to expire, a serve-stale refresh is done when the RRset is already stale. This means that the check for the stale-refresh window needs to move into query_stale_refresh(). We need to clear the DNS_DBFIND_STALEENABLED option at the same places as where we clear DNS_DBFIND_STALETIMEOUT. Now that serve-stale refresh acts the same as prefetch, there is no worry that the same rdataset is added to the message twice. This makes some code obsolete, specifically where we need to clear rdatasets from the message. (cherry picked from commit a66b04c8d46505fc3a9918dd8b7f589ef6b89ff3) --- lib/dns/include/dns/rdataset.h | 11 +- lib/ns/include/ns/query.h | 2 - lib/ns/query.c | 199 +++++++++------------------------ 3 files changed, 54 insertions(+), 158 deletions(-) diff --git a/lib/dns/include/dns/rdataset.h b/lib/dns/include/dns/rdataset.h index e81e195f90..37e1bc689b 100644 --- a/lib/dns/include/dns/rdataset.h +++ b/lib/dns/include/dns/rdataset.h @@ -245,11 +245,6 @@ struct dns_rdataset { * * \def DNS_RDATASETATTR_LOADORDER * Output the RRset in load order. - * - * \def DNS_RDATASETATTR_STALE_ADDED - * Set on rdatasets that were added during a stale-answer-client-timeout - * lookup. In other words, the RRset was added during a lookup of stale - * data and does not necessarily mean that the rdataset itself is stale. */ #define DNS_RDATASETATTR_NONE 0x00000000 /*%< No ordering. */ @@ -281,9 +276,9 @@ struct dns_rdataset { #define DNS_RDATASETATTR_STALE 0x01000000 #define DNS_RDATASETATTR_ANCIENT 0x02000000 #define DNS_RDATASETATTR_STALE_WINDOW 0x04000000 -#define DNS_RDATASETATTR_STALE_ADDED 0x08000000 -#define DNS_RDATASETATTR_KEEPCASE 0x10000000 -#define DNS_RDATASETATTR_STATICSTUB 0x20000000 +/* #define DNS_RDATASETATTR_STALE_ADDED 0x08000000 - Obsolete */ +#define DNS_RDATASETATTR_KEEPCASE 0x10000000 +#define DNS_RDATASETATTR_STATICSTUB 0x20000000 /*% * _OMITDNSSEC: diff --git a/lib/ns/include/ns/query.h b/lib/ns/include/ns/query.h index c38561fa21..ed3ad84693 100644 --- a/lib/ns/include/ns/query.h +++ b/lib/ns/include/ns/query.h @@ -174,7 +174,6 @@ struct ns_query { #define NS_QUERYATTR_RRL_CHECKED 0x010000 #define NS_QUERYATTR_REDIRECT 0x020000 #define NS_QUERYATTR_ANSWERED 0x040000 -#define NS_QUERYATTR_STALEOK 0x080000 typedef struct query_ctx query_ctx_t; @@ -202,7 +201,6 @@ struct query_ctx { bool authoritative; /* authoritative query? */ bool want_restart; /* CNAME chain or other * restart needed */ - bool refresh_rrset; /* stale RRset refresh needed */ bool need_wildcardproof; /* wildcard proof needed */ bool nxrewrite; /* negative answer from RPZ */ bool findcoveringnsec; /* lookup covering NSEC */ diff --git a/lib/ns/query.c b/lib/ns/query.c index 42fa82d357..5e09a9268f 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -141,9 +141,6 @@ /*% Was the client already sent a response? */ #define QUERY_ANSWERED(q) (((q)->attributes & NS_QUERYATTR_ANSWERED) != 0) -/*% Does the query allow stale data in the response? */ -#define QUERY_STALEOK(q) (((q)->attributes & NS_QUERYATTR_STALEOK) != 0) - /*% Does the query wants to check for stale RRset due to a timeout? */ #define QUERY_STALETIMEOUT(q) (((q)->dboptions & DNS_DBFIND_STALETIMEOUT) != 0) @@ -512,9 +509,6 @@ query_addwildcardproof(query_ctx_t *qctx, bool ispositive, bool nodata); static void query_addauth(query_ctx_t *qctx); -static void -query_clear_stale(ns_client_t *client); - /* * Increment query statistics counters. */ @@ -2292,10 +2286,6 @@ query_addrrset(query_ctx_t *qctx, dns_name_t **namep, if ((rdataset->attributes & DNS_RDATASETATTR_REQUIRED) != 0) { mrdataset->attributes |= DNS_RDATASETATTR_REQUIRED; } - if ((rdataset->attributes & DNS_RDATASETATTR_STALE_ADDED) != 0) - { - mrdataset->attributes |= DNS_RDATASETATTR_STALE_ADDED; - } return; } else if (result == DNS_R_NXDOMAIN) { /* @@ -2829,6 +2819,40 @@ fetch_and_forget(ns_client_t *client, dns_name_t *qname, dns_rdatatype_t qtype, } } +static void +query_stale_refresh(ns_client_t *client, dns_name_t *qname, + dns_rdataset_t *rdataset) { + CTRACE(ISC_LOG_DEBUG(3), "query_stale_refresh"); + + bool stale_refresh_window = + (STALE_WINDOW(rdataset) && + (client->query.dboptions & DNS_DBFIND_STALEENABLED) != 0); + if (FETCH_RECTYPE_STALE_REFRESH(client) != NULL || + (client->query.dboptions & DNS_DBFIND_STALETIMEOUT) == 0 || + !STALE(rdataset) || stale_refresh_window) + { + return; + } + + char namebuf[DNS_NAME_FORMATSIZE]; + char typebuf[DNS_RDATATYPE_FORMATSIZE]; + dns_name_format(qname, namebuf, sizeof(namebuf)); + dns_rdatatype_format(client->query.qtype, typebuf, sizeof(typebuf)); + isc_log_write(ns_lctx, NS_LOGCATEGORY_SERVE_STALE, NS_LOGMODULE_QUERY, + ISC_LOG_INFO, + "%s %s stale answer used, an attempt " + "to refresh the RRset will still be " + "made", + namebuf, typebuf); + + client->query.dboptions &= ~(DNS_DBFIND_STALETIMEOUT | + DNS_DBFIND_STALEOK | + DNS_DBFIND_STALEENABLED); + + fetch_and_forget(client, qname, client->query.qtype, + RECTYPE_STALE_REFRESH); +} + static void query_prefetch(ns_client_t *client, dns_name_t *qname, dns_rdataset_t *rdataset) { @@ -2839,6 +2863,8 @@ query_prefetch(ns_client_t *client, dns_name_t *qname, rdataset->ttl > client->view->prefetch_trigger || (rdataset->attributes & DNS_RDATASETATTR_PREFETCH) == 0) { + /* maybe refresh stale data */ + query_stale_refresh(client, qname, rdataset); return; } @@ -2847,30 +2873,8 @@ query_prefetch(ns_client_t *client, dns_name_t *qname, dns_rdataset_clearprefetch(rdataset); ns_stats_increment(client->manager->sctx->nsstats, ns_statscounter_prefetch); -} -static void -query_stale_refresh(ns_client_t *client) { - dns_name_t *qname; - - CTRACE(ISC_LOG_DEBUG(3), "query_stale_refresh"); - - if (FETCH_RECTYPE_STALE_REFRESH(client) != NULL) { - return; - } - - client->query.dboptions &= ~(DNS_DBFIND_STALETIMEOUT | - DNS_DBFIND_STALEOK | - DNS_DBFIND_STALEENABLED); - - if (client->query.origqname != NULL) { - qname = client->query.origqname; - } else { - qname = client->query.qname; - } - - fetch_and_forget(client, qname, client->query.qtype, - RECTYPE_STALE_REFRESH); + return; } static void @@ -6068,19 +6072,19 @@ query_lookup(query_ctx_t *qctx) { qctx->client->query.dboptions |= DNS_DBFIND_STALETIMEOUT; } - dboptions = qctx->client->query.dboptions; - if (!qctx->is_zone && qctx->findcoveringnsec && - (qctx->type != dns_rdatatype_null || !dns_name_istat(rpzqname))) - { - dboptions |= DNS_DBFIND_COVERINGNSEC; - } - (void)dns_db_getservestalerefresh(qctx->client->view->cachedb, &stale_refresh); if (stale_refresh > 0 && dns_view_staleanswerenabled(qctx->client->view)) { - dboptions |= DNS_DBFIND_STALEENABLED; + qctx->client->query.dboptions |= DNS_DBFIND_STALEENABLED; + } + + dboptions = qctx->client->query.dboptions; + if (!qctx->is_zone && qctx->findcoveringnsec && + (qctx->type != dns_rdatatype_null || !dns_name_istat(rpzqname))) + { + dboptions |= DNS_DBFIND_COVERINGNSEC; } result = dns_db_findext(qctx->db, rpzqname, qctx->version, qctx->type, @@ -6235,14 +6239,6 @@ query_lookup(query_ctx_t *qctx) { * Immediately return the stale answer, start a * resolver fetch to refresh the data in cache. */ - isc_log_write( - ns_lctx, NS_LOGCATEGORY_SERVE_STALE, - NS_LOGMODULE_QUERY, ISC_LOG_INFO, - "%s %s stale answer used, an attempt " - "to refresh the RRset will still be " - "made", - namebuf, typebuf); - qctx->refresh_rrset = STALE(qctx->rdataset); if (stale_found) { dns_ede_add( &qctx->client->edectx, ede, @@ -6255,76 +6251,12 @@ query_lookup(query_ctx_t *qctx) { } } - if (stale_timeout && (answer_found || stale_found)) { - /* - * Mark RRsets that we are adding to the client message on a - * lookup during 'stale-answer-client-timeout', so we can - * clean it up if needed when we resume from recursion. - */ - qctx->client->query.attributes |= NS_QUERYATTR_STALEOK; - qctx->rdataset->attributes |= DNS_RDATASETATTR_STALE_ADDED; - } - result = query_gotanswer(qctx, result); cleanup: return result; } -/* - * Clear all rdatasets from the message that are in the given section and - * that have the 'attr' attribute set. - */ -static void -message_clearrdataset(dns_message_t *msg, unsigned int attr) { - unsigned int i; - dns_name_t *name, *next_name; - dns_rdataset_t *rds, *next_rds; - - /* - * Clean up name lists by calling the rdataset disassociate function. - */ - for (i = DNS_SECTION_ANSWER; i < DNS_SECTION_MAX; i++) { - name = ISC_LIST_HEAD(msg->sections[i]); - while (name != NULL) { - next_name = ISC_LIST_NEXT(name, link); - - rds = ISC_LIST_HEAD(name->list); - while (rds != NULL) { - next_rds = ISC_LIST_NEXT(rds, link); - if ((rds->attributes & attr) != attr) { - rds = next_rds; - continue; - } - ISC_LIST_UNLINK(name->list, rds, link); - INSIST(dns_rdataset_isassociated(rds)); - dns_rdataset_disassociate(rds); - isc_mempool_put(msg->rdspool, rds); - rds = next_rds; - } - - if (ISC_LIST_EMPTY(name->list)) { - ISC_LIST_UNLINK(msg->sections[i], name, link); - if (dns_name_dynamic(name)) { - dns_name_free(name, msg->mctx); - } - isc_mempool_put(msg->namepool, name); - } - - name = next_name; - } - } -} - -/* - * Clear any rdatasets from the client's message that were added on a lookup - * due to a client timeout. - */ -static void -query_clear_stale(ns_client_t *client) { - message_clearrdataset(client->message, DNS_RDATASETATTR_STALE_ADDED); -} - /* * Event handler to resume processing a query after recursion, or when a * client timeout is triggered. If the query has timed out or been cancelled @@ -6356,6 +6288,7 @@ fetch_callback(void *arg) { client->query.attributes |= NS_QUERYATTR_RECURSIONOK; } client->query.dboptions &= ~DNS_DBFIND_STALETIMEOUT; + client->query.dboptions &= ~DNS_DBFIND_STALEENABLED; LOCK(&client->query.fetchlock); INSIST(FETCH_RECTYPE_NORMAL(client) == resp->fetch || @@ -8208,24 +8141,6 @@ query_addanswer(query_ctx_t *qctx) { CALL_HOOK(NS_QUERY_ADDANSWER_BEGIN, qctx); - /* - * On normal lookups, clear any rdatasets that were added on a - * lookup due to stale-answer-client-timeout. Do not clear if we - * are going to refresh the RRset, because the stale contents are - * prioritized. - */ - if (QUERY_STALEOK(&qctx->client->query) && - !QUERY_STALETIMEOUT(&qctx->client->query) && !qctx->refresh_rrset) - { - CCTRACE(ISC_LOG_DEBUG(3), "query_clear_stale"); - query_clear_stale(qctx->client); - /* - * We can clear the attribute to prevent redundant clearing - * in subsequent lookups. - */ - qctx->client->query.attributes &= ~NS_QUERYATTR_STALEOK; - } - if (qctx->dns64) { result = query_dns64(qctx); qctx->noqname = NULL; @@ -8259,9 +8174,7 @@ query_addanswer(query_ctx_t *qctx) { query_filter64(qctx); ns_client_putrdataset(qctx->client, &qctx->rdataset); } else { - if (!qctx->is_zone && RECURSIONOK(qctx->client) && - !QUERY_STALETIMEOUT(&qctx->client->query)) - { + if (!qctx->is_zone && RECURSIONOK(qctx->client)) { query_prefetch(qctx->client, qctx->fname, qctx->rdataset); } @@ -10430,6 +10343,10 @@ query_ncache(query_ctx_t *qctx, isc_result_t result) { } } + if (!qctx->is_zone && RECURSIONOK(qctx->client)) { + query_stale_refresh(qctx->client, qctx->fname, qctx->rdataset); + } + return query_nodata(qctx, result); cleanup: @@ -11831,20 +11748,6 @@ ns_query_done(query_ctx_t *qctx) { query_send(qctx->client); - if (qctx->refresh_rrset) { - /* - * If we reached this point then it means that we have found a - * stale RRset entry in cache and BIND is configured to allow - * queries to be answered with stale data if no active RRset - * is available, i.e. "stale-anwer-client-timeout 0". But, we - * still need to refresh the RRset. To prevent adding duplicate - * RRsets, clear the RRsets from the message before doing the - * refresh. - */ - message_clearrdataset(qctx->client->message, 0); - query_stale_refresh(qctx->client); - } - qctx->detach_client = true; return qctx->result;