Merge branch '4132-catz-data-race-in-db.c-9.18' into 'bind-9.18'

[9.18] Fix a data race between the dns_zone and dns_catz modules

See merge request isc-projects/bind9!8036

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]

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);
 
@@ -2222,6 +2223,22 @@ 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));
+
+	dns_db_updatenotify_unregister(db, dns_catz_dbupdate_callback, catzs);
+}
+
+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));
+
+	dns_db_updatenotify_register(db, dns_catz_dbupdate_callback, catzs);
+}
+
 static bool
 catz_rdatatype_is_processable(const dns_rdatatype_t type) {
 	return (!dns_rdatatype_isdnssec(type) && type != dns_rdatatype_cds &&
@@ -2285,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,
@@ -2520,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;
 }
@@ -2556,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;

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);
 /*%<

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));

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);
 	}
 }