From 6c54337f52bd59b63bb608ba1bd34c68bcf21110 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ondr=CC=8Cej=20Sury=CC=81?= Date: Mon, 25 Mar 2024 17:23:19 -0700 Subject: [PATCH] avoid a race in the qpzone getsigningtime() implementation the previous commit introduced a possible race in getsigningtime() where the rdataset header could change between being found on the heap and being bound. getsigningtime() now looks at the first element of the heap, gathers the locknum, locks the respective lock, and retrieves the header from the heap again. If the locknum has changed, it will rinse and repeat. Theoretically, this could spin forever, but practically, it almost never will as the heap changes on the zone are very rare. we simplify matters further by changing the dns_db_getsigningtime() API call. instead of passing back a bound rdataset, we pass back the information the caller actually needed: the resigning time, owner name and type of the rdataset that was first on the heap. --- bin/named/server.c | 19 +++---- bin/tests/system/dyndb/driver/db.c | 7 ++- lib/dns/db.c | 8 +-- lib/dns/include/dns/db.h | 21 ++++---- lib/dns/qpzone.c | 84 ++++++++++++++++++------------ lib/dns/rbt-zonedb.c | 20 +++---- lib/dns/zone.c | 44 +++++++--------- 7 files changed, 107 insertions(+), 96 deletions(-) diff --git a/bin/named/server.c b/bin/named/server.c index a543b839a9..62818950fb 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -15081,29 +15081,26 @@ named_server_zonestatus(named_server_t *server, isc_lex_t *lex, { dns_name_t *name; dns_fixedname_t fixed; - dns_rdataset_t next; + isc_stdtime_t resign; + dns_typepair_t typepair; - dns_rdataset_init(&next); name = dns_fixedname_initname(&fixed); - result = dns_db_getsigningtime(db, &next, name); + result = dns_db_getsigningtime(db, &resign, name, &typepair); if (result == ISC_R_SUCCESS) { char namebuf[DNS_NAME_FORMATSIZE]; char typebuf[DNS_RDATATYPE_FORMATSIZE]; + resign -= dns_zone_getsigresigninginterval(zone); + dns_name_format(name, namebuf, sizeof(namebuf)); - dns_rdatatype_format(next.covers, typebuf, - sizeof(typebuf)); + dns_rdatatype_format(DNS_TYPEPAIR_COVERS(typepair), + typebuf, sizeof(typebuf)); snprintf(resignbuf, sizeof(resignbuf), "%s/%s", namebuf, typebuf); - isc_time_set( - &resigntime, - next.resign - - dns_zone_getsigresigninginterval(zone), - 0); + isc_time_set(&resigntime, resign, 0); isc_time_formathttptimestamp(&resigntime, rtbuf, sizeof(rtbuf)); - dns_rdataset_disassociate(&next); } } diff --git a/bin/tests/system/dyndb/driver/db.c b/bin/tests/system/dyndb/driver/db.c index 521327d125..de26821b2c 100644 --- a/bin/tests/system/dyndb/driver/db.c +++ b/bin/tests/system/dyndb/driver/db.c @@ -363,14 +363,13 @@ setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, isc_stdtime_t resign) { } static isc_result_t -getsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, - dns_name_t *name DNS__DB_FLARG) { +getsigningtime(dns_db_t *db, isc_stdtime_t *resign, dns_name_t *name, + dns_typepair_t *type) { sampledb_t *sampledb = (sampledb_t *)db; REQUIRE(VALID_SAMPLEDB(sampledb)); - return (dns__db_getsigningtime(sampledb->rbtdb, rdataset, - name DNS__DB_FLARG_PASS)); + return (dns_db_getsigningtime(sampledb->rbtdb, resign, name, type)); } static dns_stats_t * diff --git a/lib/dns/db.c b/lib/dns/db.c index a5612e49fd..013065c26d 100644 --- a/lib/dns/db.c +++ b/lib/dns/db.c @@ -957,11 +957,11 @@ dns_db_setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, } isc_result_t -dns__db_getsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, - dns_name_t *name DNS__DB_FLARG) { +dns_db_getsigningtime(dns_db_t *db, isc_stdtime_t *resign, dns_name_t *name, + dns_typepair_t *typepair) { if (db->methods->getsigningtime != NULL) { - return ((db->methods->getsigningtime)(db, rdataset, - name DNS__DB_FLARG_PASS)); + return ((db->methods->getsigningtime)(db, resign, name, + typepair)); } return (ISC_R_NOTFOUND); } diff --git a/lib/dns/include/dns/db.h b/lib/dns/include/dns/db.h index 7643ec3deb..c8089b66c6 100644 --- a/lib/dns/include/dns/db.h +++ b/lib/dns/include/dns/db.h @@ -147,8 +147,9 @@ typedef struct dns_dbmethods { dns_dbnode_t **nodep DNS__DB_FLARG); isc_result_t (*setsigningtime)(dns_db_t *db, dns_rdataset_t *rdataset, isc_stdtime_t resign); - isc_result_t (*getsigningtime)(dns_db_t *db, dns_rdataset_t *rdataset, - dns_name_t *name DNS__DB_FLARG); + isc_result_t (*getsigningtime)(dns_db_t *db, isc_stdtime_t *resign, + dns_name_t *name, + dns_typepair_t *typepair); dns_stats_t *(*getrrsetstats)(dns_db_t *db); isc_result_t (*findnodeext)(dns_db_t *db, const dns_name_t *name, bool create, @@ -1585,19 +1586,19 @@ dns_db_setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, * \li #ISC_R_NOTIMPLEMENTED - Not supported by this DB implementation. */ -#define dns_db_getsigningtime(db, rdataset, name) \ - dns__db_getsigningtime(db, rdataset, name DNS__DB_FILELINE) isc_result_t -dns__db_getsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, - dns_name_t *name DNS__DB_FLARG); +dns_db_getsigningtime(dns_db_t *db, isc_stdtime_t *resign, + dns_name_t *foundname, dns_typepair_t *typepair); /*%< - * Return the rdataset with the earliest signing time in the zone. - * Note: the rdataset is version agnostic. + * Find the rdataset header with the earliest signing time in a zone + * database. Update 'foundname' and 'typepair' with its name and + * type, and update 'resign' with the time at which it is to be signed. * * Requires: * \li 'db' is a valid zone database. - * \li 'rdataset' to be initialized but not associated. - * \li 'name' to be NULL or have a buffer associated with it. + * \li 'resign' is not NULL. + * \li 'foundname' is not NULL. + * \li 'typepair' is not NULL. * * Returns: * \li #ISC_R_SUCCESS diff --git a/lib/dns/qpzone.c b/lib/dns/qpzone.c index e1c63e9d34..c8b9a7f9ee 100644 --- a/lib/dns/qpzone.c +++ b/lib/dns/qpzone.c @@ -188,7 +188,6 @@ struct qpzonedb { isc_loop_t *loop; struct rcu_head rcu_head; - isc_mutex_t heaplock; isc_heap_t *heap; /* Resigning heap */ dns_qpmulti_t *tree; /* Main QP trie for data storage */ @@ -468,7 +467,6 @@ free_db_rcu(struct rcu_head *rcu_head) { } isc_heap_destroy(&qpdb->heap); - isc_mutex_destroy(&qpdb->heaplock); if (qpdb->gluecachestats != NULL) { isc_stats_detach(&qpdb->gluecachestats); @@ -657,7 +655,6 @@ dns__qpzone_create(isc_mem_t *mctx, const dns_name_t *origin, dns_dbtype_t type, qpdb->common.update_listeners = cds_lfht_new(16, 16, 0, 0, NULL); - isc_mutex_init(&qpdb->heaplock); isc_heap_create(mctx, resign_sooner, set_index, 0, &qpdb->heap); qpdb->active = qpdb->node_lock_count; @@ -1268,12 +1265,12 @@ newversion(dns_db_t *db, dns_dbversion_t **versionp) { static void resigninsert(qpzonedb_t *qpdb, dns_slabheader_t *newheader) { - INSIST(newheader->heap_index == 0); - INSIST(!ISC_LINK_LINKED(newheader, link)); + REQUIRE(newheader->heap_index == 0); + REQUIRE(!ISC_LINK_LINKED(newheader, link)); - LOCK(&qpdb->heaplock); + RWLOCK(&qpdb->lock, isc_rwlocktype_write); isc_heap_insert(qpdb->heap, newheader); - UNLOCK(&qpdb->heaplock); + RWUNLOCK(&qpdb->lock, isc_rwlocktype_write); newheader->heap = qpdb->heap; } @@ -1281,15 +1278,17 @@ resigninsert(qpzonedb_t *qpdb, dns_slabheader_t *newheader) { static void resigndelete(qpzonedb_t *qpdb, qpdb_version_t *version, dns_slabheader_t *header DNS__DB_FLARG) { - if (header != NULL && header->heap_index != 0) { - LOCK(&qpdb->heaplock); - isc_heap_delete(qpdb->heap, header->heap_index); - UNLOCK(&qpdb->heaplock); - - header->heap_index = 0; - newref(qpdb, HEADERNODE(header) DNS__DB_FLARG_PASS); - ISC_LIST_APPEND(version->resigned_list, header, link); + if (header == NULL || header->heap_index == 0) { + return; } + + RWLOCK(&qpdb->lock, isc_rwlocktype_write); + isc_heap_delete(qpdb->heap, header->heap_index); + RWUNLOCK(&qpdb->lock, isc_rwlocktype_write); + + header->heap_index = 0; + newref(qpdb, HEADERNODE(header) DNS__DB_FLARG_PASS); + ISC_LIST_APPEND(version->resigned_list, header, link); } static void @@ -2412,7 +2411,7 @@ setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, isc_stdtime_t resign) { } if (header->heap_index != 0) { INSIST(RESIGN(header)); - LOCK(&qpdb->heaplock); + RWLOCK(&qpdb->lock, isc_rwlocktype_write); if (resign == 0) { isc_heap_delete(qpdb->heap, header->heap_index); header->heap_index = 0; @@ -2422,7 +2421,7 @@ setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, isc_stdtime_t resign) { } else if (resign_sooner(&oldheader, header)) { isc_heap_decreased(qpdb->heap, header->heap_index); } - UNLOCK(&qpdb->heaplock); + RWUNLOCK(&qpdb->lock, isc_rwlocktype_write); } else if (resign != 0) { DNS_SLABHEADER_SETATTR(header, DNS_SLABHEADERATTR_RESIGN); resigninsert(qpdb, header); @@ -2433,33 +2432,52 @@ setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, isc_stdtime_t resign) { } static isc_result_t -getsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, - dns_name_t *foundname DNS__DB_FLARG) { +getsigningtime(dns_db_t *db, isc_stdtime_t *resign, dns_name_t *foundname, + dns_typepair_t *typepair) { qpzonedb_t *qpdb = (qpzonedb_t *)db; dns_slabheader_t *header = NULL; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; + uint16_t locknum; + isc_result_t result = ISC_R_NOTFOUND; REQUIRE(VALID_QPZONE(qpdb)); + REQUIRE(resign != NULL); + REQUIRE(foundname != NULL); + REQUIRE(typepair != NULL); - LOCK(&qpdb->heaplock); + RWLOCK(&qpdb->lock, isc_rwlocktype_read); header = isc_heap_element(qpdb->heap, 1); - UNLOCK(&qpdb->heaplock); - if (header == NULL) { + RWUNLOCK(&qpdb->lock, isc_rwlocktype_read); return (ISC_R_NOTFOUND); } + locknum = HEADERNODE(header)->locknum; + RWUNLOCK(&qpdb->lock, isc_rwlocktype_read); - NODE_RDLOCK(&qpdb->node_locks[HEADERNODE(header)->locknum].lock, - &nlocktype); - bindrdataset(qpdb, HEADERNODE(header), header, 0, - rdataset DNS__DB_FLARG_PASS); - if (foundname != NULL) { - dns_name_copy(&HEADERNODE(header)->name, foundname); +again: + NODE_RDLOCK(&qpdb->node_locks[locknum].lock, &nlocktype); + + RWLOCK(&qpdb->lock, isc_rwlocktype_read); + header = isc_heap_element(qpdb->heap, 1); + if (header != NULL && HEADERNODE(header)->locknum != locknum) { + RWUNLOCK(&qpdb->lock, isc_rwlocktype_read); + NODE_UNLOCK(&qpdb->node_locks[locknum].lock, &nlocktype); + locknum = HEADERNODE(header)->locknum; + goto again; } - NODE_UNLOCK(&qpdb->node_locks[HEADERNODE(header)->locknum].lock, - &nlocktype); - return (ISC_R_SUCCESS); + if (header != NULL) { + *resign = RESIGN(header) + ? (header->resign << 1) | header->resign_lsb + : 0; + dns_name_copy(&HEADERNODE(header)->name, foundname); + *typepair = header->type; + result = ISC_R_SUCCESS; + } + RWUNLOCK(&qpdb->lock, isc_rwlocktype_read); + NODE_UNLOCK(&qpdb->node_locks[locknum].lock, &nlocktype); + + return (result); } static isc_result_t @@ -4008,9 +4026,9 @@ deletedata(dns_db_t *db ISC_ATTR_UNUSED, dns_dbnode_t *node ISC_ATTR_UNUSED, dns_slabheader_t *header = data; if (header->heap != NULL && header->heap_index != 0) { - LOCK(&qpdb->heaplock); + RWLOCK(&qpdb->lock, isc_rwlocktype_write); isc_heap_delete(header->heap, header->heap_index); - UNLOCK(&qpdb->heaplock); + RWUNLOCK(&qpdb->lock, isc_rwlocktype_write); } header->heap_index = 0; diff --git a/lib/dns/rbt-zonedb.c b/lib/dns/rbt-zonedb.c index f6d0afc517..43599fa381 100644 --- a/lib/dns/rbt-zonedb.c +++ b/lib/dns/rbt-zonedb.c @@ -1993,8 +1993,8 @@ setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, isc_stdtime_t resign) { } static isc_result_t -getsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, - dns_name_t *foundname DNS__DB_FLARG) { +getsigningtime(dns_db_t *db, isc_stdtime_t *resign, dns_name_t *foundname, + dns_typepair_t *typepair) { dns_rbtdb_t *rbtdb = (dns_rbtdb_t *)db; dns_slabheader_t *header = NULL, *this = NULL; unsigned int i; @@ -2004,6 +2004,9 @@ getsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, isc_rwlocktype_t nlocktype = isc_rwlocktype_none; REQUIRE(VALID_RBTDB(rbtdb)); + REQUIRE(resign != NULL); + REQUIRE(foundname != NULL); + REQUIRE(typepair != NULL); TREE_RDLOCK(&rbtdb->tree_lock, &tlocktype); @@ -2055,14 +2058,11 @@ getsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, * Found something; pass back the answer and unlock * the bucket. */ - dns__rbtdb_bindrdataset(rbtdb, RBTDB_HEADERNODE(header), header, - 0, isc_rwlocktype_read, - rdataset DNS__DB_FLARG_PASS); - - if (foundname != NULL) { - dns_rbt_fullnamefromnode(RBTDB_HEADERNODE(header), - foundname); - } + *resign = RESIGN(header) + ? (header->resign << 1) | header->resign_lsb + : 0; + dns_rbt_fullnamefromnode(RBTDB_HEADERNODE(header), foundname); + *typepair = header->type; NODE_UNLOCK(&rbtdb->node_locks[locknum].lock, &nlocktype); diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 4d6284c6b3..34e771b5cb 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -3816,12 +3816,12 @@ cleanup: static void set_resigntime(dns_zone_t *zone) { - dns_rdataset_t rdataset; dns_fixedname_t fixed; - unsigned int resign; + isc_stdtime_t resign; isc_result_t result; uint32_t nanosecs; dns_db_t *db = NULL; + dns_typepair_t typepair; INSIST(LOCKED_ZONE(zone)); @@ -3834,7 +3834,6 @@ set_resigntime(dns_zone_t *zone) { return; } - dns_rdataset_init(&rdataset); dns_fixedname_init(&fixed); ZONEDB_LOCK(&zone->dblock, isc_rwlocktype_read); @@ -3847,15 +3846,14 @@ set_resigntime(dns_zone_t *zone) { return; } - result = dns_db_getsigningtime(db, &rdataset, - dns_fixedname_name(&fixed)); + result = dns_db_getsigningtime(db, &resign, dns_fixedname_name(&fixed), + &typepair); if (result != ISC_R_SUCCESS) { isc_time_settoepoch(&zone->resigntime); goto cleanup; } - resign = rdataset.resign - dns_zone_getsigresigninginterval(zone); - dns_rdataset_disassociate(&rdataset); + resign -= dns_zone_getsigresigninginterval(zone); nanosecs = isc_random_uniform(1000000000); isc_time_set(&zone->resigntime, resign, nanosecs); @@ -5179,31 +5177,32 @@ zone_postload(dns_zone_t *zone, dns_db_t *db, isc_time_t loadtime, if (zone->type == dns_zone_primary && is_dynamic && dns_db_issecure(db) && !inline_raw(zone)) { + isc_stdtime_t resign; dns_name_t *name; dns_fixedname_t fixed; - dns_rdataset_t next; + dns_typepair_t typepair; - dns_rdataset_init(&next); name = dns_fixedname_initname(&fixed); - result = dns_db_getsigningtime(db, &next, name); + result = dns_db_getsigningtime(db, &resign, name, + &typepair); if (result == ISC_R_SUCCESS) { isc_stdtime_t timenow = isc_stdtime_now(); char namebuf[DNS_NAME_FORMATSIZE]; char typebuf[DNS_RDATATYPE_FORMATSIZE]; dns_name_format(name, namebuf, sizeof(namebuf)); - dns_rdatatype_format(next.covers, typebuf, - sizeof(typebuf)); + dns_rdatatype_format( + DNS_TYPEPAIR_COVERS(typepair), typebuf, + sizeof(typebuf)); dnssec_log( zone, ISC_LOG_DEBUG(3), "next resign: %s/%s " "in %d seconds", namebuf, typebuf, - next.resign - timenow - + resign - timenow - dns_zone_getsigresigninginterval( zone)); - dns_rdataset_disassociate(&next); } else { dnssec_log(zone, ISC_LOG_WARNING, "signed dynamic zone has no " @@ -6966,18 +6965,16 @@ zone_resigninc(dns_zone_t *zone) { dns__zonediff_t zonediff; dns_fixedname_t fixed; dns_name_t *name; - dns_rdataset_t rdataset; - dns_rdatatype_t covers; + dns_typepair_t typepair; dst_key_t *zone_keys[DNS_MAXZONEKEYS]; isc_result_t result; isc_stdtime_t now, inception, soaexpire, expire, fullexpire, stop; unsigned int i; unsigned int nkeys = 0; - unsigned int resign; + isc_stdtime_t resign; ENTER; - dns_rdataset_init(&rdataset); dns_diff_init(zone->mctx, &_sig_diff); zonediff_init(&zonediff, &_sig_diff); @@ -7024,7 +7021,7 @@ zone_resigninc(dns_zone_t *zone) { stop = now + 5; name = dns_fixedname_initname(&fixed); - result = dns_db_getsigningtime(db, &rdataset, name); + result = dns_db_getsigningtime(db, &resign, name, &typepair); if (result != ISC_R_SUCCESS && result != ISC_R_NOTFOUND) { dns_zone_log(zone, ISC_LOG_ERROR, "zone_resigninc:dns_db_getsigningtime -> %s", @@ -7033,10 +7030,9 @@ zone_resigninc(dns_zone_t *zone) { i = 0; while (result == ISC_R_SUCCESS) { - resign = rdataset.resign - - dns_zone_getsigresigninginterval(zone); - covers = rdataset.covers; - dns_rdataset_disassociate(&rdataset); + dns_rdatatype_t covers = DNS_TYPEPAIR_COVERS(typepair); + + resign -= dns_zone_getsigresigninginterval(zone); /* * Stop if we hit the SOA as that means we have walked the @@ -7076,7 +7072,7 @@ zone_resigninc(dns_zone_t *zone) { isc_result_totext(result)); break; } - result = dns_db_getsigningtime(db, &rdataset, name); + result = dns_db_getsigningtime(db, &resign, name, &typepair); if (nkeys == 0 && result == ISC_R_NOTFOUND) { result = ISC_R_SUCCESS; break;