From 6834ea968e83a061a4305d998569e0145a18994b Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Thu, 2 Mar 2023 18:38:49 +0000 Subject: [PATCH 1/8] Revert "Process db callbacks in zone_loaddone() after zone_postload()" This reverts commit a7196470233d4d79c6998d94e87d5a574f841644. The commit introduced a data race, because dns_db_endload() is called after unfreezing the zone. (not cherry picked from commit 593dea871afe903fbff420be0ccb63c85152de4a) --- lib/dns/zone.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/lib/dns/zone.c b/lib/dns/zone.c index ba130b959e..c9bb345156 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -17843,6 +17843,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. */ @@ -17861,14 +17868,10 @@ 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); zonemgr_putio(&zone->readio); DNS_ZONE_CLRFLAG(zone, DNS_ZONEFLG_LOADING); + zone_idetach(&load->callbacks.zone); /* * Leave the zone frozen if the reload fails. */ @@ -17885,14 +17888,7 @@ again: } UNLOCK_ZONE(zone); - (void)dns_db_endload(load->db, &load->callbacks); - - LOCK_ZONE(zone); - zone_idetach(&load->callbacks.zone); - UNLOCK_ZONE(zone); - load->magic = 0; - dns_db_detach(&load->db); if (load->zone->lctx != NULL) { dns_loadctx_detach(&load->zone->lctx); From 363061a1fcc61748d7a46d1ea0421da37b736a93 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. (cherry picked from commit e1627e128959a077183d955beba19cce0fa991ff) --- CHANGES | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/CHANGES b/CHANGES index f0ce6b7d6d..bda6eca03e 100644 --- a/CHANGES +++ b/CHANGES @@ -8,8 +8,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 574682b8c7e4e4bfb98b14f16bc333d3c32953d2 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. (cherry picked from commit d2ecff3c4a0d961041b860515858d258d40462d7) --- 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 5f642494b1..909b8a102c 100644 --- a/lib/dns/catz.c +++ b/lib/dns/catz.c @@ -87,11 +87,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; isc_event_t updateevent; @@ -2084,12 +2086,18 @@ dns__catz_timer_cb(isc_task_t *task, isc_event_t *event) { 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); @@ -2213,14 +2221,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; dns_rdatasetiter_t *rdsiter = NULL; @@ -2235,7 +2243,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)) { @@ -2243,12 +2251,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); @@ -2260,7 +2268,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, @@ -2275,9 +2283,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", @@ -2285,10 +2292,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", @@ -2303,21 +2309,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)", @@ -2337,7 +2341,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, @@ -2346,7 +2350,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) { @@ -2354,19 +2358,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; } @@ -2421,18 +2425,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", @@ -2499,7 +2502,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; } @@ -2555,6 +2558,9 @@ dns__catz_done_cb(void *data, isc_result_t result) { } done: + 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 d6001423af831572f1af555d8a4cebade010cca4 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. (cherry picked from commit a87859f1fa05ce92e99acb2c12aae0245bc8e79e) --- lib/dns/catz.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/dns/catz.c b/lib/dns/catz.c index 909b8a102c..30722db307 100644 --- a/lib/dns/catz.c +++ b/lib/dns/catz.c @@ -2500,6 +2500,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); @@ -2507,6 +2508,7 @@ final: oldcatz->db_registered = true; } } + UNLOCK(&catzs->lock); exit: catz->updateresult = result; From b230fbb591fd8228b1f2ab6571b016dbf9c95a47 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] (cherry picked from commit cb0d6393a7f382e5e05b009f3eefc13acfe99719) --- CHANGES | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGES b/CHANGES index bda6eca03e..d3fb3e5c50 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 fb15a6d6f6dcc1e7953ba9f8e7025c03318f03cc 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(). (cherry picked from commit 3973724d67651d3a3d90c6a4d32add040ba2b707) --- lib/dns/catz.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/dns/catz.c b/lib/dns/catz.c index 30722db307..2a69d088c6 100644 --- a/lib/dns/catz.c +++ b/lib/dns/catz.c @@ -2579,6 +2579,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)) @@ -2587,6 +2588,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 2e348627a179a3b8593264de6efc46d648699081 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. (cherry picked from commit 67c77aba380acd038bde11a5067189fed2ffb7d9) --- 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 2a69d088c6..b45dbcb239 100644 --- a/lib/dns/catz.c +++ b/lib/dns/catz.c @@ -2086,6 +2086,7 @@ dns__catz_timer_cb(isc_task_t *task, isc_event_t *event) { LOCK(&catz->catzs->lock); INSIST(DNS_DB_VALID(catz->db)); + INSIST(catz->dbversion != NULL); INSIST(catz->updb == NULL); INSIST(catz->updbversion == NULL); @@ -2093,12 +2094,22 @@ dns__catz_timer_cb(isc_task_t *task, isc_event_t *event) { 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); @@ -2106,6 +2117,7 @@ dns__catz_timer_cb(isc_task_t *task, isc_event_t *event) { isc_nm_work_offload(isc_task_getnetmgr(catz->catzs->updater), dns__catz_update_cb, dns__catz_done_cb, catz); +exit: result = isc_time_now(&catz->lastupdated); RUNTIME_CHECK(result == ISC_R_SUCCESS); From 9c48b6619ad17596a01b3eaede5bb0f84c83a7ee 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. (cherry picked from commit 6980e3b354778e3ff628d8e72ddf357cb0d8b2a0) --- lib/dns/catz.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/lib/dns/catz.c b/lib/dns/catz.c index b45dbcb239..58942dcda9 100644 --- a/lib/dns/catz.c +++ b/lib/dns/catz.c @@ -2248,6 +2248,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; @@ -2271,6 +2272,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. */ @@ -2280,6 +2282,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?!? */