From: Alessio Podda Date: Wed, 10 Jun 2026 13:24:06 +0000 (+0200) Subject: Split lock coos check design X-Git-Url: http://git.ipfire.org/gitweb/?a=commitdiff_plain;h=5b0a2f438653028429f870a171195bfe81dde793;p=thirdparty%2Fbind9.git Split lock coos check design Catalog zones might need to inspect the change-of-ownership records of other catalog zones, which required to release the lock in the middle of certain operations, leading to possible race conditions. Since the operations on change-of-ownership records are limited, we can instead use a design with a second lock protecting the change-of-ownership records on read. We structure the API so that holding two change-of-ownership locks at the same time is impossible. --- diff --git a/lib/dns/catz.c b/lib/dns/catz.c index d9316fb71d3..7d93a1eee86 100644 --- a/lib/dns/catz.c +++ b/lib/dns/catz.c @@ -57,6 +57,12 @@ struct dns_catz_coo { isc_refcount_t references; }; +typedef struct coos { + isc_mutex_t lock; + isc_mem_t *mctx; + isc_ht_t *inner; +} coos_t; + /*% * Single member zone in a catalog */ @@ -80,7 +86,7 @@ struct dns_catz_zone { /* key in entries is 'mhash', not domain name! */ isc_ht_t *entries; /* key in coos is domain name */ - isc_ht_t *coos; + coos_t coos; /* * defoptions are taken from named.conf @@ -258,16 +264,15 @@ catz_coo_new(isc_mem_t *mctx, const dns_name_t *domain) { } static void -catz_coo_detach(dns_catz_zone_t *catz, dns_catz_coo_t **coop) { +catz_coo_detach(isc_mem_t *mctx, dns_catz_coo_t **coop) { dns_catz_coo_t *coo; - REQUIRE(DNS_CATZ_ZONE_VALID(catz)); + REQUIRE(mctx != NULL); REQUIRE(coop != NULL && DNS_CATZ_COO_VALID(*coop)); coo = *coop; *coop = NULL; if (isc_refcount_decrement(&coo->references) == 1) { - isc_mem_t *mctx = catz->catzs->mctx; coo->magic = 0; isc_refcount_destroy(&coo->references); if (dns_name_dynamic(&coo->name)) { @@ -278,24 +283,126 @@ catz_coo_detach(dns_catz_zone_t *catz, dns_catz_coo_t **coop) { } static void -catz_coo_add(dns_catz_zone_t *catz, dns_catz_entry_t *entry, - const dns_name_t *domain) { - REQUIRE(DNS_CATZ_ZONE_VALID(catz)); +coos_init(coos_t *coos, isc_mem_t *mctx) { + REQUIRE(coos != NULL); + REQUIRE(mctx != NULL); + + isc_mutex_init(&coos->lock); + isc_mem_attach(mctx, &coos->mctx); + isc_ht_init(&coos->inner, coos->mctx, 4, ISC_HT_CASE_INSENSITIVE); +} + +static isc_ht_t * +coos_replace(coos_t *coos, isc_ht_t *newinner) { + isc_ht_t *oldinner = NULL; + + REQUIRE(coos != NULL); + REQUIRE(coos->mctx != NULL); + + LOCK(&coos->lock); + oldinner = coos->inner; + coos->inner = newinner; + UNLOCK(&coos->lock); + + return oldinner; +} + +static isc_ht_t * +coos_take(coos_t *coos) { + return coos_replace(coos, NULL); +} + +static void +coos_destroy_table(coos_t *coos, isc_ht_t **innerp) { + isc_ht_iter_t *iter = NULL; + isc_ht_t *inner = NULL; + isc_result_t result; + + REQUIRE(coos != NULL); + REQUIRE(coos->mctx != NULL); + REQUIRE(innerp != NULL); + + inner = *innerp; + if (inner == NULL) { + return; + } + + isc_ht_iter_create(inner, &iter); + for (result = isc_ht_iter_first(iter); result == ISC_R_SUCCESS; + result = isc_ht_iter_delcurrent_next(iter)) + { + dns_catz_coo_t *coo = NULL; + + isc_ht_iter_current(iter, (void **)&coo); + catz_coo_detach(coos->mctx, &coo); + } + INSIST(result == ISC_R_NOMORE); + isc_ht_iter_destroy(&iter); + + /* The hashtable has to be empty now. */ + INSIST(isc_ht_count(inner) == 0); + isc_ht_destroy(innerp); +} + +static void +coos_destroy(coos_t *coos) { + isc_ht_t *inner = NULL; + + REQUIRE(coos != NULL); + REQUIRE(coos->mctx != NULL); + + inner = coos_take(coos); + coos_destroy_table(coos, &inner); + isc_mutex_destroy(&coos->lock); + isc_mem_detach(&coos->mctx); +} + +static void +coos_add(coos_t *coos, dns_catz_entry_t *entry, const dns_name_t *domain) { + dns_catz_coo_t *coo = NULL; + isc_result_t result; + + REQUIRE(coos != NULL); + REQUIRE(coos->mctx != NULL); REQUIRE(DNS_CATZ_ENTRY_VALID(entry)); REQUIRE(domain != NULL); - /* We are write locked, so the add must succeed if not found */ - dns_catz_coo_t *coo = NULL; - isc_result_t result = isc_ht_find(catz->coos, entry->name.ndata, - entry->name.length, (void **)&coo); + LOCK(&coos->lock); + INSIST(coos->inner != NULL); + result = isc_ht_find(coos->inner, entry->name.ndata, entry->name.length, + (void **)&coo); if (result != ISC_R_SUCCESS) { - coo = catz_coo_new(catz->catzs->mctx, domain); - result = isc_ht_add(catz->coos, entry->name.ndata, + coo = catz_coo_new(coos->mctx, domain); + result = isc_ht_add(coos->inner, entry->name.ndata, entry->name.length, coo); } + UNLOCK(&coos->lock); + INSIST(result == ISC_R_SUCCESS); } +static bool +coos_match(coos_t *coos, const dns_name_t *zone, const dns_name_t *catz) { + dns_catz_coo_t *coo = NULL; + bool match = false; + + REQUIRE(coos != NULL); + REQUIRE(coos->mctx != NULL); + REQUIRE(zone != NULL); + REQUIRE(catz != NULL); + + LOCK(&coos->lock); + if (coos->inner != NULL && + isc_ht_find(coos->inner, zone->ndata, zone->length, + (void **)&coo) == ISC_R_SUCCESS) + { + match = dns_name_equal(&coo->name, catz); + } + UNLOCK(&coos->lock); + + return match; +} + dns_catz_entry_t * dns_catz_entry_new(isc_mem_t *mctx, const dns_name_t *domain) { REQUIRE(mctx != NULL); @@ -578,25 +685,19 @@ dns__catz_zones_merge(dns_catz_zone_t *catz, dns_catz_zone_t *newcatz) { dns_catz_entry_getname(nentry), DNS_ZTFIND_EXACT, &zone); if (find_result == ISC_R_SUCCESS) { - dns_catz_coo_t *coo = NULL; char pczname[DNS_NAME_FORMATSIZE]; - bool parentcatz_locked = false; + bool coo_match = false; /* * Change of ownership (coo) processing, if required */ parentcatz = dns_zone_get_parentcatz(zone); if (parentcatz != NULL && parentcatz != catz) { - UNLOCK(&catz->lock); - LOCK(&parentcatz->lock); - parentcatz_locked = true; + coo_match = coos_match(&parentcatz->coos, + &nentry->name, + &catz->name); } - if (parentcatz_locked && - isc_ht_find(parentcatz->coos, nentry->name.ndata, - nentry->name.length, - (void **)&coo) == ISC_R_SUCCESS && - dns_name_equal(&coo->name, &catz->name)) - { + if (coo_match) { dns_name_format(&parentcatz->name, pczname, DNS_NAME_FORMATSIZE); isc_log_write(DNS_LOGCATEGORY_GENERAL, @@ -616,10 +717,6 @@ dns__catz_zones_merge(dns_catz_zone_t *catz, dns_catz_zone_t *newcatz) { zname, pczname, isc_result_totext(result)); } - if (parentcatz_locked) { - UNLOCK(&parentcatz->lock); - LOCK(&catz->lock); - } dns_zone_detach(&zone); } @@ -745,28 +842,9 @@ dns__catz_zones_merge(dns_catz_zone_t *catz, dns_catz_zone_t *newcatz) { * We do not need to merge old coo (change of ownership) permission * records with the new ones, just replace them. */ - if (catz->coos != NULL && newcatz->coos != NULL) { - isc_ht_iter_t *iter = NULL; - - isc_ht_iter_create(catz->coos, &iter); - for (result = isc_ht_iter_first(iter); result == ISC_R_SUCCESS; - result = isc_ht_iter_delcurrent_next(iter)) - { - dns_catz_coo_t *coo = NULL; - - isc_ht_iter_current(iter, (void **)&coo); - catz_coo_detach(catz, &coo); - } - INSIST(result == ISC_R_NOMORE); - isc_ht_iter_destroy(&iter); - - /* The hashtable has to be empty now. */ - INSIST(isc_ht_count(catz->coos) == 0); - isc_ht_destroy(&catz->coos); - - catz->coos = newcatz->coos; - newcatz->coos = NULL; - } + isc_ht_t *newcoos = coos_take(&newcatz->coos); + isc_ht_t *oldcoos = coos_replace(&catz->coos, newcoos); + coos_destroy_table(&catz->coos, &oldcoos); result = ISC_R_SUCCESS; @@ -835,7 +913,7 @@ dns_catz_zone_new(dns_catz_zones_t *catzs, const dns_name_t *name) { isc_mutex_init(&catz->lock); isc_refcount_init(&catz->references, 1); isc_ht_init(&catz->entries, catzs->mctx, 4, ISC_HT_CASE_INSENSITIVE); - isc_ht_init(&catz->coos, catzs->mctx, 4, ISC_HT_CASE_INSENSITIVE); + coos_init(&catz->coos, catzs->mctx); isc_time_settoepoch(&catz->lastupdated); dns_catz_options_init(&catz->defoptions); dns_catz_options_init(&catz->zoneoptions); @@ -999,25 +1077,7 @@ dns__catz_zone_destroy(dns_catz_zone_t *catz) { INSIST(isc_ht_count(catz->entries) == 0); isc_ht_destroy(&catz->entries); } - if (catz->coos != NULL) { - isc_ht_iter_t *iter = NULL; - isc_result_t result; - isc_ht_iter_create(catz->coos, &iter); - for (result = isc_ht_iter_first(iter); result == ISC_R_SUCCESS; - result = isc_ht_iter_delcurrent_next(iter)) - { - dns_catz_coo_t *coo = NULL; - - isc_ht_iter_current(iter, (void **)&coo); - catz_coo_detach(catz, &coo); - } - INSIST(result == ISC_R_NOMORE); - isc_ht_iter_destroy(&iter); - - /* The hashtable has to be empty now. */ - INSIST(isc_ht_count(catz->coos) == 0); - isc_ht_destroy(&catz->coos); - } + coos_destroy(&catz->coos); catz->magic = 0; isc_mutex_destroy(&catz->lock); @@ -1219,7 +1279,7 @@ catz_process_coo(dns_catz_zone_t *catz, dns_label_t *mhash, CLEANUP(ISC_R_FAILURE); } - catz_coo_add(catz, entry, &ptr.ptr); + coos_add(&catz->coos, entry, &ptr.ptr); cleanup: dns_rdata_freestruct(&ptr); @@ -2331,14 +2391,6 @@ dns__catz_update_cb(void *data) { continue; } - /* - * Although newcatz->coos is accessed in - * catz_process_coo() in the call-chain below, we don't - * need to hold the newcatz->lock, because the newcatz - * is still local to this thread and function and - * newcatz->coos can't be accessed from the outside - * until dns__catz_zones_merge() has been called. - */ result = dns__catz_update_process(newcatz, name, &rdataset); if (result != ISC_R_SUCCESS) {