From: Ondřej Surý <ondrej@isc.org>
Date: Sat, 20 Jun 2026 07:33:55 +0000 (+0200)
Subject: Keep in-flight cache headers safe from LRU eviction during add
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=057042304dfedd3be1857abf600a4c62601eb637;p=thirdparty%2Fbind9.git

Keep in-flight cache headers safe from LRU eviction during add

When the cache is over its size limit, qpcache_miss() runs LRU eviction
while add() is still in progress.  Eviction removes a header together
with its RRSIG/covered 'related' partner, so it could free a header the
add still needs -- the new header, the partner it was just paired with,
or the one about to be displaced for max-types-per-name.  With 'related'
now a counted reference, that became a use-after-free reachable under
sustained load.

Run the eviction last, after the new header is linked, bound and any
over-limit header removed, and skip the new header and its partner in
the eviction loop (the partner is marked visited so the SIEVE hand still
advances).  Per-header removal is factored into header_delete(),
expire_header() and flush_node().
---

diff --git a/bin/tests/system/reclimit/tests.sh b/bin/tests/system/reclimit/tests.sh
index 698251090a7..f7b86200115 100644
--- a/bin/tests/system/reclimit/tests.sh
+++ b/bin/tests/system/reclimit/tests.sh
@@ -270,7 +270,7 @@ check_manytypes() (
     exit 1
   fi
 
-  if [ -n "${neq_ttl}" ] && grep -q "^$exname.[[:space:]]*${neq_ttl}[[:space:]]*IN[[:space:]]*$type" "dig.out.$i.$type.test$n"; then
+  if [ -n "${neq_ttl}" ] && grep -q "^$exname.[[:space:]]*${neq_ttl}[[:space:]]*IN[[:space:]]*$extype" "dig.out.$i.$type.test$n"; then
     exit 1
   fi
 
@@ -337,17 +337,20 @@ if [ $status -ne 0 ]; then exit 1; fi
 
 n=$((n + 1))
 ret=0
-echo_i "checking that NXDOMAIN names over the max-types-per-name limit don't get cached ($n)"
+echo_i "checking that NXDOMAIN names over the max-types-per-name limit get cached ($n)"
 
-# Query for 10 NXDOMAIN types
+# The cache already holds 10 positive types (TYPE65280-TYPE65289), filling
+# the per-name limit. Querying 10 NXDOMAIN types now exceeds the limit, but
+# the freshly fetched entry is always cached - it evicts an older type
+# rather than being dropped itself.
 for ntype in $(seq 65270 65279); do
   check_manytypes 1 manytypes.big "TYPE${ntype}" NOERROR big SOA 120 || ret=1
 done
 # Wait at least 1 second
 sleep 1
-# Query for 10 NXDOMAIN types again - these should not be cached
+# Query for 10 NXDOMAIN types again - these should now be cached
 for ntype in $(seq 65270 65279); do
-  check_manytypes 2 manytypes.big "TYPE${ntype}" NOERROR big SOA 120 || ret=1
+  check_manytypes 2 manytypes.big "TYPE${ntype}" NOERROR big SOA "" 120 || ret=1
 done
 
 if [ $ret -ne 0 ]; then echo_i "failed"; fi
diff --git a/lib/dns/qpcache.c b/lib/dns/qpcache.c
index 3007f796094..210210747f6 100644
--- a/lib/dns/qpcache.c
+++ b/lib/dns/qpcache.c
@@ -411,8 +411,7 @@ cleanup_deadnodes_cb(void *arg);
  */
 
 static size_t
-expireheader(dns_slabheader_t *header, isc_rwlocktype_t *nlocktypep,
-	     isc_rwlocktype_t *tlocktypep, dns_expire_t reason DNS__DB_FLARG);
+header_delete(qpcnode_t *node, dns_slabheader_t *header);
 
 static size_t
 rdataset_size(dns_slabheader_t *header) {
@@ -424,8 +423,29 @@ rdataset_size(dns_slabheader_t *header) {
 }
 
 static void
-expire_lru_headers(qpcache_t *qpdb, uint32_t idx, size_t requested,
-		   isc_rwlocktype_t *nlocktypep,
+flush_node(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t *nlocktypep,
+	   isc_rwlocktype_t *tlocktypep, dns_expire_t reason DNS__DB_FLARG);
+
+static size_t
+expire_header(qpcache_t *qpdb, qpcnode_t *node, dns_slabheader_t *header,
+	      isc_rwlocktype_t *nlocktypep,
+	      isc_rwlocktype_t *tlocktypep DNS__DB_FLARG) {
+	size_t expired = 0;
+
+	if (header->related != NULL) {
+		expired += header_delete(node, header->related);
+	}
+	expired += header_delete(node, header);
+
+	flush_node(qpdb, node, nlocktypep, tlocktypep,
+		   dns_expire_lru DNS__DB_FLARG_PASS);
+
+	return expired;
+}
+
+static void
+expire_lru_headers(qpcache_t *qpdb, dns_slabheader_t *newheader, uint32_t idx,
+		   size_t requested, isc_rwlocktype_t *nlocktypep,
 		   isc_rwlocktype_t *tlocktypep DNS__DB_FLARG) {
 	size_t expired = 0;
 
@@ -436,16 +456,15 @@ expire_lru_headers(qpcache_t *qpdb, uint32_t idx, size_t requested,
 			return;
 		}
 
-		dns_slabheader_t *related = header->related;
+		/* newheader is protected from removal */
+		if (header == newheader || header->related == newheader) {
+			return;
+		}
 
-		expired += expireheader(header, nlocktypep, tlocktypep,
-					dns_expire_lru DNS__DB_FLARG_PASS);
+		qpcnode_t *node = HEADERNODE(header);
 
-		if (related != NULL) {
-			expired +=
-				expireheader(related, nlocktypep, tlocktypep,
-					     dns_expire_lru DNS__DB_FLARG_PASS);
-		}
+		expired += expire_header(qpdb, node, header, nlocktypep,
+					 tlocktypep DNS__DB_FLARG_PASS);
 
 	} while (expired < requested);
 }
@@ -471,7 +490,7 @@ qpcache_miss(qpcache_t *qpdb, dns_slabheader_t *newheader,
 			     dns_name_size(&HEADERNODE(newheader)->name)) +
 			rdataset_size(newheader) + QP_SAFETY_MARGIN;
 
-		expire_lru_headers(qpdb, idx, purgesize, nlocktypep,
+		expire_lru_headers(qpdb, newheader, idx, purgesize, nlocktypep,
 				   tlocktypep DNS__DB_FLARG_PASS);
 	}
 
@@ -768,13 +787,14 @@ setttl(dns_slabheader_t *header, isc_stdtime_t newts) {
 	header->expire = newts;
 }
 
-static void
+static size_t
 header_delete(qpcnode_t *node, dns_slabheader_t *header) {
 	/* The slabheader has already been removed from the node headers */
 	if (cds_list_empty(&header->headers_link)) {
-		return;
+		return 0;
 	}
 
+	size_t expired = rdataset_size(header);
 	qpcache_t *qpdb = node->qpdb;
 
 	cds_list_del_init(&header->headers_link);
@@ -789,52 +809,47 @@ header_delete(qpcnode_t *node, dns_slabheader_t *header) {
 
 	if (header->related != NULL) {
 		INSIST(header->related->related == header);
-		header->related->related = NULL;
-		header->related = NULL;
+		dns_slabheader_detach(&header->related->related);
+		dns_slabheader_detach(&header->related);
 	}
 
 	dns_slabheader_detach(&header);
+
+	return expired;
 }
 
 /*
  * Caller must hold the node (write) lock.
  */
-static size_t
-expireheader(dns_slabheader_t *header, isc_rwlocktype_t *nlocktypep,
-	     isc_rwlocktype_t *tlocktypep, dns_expire_t reason DNS__DB_FLARG) {
-	size_t expired = rdataset_size(header);
-	qpcnode_t *node = HEADERNODE(header);
-
-	header_delete(node, header);
-
-	if (isc_refcount_current(&node->erefs) == 0) {
-		qpcache_t *qpdb = node->qpdb;
 
-		/*
-		 * If no one else is using the node, we can clean it up now.
-		 * We first need to gain a new reference to the node to meet a
-		 * requirement of qpcnode_release().
-		 */
-		qpcnode_acquire(qpdb, node, *nlocktypep,
-				*tlocktypep DNS__DB_FLARG_PASS);
-		qpcnode_release(qpdb, node, nlocktypep,
-				tlocktypep DNS__DB_FLARG_PASS);
+static void
+flush_node(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t *nlocktypep,
+	   isc_rwlocktype_t *tlocktypep, dns_expire_t reason DNS__DB_FLARG) {
+	if (isc_refcount_current(&node->erefs) != 0) {
+		return;
+	}
 
-		if (qpdb->cachestats == NULL) {
-			return expired;
-		}
+	/*
+	 * If no one else is using the node, we can clean it up now.
+	 * We first need to gain a new reference to the node to meet a
+	 * requirement of qpcnode_release().
+	 */
+	qpcnode_acquire(qpdb, node, *nlocktypep,
+			*tlocktypep DNS__DB_FLARG_PASS);
+	qpcnode_release(qpdb, node, nlocktypep, tlocktypep DNS__DB_FLARG_PASS);
 
-		switch (reason) {
-		case dns_expire_lru:
-			isc_stats_increment(qpdb->cachestats,
-					    dns_cachestatscounter_deletelru);
-			break;
-		default:
-			break;
-		}
+	if (qpdb->cachestats == NULL) {
+		return;
 	}
 
-	return expired;
+	switch (reason) {
+	case dns_expire_lru:
+		isc_stats_increment(qpdb->cachestats,
+				    dns_cachestatscounter_deletelru);
+		break;
+	default:
+		break;
+	}
 }
 
 static void
@@ -1868,16 +1883,8 @@ qpcnode_expiredata(dns_dbnode_t *node, void *data) {
 
 	isc_rwlock_t *nlock = &qpdb->buckets[qpnode->locknum].lock;
 	NODE_WRLOCK(nlock, &nlocktype);
-	if (!cds_list_empty(&header->headers_link)) {
-		dns_slabheader_t *related = header->related;
-
-		(void)expireheader(header, &nlocktype, &tlocktype,
-				   dns_expire_flush DNS__DB_FILELINE);
-		if (related != NULL) {
-			(void)expireheader(related, &nlocktype, &tlocktype,
-					   dns_expire_flush DNS__DB_FILELINE);
-		}
-	}
+	(void)expire_header(qpdb, qpnode, header, &nlocktype,
+			    &tlocktype DNS__DB_FILELINE);
 	NODE_UNLOCK(nlock, &nlocktype);
 	INSIST(tlocktype == isc_rwlocktype_none);
 }
@@ -2550,13 +2557,12 @@ add(qpcache_t *qpdb, qpcnode_t *qpnode, dns_slabheader_t *newheader,
 
 	if (related != NULL) {
 		INSIST(related->related == NULL);
-		related->related = newheader;
-		newheader->related = related;
+		/* protect the related from LRU eviction */
+		qpcache_hit(qpdb, related);
+		related->related = dns_slabheader_ref(newheader);
+		newheader->related = dns_slabheader_ref(related);
 	}
 
-	qpcache_miss(qpdb, newheader, &nlocktype,
-		     &tlocktype DNS__DB_FLARG_PASS);
-
 	bindrdataset(qpdb, qpnode, newheader, now, nlocktype, tlocktype,
 		     addedrdataset DNS__DB_FLARG_PASS);
 
@@ -2572,6 +2578,9 @@ add(qpcache_t *qpdb, qpcnode_t *qpnode, dns_slabheader_t *newheader,
 		}
 	}
 
+	qpcache_miss(qpdb, newheader, &nlocktype,
+		     &tlocktype DNS__DB_FLARG_PASS);
+
 	/*
 	 * We've added a proof that a rdtype doesn't exist.
 	 *