From 593dea871afe903fbff420be0ccb63c85152de4a Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Wed, 1 Mar 2023 17:26:41 +0000 Subject: [PATCH 1/8] Revert "Process db callbacks in zone_loaddone() after zone_postload()" This reverts commit ed268b46f11706bde3da68bd2a4b45752350f736. The commit introduced a data race, because dns_db_endload() is called after unfreezing the zone. --- lib/dns/zone.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 544b502a4c..bb3bcaa79b 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -17244,6 +17244,13 @@ zone_loaddone(void *arg, isc_result_t result) { dns_zone_catz_disable_db(zone, load->db); } + tresult = dns_db_endload(load->db, &load->callbacks); + if (tresult != ISC_R_SUCCESS && + (result == ISC_R_SUCCESS || result == DNS_R_SEENINCLUDE)) + { + result = tresult; + } + /* * Lock hierarchy: zmgr, zone, raw. */ @@ -17262,13 +17269,9 @@ again: goto again; } } - tresult = zone_postload(zone, load->db, load->loadtime, result); - if (tresult != ISC_R_SUCCESS && - (result == ISC_R_SUCCESS || result == DNS_R_SEENINCLUDE)) - { - result = tresult; - } + (void)zone_postload(zone, load->db, load->loadtime, result); DNS_ZONE_CLRFLAG(zone, DNS_ZONEFLG_LOADING); + zone_idetach(&load->callbacks.zone); /* * Leave the zone frozen if the reload fails. */ @@ -17285,12 +17288,6 @@ again: } UNLOCK_ZONE(zone); - (void)dns_db_endload(load->db, &load->callbacks); - - LOCK_ZONE(zone); - zone_idetach(&load->callbacks.zone); - UNLOCK_ZONE(zone); - dns_db_detach(&load->db); if (zone->loadctx != NULL) { dns_loadctx_detach(&zone->loadctx); From e1627e128959a077183d955beba19cce0fa991ff Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Thu, 2 Mar 2023 09:39:32 +0000 Subject: [PATCH 2/8] Update the CHANGES note for [GL #3777] Remove the part which is no longer true after reverting the commit in question. The CHANGES entry was never part of a released BIND 9 version. --- CHANGES | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/CHANGES b/CHANGES index cd12176c6e..cbb7421d6d 100644 --- a/CHANGES +++ b/CHANGES @@ -16,8 +16,7 @@ 6115. [bug] Unregister db update notify callback before detaching from the previous db inside the catz update notify - callback. Also, call the db notify callbacks only after - zone_postload() returns successfully. [GL #3777] + callback. [GL #3777] 6114. [func] Run the catalog zone update process on the offload threads. [GL #3881] From d2ecff3c4a0d961041b860515858d258d40462d7 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Thu, 2 Mar 2023 08:52:25 +0000 Subject: [PATCH 3/8] catz: use two pairs of dns_db_t and dns_dbversion_t in a catalog zone As it is done in the RPZ module, use 'db' and 'dbversion' for the database we are going to update to, and 'updb' and 'updbversion' for the database we are working on. Doing this should avoid a race between the 'dns__catz_update_cb()' and 'dns_catz_dbupdate_callback()' functions. --- lib/dns/catz.c | 76 +++++++++++++++++++++++++++----------------------- 1 file changed, 41 insertions(+), 35 deletions(-) diff --git a/lib/dns/catz.c b/lib/dns/catz.c index 3a28fbcf0e..60f690fbe1 100644 --- a/lib/dns/catz.c +++ b/lib/dns/catz.c @@ -88,11 +88,13 @@ struct dns_catz_zone { dns_catz_options_t zoneoptions; isc_time_t lastupdated; - bool updatepending; /* there is an update pending */ - bool updaterunning; /* there is an update running */ - isc_result_t updateresult; /* result from the offloaded work */ - dns_db_t *db; /* zones database */ - dns_dbversion_t *dbversion; /* zones database version */ + bool updatepending; /* there is an update pending */ + bool updaterunning; /* there is an update running */ + isc_result_t updateresult; /* result from the offloaded work */ + dns_db_t *db; /* zones database */ + dns_dbversion_t *dbversion; /* version we will be updating to */ + dns_db_t *updb; /* zones database we're working on */ + dns_dbversion_t *updbversion; /* version we're working on */ isc_timer_t *updatetimer; @@ -2094,12 +2096,18 @@ dns__catz_timer_cb(void *arg) { LOCK(&catz->catzs->lock); INSIST(DNS_DB_VALID(catz->db)); - INSIST(catz->dbversion != NULL); + INSIST(catz->updb == NULL); + INSIST(catz->updbversion == NULL); catz->updatepending = false; catz->updaterunning = true; catz->updateresult = ISC_R_UNSET; + dns_db_attach(catz->db, &catz->updb); + INSIST(catz->dbversion != NULL); + catz->updbversion = catz->dbversion; + catz->dbversion = NULL; + dns_name_format(&catz->name, domain, DNS_NAME_FORMATSIZE); isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, DNS_LOGMODULE_MASTER, ISC_LOG_INFO, "catz: %s: reload start", domain); @@ -2193,14 +2201,14 @@ catz_rdatatype_is_processable(const dns_rdatatype_t type) { static void dns__catz_update_cb(void *data) { dns_catz_zone_t *catz = (dns_catz_zone_t *)data; - dns_db_t *db = NULL; + dns_db_t *updb = NULL; dns_catz_zones_t *catzs = NULL; dns_catz_zone_t *oldcatz = NULL, *newcatz = NULL; isc_result_t result; isc_region_t r; dns_dbnode_t *node = NULL; const dns_dbnode_t *vers_node = NULL; - dns_dbiterator_t *it = NULL; + dns_dbiterator_t *updbit = NULL; dns_fixedname_t fixname; dns_name_t *name = NULL; dns_rdatasetiter_t *rdsiter = NULL; @@ -2215,7 +2223,7 @@ dns__catz_update_cb(void *data) { REQUIRE(DNS_DB_VALID(catz->db)); REQUIRE(DNS_CATZ_ZONES_VALID(catz->catzs)); - db = catz->db; + updb = catz->updb; catzs = catz->catzs; if (atomic_load(&catzs->shuttingdown)) { @@ -2223,12 +2231,12 @@ dns__catz_update_cb(void *data) { goto exit; } - dns_name_format(&db->origin, bname, DNS_NAME_FORMATSIZE); + dns_name_format(&updb->origin, bname, DNS_NAME_FORMATSIZE); /* * Create a new catz in the same context as current catz. */ - dns_name_toregion(&db->origin, &r); + dns_name_toregion(&updb->origin, &r); LOCK(&catzs->lock); result = isc_ht_find(catzs->zones, r.base, r.length, (void **)&oldcatz); UNLOCK(&catzs->lock); @@ -2240,7 +2248,7 @@ dns__catz_update_cb(void *data) { goto exit; } - result = dns_db_getsoaserial(db, oldcatz->dbversion, &vers); + result = dns_db_getsoaserial(updb, oldcatz->updbversion, &vers); if (result != ISC_R_SUCCESS) { /* A zone without SOA record?!? */ isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, @@ -2255,9 +2263,8 @@ dns__catz_update_cb(void *data) { "catz: updating catalog zone '%s' with serial %" PRIu32, bname, vers); - result = dns_catz_new_zone(catzs, &newcatz, &db->origin); + result = dns_catz_new_zone(catzs, &newcatz, &updb->origin); if (result != ISC_R_SUCCESS) { - dns_db_closeversion(db, &oldcatz->dbversion, false); isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, DNS_LOGMODULE_MASTER, ISC_LOG_ERROR, "catz: failed to create new zone - %s", @@ -2265,10 +2272,9 @@ dns__catz_update_cb(void *data) { goto exit; } - result = dns_db_createiterator(db, DNS_DB_NONSEC3, &it); + result = dns_db_createiterator(updb, DNS_DB_NONSEC3, &updbit); if (result != ISC_R_SUCCESS) { dns_catz_detach_catz(&newcatz); - dns_db_closeversion(db, &oldcatz->dbversion, false); isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, DNS_LOGMODULE_MASTER, ISC_LOG_ERROR, "catz: failed to create DB iterator - %s", @@ -2283,21 +2289,19 @@ dns__catz_update_cb(void *data) { * records might be processed differently depending on the version of * the catalog zone's schema. */ - result = dns_name_fromstring2(name, "version", &db->origin, 0, NULL); + result = dns_name_fromstring2(name, "version", &updb->origin, 0, NULL); if (result != ISC_R_SUCCESS) { - dns_dbiterator_destroy(&it); + dns_dbiterator_destroy(&updbit); dns_catz_detach_catz(&newcatz); - dns_db_closeversion(db, &oldcatz->dbversion, false); isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, DNS_LOGMODULE_MASTER, ISC_LOG_ERROR, "catz: failed to create name from string - %s", isc_result_totext(result)); goto exit; } - result = dns_dbiterator_seek(it, name); + result = dns_dbiterator_seek(updbit, name); if (result != ISC_R_SUCCESS) { - dns_dbiterator_destroy(&it); - dns_db_closeversion(db, &oldcatz->dbversion, false); + dns_dbiterator_destroy(&updbit); isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, DNS_LOGMODULE_MASTER, ISC_LOG_ERROR, "catz: zone '%s' has no 'version' record (%s)", @@ -2317,7 +2321,7 @@ dns__catz_update_cb(void *data) { break; } - result = dns_dbiterator_current(it, &node, name); + result = dns_dbiterator_current(updbit, &node, name); if (result != ISC_R_SUCCESS) { isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, DNS_LOGMODULE_MASTER, ISC_LOG_ERROR, @@ -2326,7 +2330,7 @@ dns__catz_update_cb(void *data) { break; } - result = dns_dbiterator_pause(it); + result = dns_dbiterator_pause(updbit); RUNTIME_CHECK(result == ISC_R_SUCCESS); if (!is_vers_processed) { @@ -2334,19 +2338,19 @@ dns__catz_update_cb(void *data) { vers_node = node; } else if (node == vers_node) { /* Skip the already processed version node */ - dns_db_detachnode(db, &node); - result = dns_dbiterator_next(it); + dns_db_detachnode(updb, &node); + result = dns_dbiterator_next(updbit); continue; } - result = dns_db_allrdatasets(db, node, oldcatz->dbversion, 0, 0, - &rdsiter); + result = dns_db_allrdatasets(updb, node, oldcatz->updbversion, + 0, 0, &rdsiter); if (result != ISC_R_SUCCESS) { isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, DNS_LOGMODULE_MASTER, ISC_LOG_ERROR, "catz: failed to fetch rrdatasets - %s", isc_result_totext(result)); - dns_db_detachnode(db, &node); + dns_db_detachnode(updb, &node); break; } @@ -2401,18 +2405,17 @@ dns__catz_update_cb(void *data) { dns_rdatasetiter_destroy(&rdsiter); - dns_db_detachnode(db, &node); + dns_db_detachnode(updb, &node); if (!is_vers_processed) { is_vers_processed = true; - result = dns_dbiterator_first(it); + result = dns_dbiterator_first(updbit); } else { - result = dns_dbiterator_next(it); + result = dns_dbiterator_next(updbit); } } - dns_dbiterator_destroy(&it); - dns_db_closeversion(db, &oldcatz->dbversion, false); + dns_dbiterator_destroy(&updbit); isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, DNS_LOGMODULE_MASTER, ISC_LOG_DEBUG(3), "catz: update_from_db: iteration finished: %s", @@ -2479,7 +2482,7 @@ final: */ if (!oldcatz->db_registered) { result = dns_db_updatenotify_register( - db, dns_catz_dbupdate_callback, oldcatz->catzs); + updb, dns_catz_dbupdate_callback, oldcatz->catzs); if (result == ISC_R_SUCCESS) { oldcatz->db_registered = true; } @@ -2506,6 +2509,9 @@ dns__catz_done_cb(void *data) { dns__catz_timer_start(catz); } + dns_db_closeversion(catz->updb, &catz->updbversion, false); + dns_db_detach(&catz->updb); + UNLOCK(&catz->catzs->lock); isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, DNS_LOGMODULE_MASTER, From a87859f1fa05ce92e99acb2c12aae0245bc8e79e Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Thu, 2 Mar 2023 10:18:11 +0000 Subject: [PATCH 4/8] catz: protect db_registered and db callback (un)registration with a lock Doing this to avoid a race between the 'dns__catz_update_cb()' and 'dns_catz_dbupdate_callback()' functions. --- lib/dns/catz.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/dns/catz.c b/lib/dns/catz.c index 60f690fbe1..f0f3d04771 100644 --- a/lib/dns/catz.c +++ b/lib/dns/catz.c @@ -2480,6 +2480,7 @@ final: * update callback in zone_startload or axfr_makedb, but we will * call onupdate() artificially so we can register the callback here. */ + LOCK(&catzs->lock); if (!oldcatz->db_registered) { result = dns_db_updatenotify_register( updb, dns_catz_dbupdate_callback, oldcatz->catzs); @@ -2487,6 +2488,7 @@ final: oldcatz->db_registered = true; } } + UNLOCK(&catzs->lock); exit: catz->updateresult = result; From cb0d6393a7f382e5e05b009f3eefc13acfe99719 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Thu, 2 Mar 2023 09:43:01 +0000 Subject: [PATCH 5/8] Add a CHANGES note for [GL #3907] --- CHANGES | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGES b/CHANGES index cbb7421d6d..8023c4855b 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,8 @@ +6120. [bug] Use two pairs of dns_db_t and dns_dbversion_t in a + catalog zone structure to avoid a race between the + dns__catz_update_cb() and dns_catz_dbupdate_callback() + functions. [GL #3907] + 6119. [bug] Make sure to revert the reconfigured zones to the previous version of the view, when the new view reconfiguration fails during the configuration of From 3973724d67651d3a3d90c6a4d32add040ba2b707 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Thu, 2 Mar 2023 13:19:09 +0000 Subject: [PATCH 6/8] Use catzs->lock in dns_catz_prereconfig() There can be an update running in another thread, so use a lock, like it's done in dns_catz_postreconfig(). --- lib/dns/catz.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/dns/catz.c b/lib/dns/catz.c index f0f3d04771..f02784c659 100644 --- a/lib/dns/catz.c +++ b/lib/dns/catz.c @@ -2530,6 +2530,7 @@ dns_catz_prereconfig(dns_catz_zones_t *catzs) { REQUIRE(DNS_CATZ_ZONES_VALID(catzs)); + LOCK(&catzs->lock); isc_ht_iter_create(catzs->zones, &iter); for (result = isc_ht_iter_first(iter); result == ISC_R_SUCCESS; result = isc_ht_iter_next(iter)) @@ -2538,6 +2539,7 @@ dns_catz_prereconfig(dns_catz_zones_t *catzs) { isc_ht_iter_current(iter, (void **)&catz); catz->active = false; } + UNLOCK(&catzs->lock); INSIST(result == ISC_R_NOMORE); isc_ht_iter_destroy(&iter); } From 67c77aba380acd038bde11a5067189fed2ffb7d9 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Thu, 2 Mar 2023 13:19:53 +0000 Subject: [PATCH 7/8] Check if catz is active in dns__catz_timer_cb() A reconfiguration can deactivate the catalog zone, while the update process was deferred using a timer. --- lib/dns/catz.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/lib/dns/catz.c b/lib/dns/catz.c index f02784c659..c878c79287 100644 --- a/lib/dns/catz.c +++ b/lib/dns/catz.c @@ -2096,6 +2096,7 @@ dns__catz_timer_cb(void *arg) { LOCK(&catz->catzs->lock); INSIST(DNS_DB_VALID(catz->db)); + INSIST(catz->dbversion != NULL); INSIST(catz->updb == NULL); INSIST(catz->updbversion == NULL); @@ -2103,12 +2104,22 @@ dns__catz_timer_cb(void *arg) { catz->updaterunning = true; catz->updateresult = ISC_R_UNSET; + dns_name_format(&catz->name, domain, DNS_NAME_FORMATSIZE); + + if (!catz->active) { + isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, + DNS_LOGMODULE_MASTER, ISC_LOG_INFO, + "catz: %s: no longer active, reload is canceled", + domain); + catz->updaterunning = false; + catz->updateresult = ISC_R_CANCELED; + goto exit; + } + dns_db_attach(catz->db, &catz->updb); - INSIST(catz->dbversion != NULL); catz->updbversion = catz->dbversion; catz->dbversion = NULL; - dns_name_format(&catz->name, domain, DNS_NAME_FORMATSIZE); isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, DNS_LOGMODULE_MASTER, ISC_LOG_INFO, "catz: %s: reload start", domain); @@ -2116,6 +2127,7 @@ dns__catz_timer_cb(void *arg) { isc_work_enqueue(catz->loop, dns__catz_update_cb, dns__catz_done_cb, catz); +exit: isc_timer_destroy(&catz->updatetimer); catz->loop = NULL; From 6980e3b354778e3ff628d8e72ddf357cb0d8b2a0 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Thu, 2 Mar 2023 13:32:21 +0000 Subject: [PATCH 8/8] Check if catz is active in dns__catz_update_cb() A reconfiguration can deactivate the catalog zone, while the offloaded update process was preparing to run. --- lib/dns/catz.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/lib/dns/catz.c b/lib/dns/catz.c index c878c79287..73d30b62a0 100644 --- a/lib/dns/catz.c +++ b/lib/dns/catz.c @@ -2228,6 +2228,7 @@ dns__catz_update_cb(void *data) { char bname[DNS_NAME_FORMATSIZE]; char cname[DNS_NAME_FORMATSIZE]; bool is_vers_processed = false; + bool is_active; uint32_t vers; uint32_t catz_vers; @@ -2251,6 +2252,7 @@ dns__catz_update_cb(void *data) { dns_name_toregion(&updb->origin, &r); LOCK(&catzs->lock); result = isc_ht_find(catzs->zones, r.base, r.length, (void **)&oldcatz); + is_active = (result == ISC_R_SUCCESS && oldcatz->active); UNLOCK(&catzs->lock); if (result != ISC_R_SUCCESS) { /* This can happen if we remove the zone in the meantime. */ @@ -2260,6 +2262,15 @@ dns__catz_update_cb(void *data) { goto exit; } + if (!is_active) { + /* This can happen during a reconfiguration. */ + isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, + DNS_LOGMODULE_MASTER, ISC_LOG_INFO, + "catz: zone '%s' is no longer active", bname); + result = ISC_R_CANCELED; + goto exit; + } + result = dns_db_getsoaserial(updb, oldcatz->updbversion, &vers); if (result != ISC_R_SUCCESS) { /* A zone without SOA record?!? */