From 26bb402b44987f3235b3db65b7b4806d38ebb930 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= <ondrej@isc.org>
Date: Tue, 4 Jul 2023 15:15:12 +0200
Subject: [PATCH] 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 <isc/print.h>
 #include <isc/result.h>
 #include <isc/task.h>
+#include <isc/thread.h>
 #include <isc/util.h>
 
 #include <dns/catz.h>
@@ -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));