From b32512a2329e66bf6eec02c97945a6b3709c805b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Sun, 2 Feb 2025 11:44:00 +0100 Subject: [PATCH 1/3] In cache, set rdataset TTL to 0 when the header is not active When the header has been marked as ANCIENT, but the ttl hasn't been reset (this happens in couple of places), the rdataset TTL would be set to the header timestamp instead to a reasonable TTL value. Since this header has been already expired (ANCIENT is set), set the rdataset TTL to 0 and don't reuse this field to print the expiration time when dumping the cache. Instead of printing the time, we now just print 'expired (awaiting cleanup'. (cherry picked from commit 1bbb57f81b50bb594327428938a129a51d8ca493) --- bin/tests/system/serve-stale/tests.sh | 10 +++++----- lib/dns/masterdump.c | 10 +--------- lib/dns/qpcache.c | 2 +- lib/dns/rbtdb.c | 2 +- 4 files changed, 8 insertions(+), 16 deletions(-) diff --git a/bin/tests/system/serve-stale/tests.sh b/bin/tests/system/serve-stale/tests.sh index dc5e7d9d9f..6dc3c20308 100755 --- a/bin/tests/system/serve-stale/tests.sh +++ b/bin/tests/system/serve-stale/tests.sh @@ -1649,7 +1649,7 @@ if [ $ret != 0 ]; then echo_i "failed"; fi status=$((status + ret)) # Check that expired records are not dumped. ret=0 -grep "; expired since .* (awaiting cleanup)" ns5/named_dump.db.test$n && ret=1 +grep "; expired (awaiting cleanup)" ns5/named_dump.db.test$n && ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi status=$((status + ret)) @@ -1665,13 +1665,13 @@ status=$((status + ret)) echo_i "check rndc dump expired data.example ($n)" ret=0 awk '/; expired/ { x=$0; getline; print x, $0}' ns5/named_dump.db.test$n \ - | grep "; expired since .* (awaiting cleanup) data\.example\..*A text record with a 2 second ttl" >/dev/null 2>&1 || ret=1 + | grep "; expired (awaiting cleanup) data\.example\..*A text record with a 2 second ttl" >/dev/null 2>&1 || ret=1 awk '/; expired/ { x=$0; getline; print x, $0}' ns5/named_dump.db.test$n \ - | grep "; expired since .* (awaiting cleanup) nodata\.example\." >/dev/null 2>&1 || ret=1 + | grep "; expired (awaiting cleanup) nodata\.example\." >/dev/null 2>&1 || ret=1 awk '/; expired/ { x=$0; getline; print x, $0}' ns5/named_dump.db.test$n \ - | grep "; expired since .* (awaiting cleanup) nxdomain\.example\." >/dev/null 2>&1 || ret=1 + | grep "; expired (awaiting cleanup) nxdomain\.example\." >/dev/null 2>&1 || ret=1 awk '/; expired/ { x=$0; getline; print x, $0}' ns5/named_dump.db.test$n \ - | grep "; expired since .* (awaiting cleanup) othertype\.example\." >/dev/null 2>&1 || ret=1 + | grep "; expired (awaiting cleanup) othertype\.example\." >/dev/null 2>&1 || ret=1 # Also make sure the not expired data does not have an expired comment. awk '/; authanswer/ { x=$0; getline; print x, $0}' ns5/named_dump.db.test$n \ | grep "; authanswer longttl\.example.*A text record with a 600 second ttl" >/dev/null 2>&1 || ret=1 diff --git a/lib/dns/masterdump.c b/lib/dns/masterdump.c index 9c98ffd552..7b8346320a 100644 --- a/lib/dns/masterdump.c +++ b/lib/dns/masterdump.c @@ -1164,15 +1164,7 @@ again: if (STALE(rds)) { fprintf(f, "; stale\n"); } else if (ANCIENT(rds)) { - isc_buffer_t b; - char buf[sizeof("YYYYMMDDHHMMSS")]; - memset(buf, 0, sizeof(buf)); - isc_buffer_init(&b, buf, sizeof(buf) - 1); - dns_time64_totext((uint64_t)rds->ttl, &b); - fprintf(f, - "; expired since %s " - "(awaiting cleanup)\n", - buf); + fprintf(f, "; expired (awaiting cleanup)\n"); } result = dump_rdataset(mctx, name, rds, ctx, buffer, f); if (result != ISC_R_SUCCESS) { diff --git a/lib/dns/qpcache.c b/lib/dns/qpcache.c index 01b8bf1321..83ce178ee0 100644 --- a/lib/dns/qpcache.c +++ b/lib/dns/qpcache.c @@ -1103,7 +1103,7 @@ bindrdataset(qpcache_t *qpdb, qpcnode_t *node, dns_slabheader_t *header, rdataset->attributes |= DNS_RDATASETATTR_STALE; } else if (!ACTIVE(header, now)) { rdataset->attributes |= DNS_RDATASETATTR_ANCIENT; - rdataset->ttl = header->ttl; + rdataset->ttl = 0; } rdataset->count = atomic_fetch_add_relaxed(&header->count, 1); diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index a708f7b7fe..1a53ede503 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -2194,7 +2194,7 @@ dns__rbtdb_bindrdataset(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, rdataset->attributes |= DNS_RDATASETATTR_STALE; } else if (IS_CACHE(rbtdb) && !ACTIVE(header, now)) { rdataset->attributes |= DNS_RDATASETATTR_ANCIENT; - rdataset->ttl = header->ttl; + rdataset->ttl = 0; } rdataset->count = atomic_fetch_add_relaxed(&header->count, 1); From 4b114838de3291c3d555935f2a42f62383b917d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Sun, 2 Feb 2025 13:38:04 +0100 Subject: [PATCH 2/3] Add better ZEROTTL handling in bindrdataset() If we know that the header has ZEROTTL set, the server should never send stale records for it and the TTL should never be anything else than 0. The comment was already there, but the code was not matching the comment. (cherry picked from commit cfee6aa56557f9fde8bd47949c5165edaf350113) --- lib/dns/qpcache.c | 3 ++- lib/dns/rbtdb.c | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/dns/qpcache.c b/lib/dns/qpcache.c index 83ce178ee0..25a93431d2 100644 --- a/lib/dns/qpcache.c +++ b/lib/dns/qpcache.c @@ -1058,7 +1058,7 @@ bindrdataset(qpcache_t *qpdb, qpcnode_t *node, dns_slabheader_t *header, * (these records should not be cached anyway). */ - if (KEEPSTALE(qpdb) && stale_ttl > now) { + if (!ZEROTTL(header) && KEEPSTALE(qpdb) && stale_ttl > now) { stale = true; } else { /* @@ -1073,6 +1073,7 @@ bindrdataset(qpcache_t *qpdb, qpcnode_t *node, dns_slabheader_t *header, rdataset->rdclass = qpdb->common.rdclass; rdataset->type = DNS_TYPEPAIR_TYPE(header->type); rdataset->covers = DNS_TYPEPAIR_COVERS(header->type); + rdataset->ttl = !ZEROTTL(header) ? header->ttl - now : 0; rdataset->ttl = header->ttl - now; rdataset->trust = header->trust; rdataset->resign = 0; diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index 1a53ede503..cd81fe4550 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -2150,7 +2150,7 @@ dns__rbtdb_bindrdataset(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, * (these records should not be cached anyway). */ - if (KEEPSTALE(rbtdb) && stale_ttl > now) { + if (!ZEROTTL(header) && KEEPSTALE(rbtdb) && stale_ttl > now) { stale = true; } else { /* @@ -2165,7 +2165,7 @@ dns__rbtdb_bindrdataset(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, rdataset->rdclass = rbtdb->common.rdclass; rdataset->type = DNS_TYPEPAIR_TYPE(header->type); rdataset->covers = DNS_TYPEPAIR_COVERS(header->type); - rdataset->ttl = header->ttl - now; + rdataset->ttl = !ZEROTTL(header) ? header->ttl - now : 0; rdataset->trust = header->trust; if (NEGATIVE(header)) { From 302aca809d15908e932646cc0be51e133e0343bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Sun, 2 Feb 2025 13:56:37 +0100 Subject: [PATCH 3/3] Expand the usage of mark_ancient() helper functions When the mark_ancient() helper function was introduced, couple of places with duplicate (or almost duplicate) code was missed. Move the mark_ancient() function closer to the top of the file, and correctly use it in places that mark the header as ANCIENT. (cherry picked from commit 58179e6a192998a49732df57847091e42c654f0b) --- lib/dns/qpcache.c | 25 ++++++++++--------------- lib/dns/rbt-cachedb.c | 12 +++--------- lib/dns/rbtdb.c | 14 +++++++------- lib/dns/rbtdb_p.h | 2 ++ 4 files changed, 22 insertions(+), 31 deletions(-) diff --git a/lib/dns/qpcache.c b/lib/dns/qpcache.c index 25a93431d2..633d527a80 100644 --- a/lib/dns/qpcache.c +++ b/lib/dns/qpcache.c @@ -954,15 +954,20 @@ setttl(dns_slabheader_t *header, dns_ttl_t newttl) { } } +static void +mark_ancient(dns_slabheader_t *header) { + setttl(header, 0); + mark(header, DNS_SLABHEADERATTR_ANCIENT); + HEADERNODE(header)->dirty = 1; +} + /* * Caller must hold the node (write) lock. */ static void expireheader(dns_slabheader_t *header, isc_rwlocktype_t *nlocktypep, isc_rwlocktype_t *tlocktypep, dns_expire_t reason DNS__DB_FLARG) { - setttl(header, 0); - mark(header, DNS_SLABHEADERATTR_ANCIENT); - HEADERNODE(header)->dirty = 1; + mark_ancient(header); if (isc_refcount_current(&HEADERNODE(header)->erefs) == 0) { qpcache_t *qpdb = (qpcache_t *)header->db; @@ -1271,8 +1276,7 @@ check_stale_header(qpcnode_t *node, dns_slabheader_t *header, } dns_slabheader_destroy(&header); } else { - mark(header, DNS_SLABHEADERATTR_ANCIENT); - HEADERNODE(header)->dirty = 1; + mark_ancient(header); *header_prev = header; } } else { @@ -2234,8 +2238,7 @@ findrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, * non-zero. This is so because 'node' is an * argument to the function. */ - mark(header, DNS_SLABHEADERATTR_ANCIENT); - HEADERNODE(header)->dirty = 1; + mark_ancient(header); } } else if (EXISTS(header) && !ANCIENT(header)) { if (header->type == matchtype) { @@ -2586,13 +2589,6 @@ qpdb_destroy(dns_db_t *arg) { qpcache_detach(&qpdb); } -static void -mark_ancient(dns_slabheader_t *header) { - setttl(header, 0); - mark(header, DNS_SLABHEADERATTR_ANCIENT); - HEADERNODE(header)->dirty = 1; -} - /*% * Clean up dead nodes. These are nodes which have no references, and * have no data. They are dead but we could not or chose not to delete @@ -3163,7 +3159,6 @@ find_header: newheader->next = topheader->next; newheader->down = topheader; topheader->next = newheader; - qpnode->dirty = 1; mark_ancient(header); if (sigheader != NULL) { mark_ancient(sigheader); diff --git a/lib/dns/rbt-cachedb.c b/lib/dns/rbt-cachedb.c index f449e90cfd..7a77739869 100644 --- a/lib/dns/rbt-cachedb.c +++ b/lib/dns/rbt-cachedb.c @@ -417,9 +417,7 @@ check_stale_header(dns_rbtnode_t *node, dns_slabheader_t *header, } dns_slabheader_destroy(&header); } else { - dns__rbtdb_mark(header, - DNS_SLABHEADERATTR_ANCIENT); - RBTDB_HEADERNODE(header)->dirty = 1; + dns__rbtdb_mark_ancient(header); *header_prev = header; } } else { @@ -1401,9 +1399,7 @@ cache_findrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, * non-zero. This is so because 'node' is an * argument to the function. */ - dns__rbtdb_mark(header, - DNS_SLABHEADERATTR_ANCIENT); - RBTDB_HEADERNODE(header)->dirty = 1; + dns__rbtdb_mark_ancient(header); } } else if (EXISTS(header) && !ANCIENT(header)) { if (header->type == matchtype) { @@ -1589,9 +1585,7 @@ void dns__cacherbt_expireheader(dns_slabheader_t *header, isc_rwlocktype_t *tlocktypep, dns_expire_t reason DNS__DB_FLARG) { - dns__rbtdb_setttl(header, 0); - dns__rbtdb_mark(header, DNS_SLABHEADERATTR_ANCIENT); - RBTDB_HEADERNODE(header)->dirty = 1; + dns__rbtdb_mark_ancient(header); if (isc_refcount_current(&RBTDB_HEADERNODE(header)->references) == 0) { isc_rwlocktype_t nlocktype = isc_rwlocktype_write; diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index cd81fe4550..cb69d6be0c 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -857,8 +857,8 @@ dns__rbtdb_mark(dns_slabheader_t *header, uint_least16_t flag) { } } -static void -mark_ancient(dns_slabheader_t *header) { +void +dns__rbtdb_mark_ancient(dns_slabheader_t *header) { dns__rbtdb_setttl(header, 0); dns__rbtdb_mark(header, DNS_SLABHEADERATTR_ANCIENT); RBTDB_HEADERNODE(header)->dirty = 1; @@ -2604,7 +2604,7 @@ dns__rbtdb_add(dns_rbtdb_t *rbtdb, dns_rbtnode_t *rbtnode, topheader != NULL; topheader = topheader->next) { - mark_ancient(topheader); + dns__rbtdb_mark_ancient(topheader); } goto find_header; } @@ -2667,7 +2667,7 @@ dns__rbtdb_add(dns_rbtdb_t *rbtdb, dns_rbtnode_t *rbtnode, * The new rdataset is better. Expire the * ncache entry. */ - mark_ancient(topheader); + dns__rbtdb_mark_ancient(topheader); topheader = NULL; goto find_header; } @@ -3013,9 +3013,9 @@ find_header: changed->dirty = true; } if (rbtversion == NULL) { - mark_ancient(header); + dns__rbtdb_mark_ancient(header); if (sigheader != NULL) { - mark_ancient(sigheader); + dns__rbtdb_mark_ancient(sigheader); } } if (rbtversion != NULL && !header_nx) { @@ -3119,7 +3119,7 @@ find_header: expireheader = newheader; } - mark_ancient(expireheader); + dns__rbtdb_mark_ancient(expireheader); /* * FIXME: In theory, we should mark the RRSIG * and the header at the same time, but there is diff --git a/lib/dns/rbtdb_p.h b/lib/dns/rbtdb_p.h index 45ab1bcc90..1b2c7dca2e 100644 --- a/lib/dns/rbtdb_p.h +++ b/lib/dns/rbtdb_p.h @@ -486,6 +486,8 @@ dns__zonerbt_addwildcards(dns_rbtdb_t *rbtdb, const dns_name_t *name, * Cache-specific functions that are called from rbtdb.c */ void +dns__rbtdb_mark_ancient(dns_slabheader_t *header); +void dns__cacherbt_expireheader(dns_slabheader_t *header, isc_rwlocktype_t *tlocktypep, dns_expire_t reason DNS__DB_FLARG);