From c67ce9704525b94543f317198be1fe9131ca4acf Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Fri, 9 Jun 2023 07:13:27 +0000 Subject: [PATCH 1/3] Fix a data race between the dns_zone and dns_catz modules The dns_zone_catz_enable_db() and dns_zone_catz_disable_db() functions can race with similar operations in the catz module because there is no synchronization between the threads. Add catz functions which use the view's catalog zones' lock when registering/unregistering the database update notify callback, and use those functions in the dns_zone module, instead of doing it directly. (cherry picked from commit 6f1f5fc307c2513d31cda94ef8cc47aa5efeaf88) --- lib/dns/catz.c | 20 ++++++++++++++++++++ lib/dns/include/dns/catz.h | 20 ++++++++++++++++++++ lib/dns/zone.c | 6 ++---- 3 files changed, 42 insertions(+), 4 deletions(-) diff --git a/lib/dns/catz.c b/lib/dns/catz.c index 37ba7f0fb4..70b48fdc2c 100644 --- a/lib/dns/catz.c +++ b/lib/dns/catz.c @@ -2222,6 +2222,26 @@ cleanup: return (result); } +void +dns_catz_dbupdate_unregister(dns_db_t *db, dns_catz_zones_t *catzs) { + REQUIRE(DNS_DB_VALID(db)); + REQUIRE(DNS_CATZ_ZONES_VALID(catzs)); + + LOCK(&catzs->lock); + dns_db_updatenotify_unregister(db, dns_catz_dbupdate_callback, catzs); + UNLOCK(&catzs->lock); +} + +void +dns_catz_dbupdate_register(dns_db_t *db, dns_catz_zones_t *catzs) { + REQUIRE(DNS_DB_VALID(db)); + REQUIRE(DNS_CATZ_ZONES_VALID(catzs)); + + LOCK(&catzs->lock); + dns_db_updatenotify_register(db, dns_catz_dbupdate_callback, catzs); + UNLOCK(&catzs->lock); +} + static bool catz_rdatatype_is_processable(const dns_rdatatype_t type) { return (!dns_rdatatype_isdnssec(type) && type != dns_rdatatype_cds && diff --git a/lib/dns/include/dns/catz.h b/lib/dns/include/dns/catz.h index 2454ed4f66..1401380ee9 100644 --- a/lib/dns/include/dns/catz.h +++ b/lib/dns/include/dns/catz.h @@ -358,6 +358,26 @@ dns_catz_dbupdate_callback(dns_db_t *db, void *fn_arg); * \li 'fn_arg' is not NULL (casted to dns_catz_zones_t*). */ +void +dns_catz_dbupdate_unregister(dns_db_t *db, dns_catz_zones_t *catzs); +/*%< + * Register the catalog zone database update notify callback. + * + * Requires: + * \li 'db' is a valid database. + * \li 'catzs' is valid. + */ + +void +dns_catz_dbupdate_register(dns_db_t *db, dns_catz_zones_t *catzs); +/*%< + * Unregister the catalog zone database update notify callback. + * + * Requires: + * \li 'db' is a valid database. + * \li 'catzs' is valid. + */ + void dns_catz_prereconfig(dns_catz_zones_t *catzs); /*%< diff --git a/lib/dns/zone.c b/lib/dns/zone.c index b9e9791d4f..52122ad8d1 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -1972,8 +1972,7 @@ dns_zone_catz_enable_db(dns_zone_t *zone, dns_db_t *db) { REQUIRE(db != NULL); if (zone->catzs != NULL) { - dns_db_updatenotify_register(db, dns_catz_dbupdate_callback, - zone->catzs); + dns_catz_dbupdate_register(db, zone->catzs); } } @@ -1983,8 +1982,7 @@ dns_zone_catz_disable_db(dns_zone_t *zone, dns_db_t *db) { REQUIRE(db != NULL); if (zone->catzs != NULL) { - dns_db_updatenotify_unregister(db, dns_catz_dbupdate_callback, - zone->catzs); + dns_catz_dbupdate_unregister(db, zone->catzs); } } From 471a2c0dd5945c86c2cc7c5896e780eff059f903 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Wed, 14 Jun 2023 09:02:51 +0000 Subject: [PATCH 2/3] Add a CHANGES note for [GL #4132] (cherry picked from commit f73007afe7a02b0ca0a18fe80609e5ff5bea636d) --- CHANGES | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGES b/CHANGES index 98d3b80362..19c25c570f 100644 --- a/CHANGES +++ b/CHANGES @@ -16,6 +16,11 @@ 6198. [func] Remove the holes in the isc_result_t enum to compact the isc_result tables. [GL #4149] +6197. [bug] Fix a data race between the dns_zone and dns_catz + modules when registering/unregistering a database + update notification callback for a catalog zone. + [GL #4132] + 6196. [cleanup] Report "permission denied" instead of "unexpected error" when trying to update a zone file on a read-only file system. Thanks to Midnight Veil. [GL #4134] From 26bb402b44987f3235b3db65b7b4806d38ebb930 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 4 Jul 2023 15:15:12 +0200 Subject: [PATCH 3/3] Run RPZ and catalog zones tasks in exclusive mode All the heavy RPZ and CATZ work is already running with offloaded threads, and running the remaining small functions in exclusive mode offers more synchronization guaranties. Move the update notify registration code from the offloaded dns__catz_update_cb() function into dns__catz_done_cb(). After this change, it should be safe to remove the lock/unlock code from the dns_catz_dbupdate_register() and dns_catz_dbupdate_unregister() functions, as they were causing a benign TSAN lock-order-inversion report. --- lib/dns/catz.c | 42 ++++++++++++++++++++---------------------- lib/dns/rpz.c | 4 ++-- 2 files changed, 22 insertions(+), 24 deletions(-) diff --git a/lib/dns/catz.c b/lib/dns/catz.c index 70b48fdc2c..f790cdc0cf 100644 --- a/lib/dns/catz.c +++ b/lib/dns/catz.c @@ -25,6 +25,7 @@ #include #include #include +#include #include #include @@ -790,7 +791,7 @@ dns_catz_new_zones(isc_mem_t *mctx, isc_taskmgr_t *taskmgr, .zmm = zmm, .magic = DNS_CATZ_ZONES_MAGIC }; - result = isc_task_create_bound(taskmgr, 0, &catzs->updater, 0); + result = isc_taskmgr_excltask(taskmgr, &catzs->updater); if (result != ISC_R_SUCCESS) { goto cleanup_task; } @@ -1019,7 +1020,7 @@ dns__catz_zones_destroy(dns_catz_zones_t *catzs) { REQUIRE(catzs->zones == NULL); catzs->magic = 0; - isc_task_destroy(&catzs->updater); + isc_task_detach(&catzs->updater); isc_mutex_destroy(&catzs->lock); isc_refcount_destroy(&catzs->references); @@ -2227,9 +2228,7 @@ dns_catz_dbupdate_unregister(dns_db_t *db, dns_catz_zones_t *catzs) { REQUIRE(DNS_DB_VALID(db)); REQUIRE(DNS_CATZ_ZONES_VALID(catzs)); - LOCK(&catzs->lock); dns_db_updatenotify_unregister(db, dns_catz_dbupdate_callback, catzs); - UNLOCK(&catzs->lock); } void @@ -2237,9 +2236,7 @@ dns_catz_dbupdate_register(dns_db_t *db, dns_catz_zones_t *catzs) { REQUIRE(DNS_DB_VALID(db)); REQUIRE(DNS_CATZ_ZONES_VALID(catzs)); - LOCK(&catzs->lock); dns_db_updatenotify_register(db, dns_catz_dbupdate_callback, catzs); - UNLOCK(&catzs->lock); } static bool @@ -2305,6 +2302,8 @@ dns__catz_update_cb(void *data) { goto exit; } + INSIST(catz == oldcatz); + if (!is_active) { /* This can happen during a reconfiguration. */ isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, @@ -2540,22 +2539,6 @@ final: ISC_LOG_DEBUG(3), "catz: update_from_db: new zone merged"); - /* - * When we're doing reconfig and setting a new catalog zone - * from an existing zone we won't have a chance to set up - * 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); - if (result == ISC_R_SUCCESS) { - oldcatz->db_registered = true; - } - } - UNLOCK(&catzs->lock); - exit: catz->updateresult = result; } @@ -2576,6 +2559,21 @@ dns__catz_done_cb(void *data, isc_result_t result) { dns_name_format(&catz->name, dname, DNS_NAME_FORMATSIZE); + /* + * When we're doing reconfig and setting a new catalog zone + * from an existing zone we won't have a chance to set up + * update callback in zone_startload or axfr_makedb, but we will + * call onupdate() artificially so we can register the callback + * here. + */ + if (result == ISC_R_SUCCESS && !catz->db_registered) { + result = dns_db_updatenotify_register( + catz->db, dns_catz_dbupdate_callback, catz->catzs); + if (result == ISC_R_SUCCESS) { + catz->db_registered = true; + } + } + /* If there's no update pending, or if shutting down, finish. */ if (!catz->updatepending || atomic_load(&catz->catzs->shuttingdown)) { goto done; diff --git a/lib/dns/rpz.c b/lib/dns/rpz.c index a29b6f8e3d..62d13d3a6a 100644 --- a/lib/dns/rpz.c +++ b/lib/dns/rpz.c @@ -1474,7 +1474,7 @@ dns_rpz_new_zones(isc_mem_t *mctx, isc_taskmgr_t *taskmgr, goto cleanup_rbt; } - result = isc_task_create_bound(taskmgr, 0, &rpzs->updater, 0); + result = isc_taskmgr_excltask(taskmgr, &rpzs->updater); if (result != ISC_R_SUCCESS) { goto cleanup_task; } @@ -2120,7 +2120,7 @@ dns__rpz_zones_destroy(dns_rpz_zones_t *rpzs) { if (rpzs->rbt != NULL) { dns_rbt_destroy(&rpzs->rbt); } - isc_task_destroy(&rpzs->updater); + isc_task_detach(&rpzs->updater); isc_mutex_destroy(&rpzs->maint_lock); isc_rwlock_destroy(&rpzs->search_lock); isc_mem_putanddetach(&rpzs->mctx, rpzs, sizeof(*rpzs));