Use cds_lfht for updatenotify mechanism in dns_db unit

The updatenotify mechanism in dns_db relied on unlocked ISC_LIST for
adding and removing the "listeners".  The mechanism relied on the
exclusive mode - it should have been updated only during reconfiguration
of the server.  This turned not to be true anymore in the dns_catz - the
updatenotify list could have been updated during offloaded work as the
offloaded threads are not subject to the exclusive mode.

Change the update_listeners to be cds_lfht (lock-free hash-table), and
slightly refactor how register and unregister the callbacks - the calls
are now idempotent (the register call already was and the return value
of the unregister function was mostly ignored by the callers).

lib/dns/catz.c

@@ -99,7 +99,6 @@ struct dns_catz_zone {
 	isc_timer_t *updatetimer;
 
 	bool active;
-	bool db_registered;
 	bool broken;
 
 	isc_refcount_t references;
@@ -1005,14 +1004,12 @@ dns__catz_zone_destroy(dns_catz_zone_t *catz) {
 		isc_timer_async_destroy(&catz->updatetimer);
 	}
 
-	if (catz->db_registered) {
+	if (catz->db != NULL) {
+		if (catz->dbversion != NULL) {
+			dns_db_closeversion(catz->db, &catz->dbversion, false);
+		}
 		dns_db_updatenotify_unregister(
 			catz->db, dns_catz_dbupdate_callback, catz->catzs);
-	}
-	if (catz->dbversion != NULL) {
-		dns_db_closeversion(catz->db, &catz->dbversion, false);
-	}
-	if (catz->db != NULL) {
 		dns_db_detach(&catz->db);
 	}
 
@@ -2144,16 +2141,12 @@ dns_catz_dbupdate_callback(dns_db_t *db, void *fn_arg) {
 		dns_db_updatenotify_unregister(
 			catz->db, dns_catz_dbupdate_callback, catz->catzs);
 		dns_db_detach(&catz->db);
-		catz->db_registered = false;
 	}
 	if (catz->db == NULL) {
 		/* New db registration. */
 		dns_db_attach(db, &catz->db);
-		result = dns_db_updatenotify_register(
-			db, dns_catz_dbupdate_callback, catz->catzs);
-		if (result == ISC_R_SUCCESS) {
-			catz->db_registered = true;
-		}
+		dns_db_updatenotify_register(db, dns_catz_dbupdate_callback,
+					     catz->catzs);
 	}
 
 	if (!catz->updatepending && !catz->updaterunning) {
@@ -2186,9 +2179,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
@@ -2196,9 +2187,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
@@ -2495,15 +2484,8 @@ dns__catz_update_cb(void *data) {
 	 * 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);
+	dns_db_updatenotify_register(updb, dns_catz_dbupdate_callback,
+				     oldcatz->catzs);
 
 exit:
 	catz->updateresult = result;

lib/dns/db.c

@@ -21,12 +21,14 @@
 #include <stdbool.h>
 
 #include <isc/buffer.h>
+#include <isc/hash.h>
 #include <isc/mem.h>
 #include <isc/once.h>
 #include <isc/result.h>
 #include <isc/rwlock.h>
 #include <isc/string.h>
 #include <isc/tid.h>
+#include <isc/urcu.h>
 #include <isc/util.h>
 
 #include <dns/callbacks.h>
@@ -97,6 +99,9 @@ impfind(const char *name) {
 	return (NULL);
 }
 
+static void
+call_updatenotify(dns_db_t *db);
+
 /***
  *** Basic DB Methods
  ***/
@@ -265,8 +270,6 @@ dns_db_beginload(dns_db_t *db, dns_rdatacallbacks_t *callbacks) {
 
 isc_result_t
 dns_db_endload(dns_db_t *db, dns_rdatacallbacks_t *callbacks) {
-	dns_dbonupdatelistener_t *listener;
-
 	/*
 	 * Finish loading 'db'.
 	 */
@@ -280,11 +283,7 @@ dns_db_endload(dns_db_t *db, dns_rdatacallbacks_t *callbacks) {
 	 * for all registered listeners, regardless of whether the underlying
 	 * database has an 'endload' implementation.
 	 */
-	for (listener = ISC_LIST_HEAD(db->update_listeners); listener != NULL;
-	     listener = ISC_LIST_NEXT(listener, link))
-	{
-		listener->onupdate(db, listener->onupdate_arg);
-	}
+	call_updatenotify(db);
 
 	if (db->methods->endload != NULL) {
 		return ((db->methods->endload)(db, callbacks));
@@ -385,8 +384,6 @@ dns_db_attachversion(dns_db_t *db, dns_dbversion_t *source,
 void
 dns__db_closeversion(dns_db_t *db, dns_dbversion_t **versionp,
 		     bool commit DNS__DB_FLARG) {
-	dns_dbonupdatelistener_t *listener;
-
 	/*
 	 * Close version '*versionp'.
 	 */
@@ -398,11 +395,7 @@ dns__db_closeversion(dns_db_t *db, dns_dbversion_t **versionp,
 	(db->methods->closeversion)(db, versionp, commit DNS__DB_FLARG_PASS);
 
 	if (commit) {
-		for (listener = ISC_LIST_HEAD(db->update_listeners);
-		     listener != NULL; listener = ISC_LIST_NEXT(listener, link))
-		{
-			listener->onupdate(db, listener->onupdate_arg);
-		}
+		call_updatenotify(db);
 	}
 
 	ENSURE(*versionp == NULL);
@@ -951,59 +944,97 @@ dns__db_getsigningtime(dns_db_t *db, dns_rdataset_t *rdataset,
 	return (ISC_R_NOTFOUND);
 }
 
+static void
+call_updatenotify(dns_db_t *db) {
+	rcu_read_lock();
+	struct cds_lfht *update_listeners =
+		rcu_dereference(db->update_listeners);
+	if (update_listeners != NULL) {
+		struct cds_lfht_iter iter;
+		dns_dbonupdatelistener_t *listener;
+		cds_lfht_for_each_entry(update_listeners, &iter, listener,
+					ht_node) {
+			if (!cds_lfht_is_node_deleted(&listener->ht_node)) {
+				listener->onupdate(db, listener->onupdate_arg);
+			}
+		}
+	}
+	rcu_read_unlock();
+}
+
+static void
+updatenotify_free(struct rcu_head *rcu_head) {
+	dns_dbonupdatelistener_t *listener =
+		caa_container_of(rcu_head, dns_dbonupdatelistener_t, rcu_head);
+	isc_mem_putanddetach(&listener->mctx, listener, sizeof(*listener));
+}
+
+static int
+updatenotify_match(struct cds_lfht_node *ht_node, const void *_key) {
+	const dns_dbonupdatelistener_t *listener =
+		caa_container_of(ht_node, dns_dbonupdatelistener_t, ht_node);
+	const dns_dbonupdatelistener_t *key = _key;
+
+	return (listener->onupdate == key->onupdate &&
+		listener->onupdate_arg == key->onupdate_arg);
+}
+
 /*
  * Attach a notify-on-update function the database
  */
-isc_result_t
+void
 dns_db_updatenotify_register(dns_db_t *db, dns_dbupdate_callback_t fn,
 			     void *fn_arg) {
-	dns_dbonupdatelistener_t *listener;
-
 	REQUIRE(db != NULL);
 	REQUIRE(fn != NULL);
 
-	for (listener = ISC_LIST_HEAD(db->update_listeners); listener != NULL;
-	     listener = ISC_LIST_NEXT(listener, link))
-	{
-		if ((listener->onupdate == fn) &&
-		    (listener->onupdate_arg == fn_arg))
-		{
-			return (ISC_R_SUCCESS);
-		}
+	dns_dbonupdatelistener_t key = { .onupdate = fn,
+					 .onupdate_arg = fn_arg };
+	uint32_t hash = isc_hash32(&key, sizeof(key), true);
+	dns_dbonupdatelistener_t *listener = isc_mem_get(db->mctx,
+							 sizeof(*listener));
+	*listener = key;
+
+	isc_mem_attach(db->mctx, &listener->mctx);
+
+	rcu_read_lock();
+	struct cds_lfht *update_listeners =
+		rcu_dereference(db->update_listeners);
+	INSIST(update_listeners != NULL);
+	struct cds_lfht_node *ht_node =
+		cds_lfht_add_unique(update_listeners, hash, updatenotify_match,
+				    &key, &listener->ht_node);
+	rcu_read_unlock();
+
+	if (ht_node != &listener->ht_node) {
+		updatenotify_free(&listener->rcu_head);
 	}
-
-	listener = isc_mem_get(db->mctx, sizeof(dns_dbonupdatelistener_t));
-
-	listener->onupdate = fn;
-	listener->onupdate_arg = fn_arg;
-
-	ISC_LINK_INIT(listener, link);
-	ISC_LIST_APPEND(db->update_listeners, listener, link);
-
-	return (ISC_R_SUCCESS);
 }
 
-isc_result_t
+void
 dns_db_updatenotify_unregister(dns_db_t *db, dns_dbupdate_callback_t fn,
 			       void *fn_arg) {
-	dns_dbonupdatelistener_t *listener;
-
 	REQUIRE(db != NULL);
 
-	for (listener = ISC_LIST_HEAD(db->update_listeners); listener != NULL;
-	     listener = ISC_LIST_NEXT(listener, link))
-	{
-		if ((listener->onupdate == fn) &&
-		    (listener->onupdate_arg == fn_arg))
-		{
-			ISC_LIST_UNLINK(db->update_listeners, listener, link);
-			isc_mem_put(db->mctx, listener,
-				    sizeof(dns_dbonupdatelistener_t));
-			return (ISC_R_SUCCESS);
-		}
-	}
+	dns_dbonupdatelistener_t key = { .onupdate = fn,
+					 .onupdate_arg = fn_arg };
+	uint32_t hash = isc_hash32(&key, sizeof(key), true);
+	struct cds_lfht_iter iter;
 
-	return (ISC_R_NOTFOUND);
+	rcu_read_lock();
+	struct cds_lfht *update_listeners =
+		rcu_dereference(db->update_listeners);
+	INSIST(update_listeners != NULL);
+	cds_lfht_lookup(update_listeners, hash, updatenotify_match, &key,
+			&iter);
+
+	struct cds_lfht_node *ht_node = cds_lfht_iter_get_node(&iter);
+	if (ht_node != NULL && !cds_lfht_del(update_listeners, ht_node)) {
+		dns_dbonupdatelistener_t *listener = caa_container_of(
+			ht_node, dns_dbonupdatelistener_t, ht_node);
+		call_rcu(&listener->rcu_head, updatenotify_free);
+	}
+	rcu_read_unlock();
 }
 
 isc_result_t

lib/dns/include/dns/db.h

@@ -58,6 +58,7 @@
 #include <isc/rwlock.h>
 #include <isc/stats.h>
 #include <isc/stdtime.h>
+#include <isc/urcu.h>
 
 #include <dns/clientinfo.h>
 #include <dns/fixedname.h>
@@ -212,7 +213,7 @@ struct dns_db {
 	dns_ttl_t	 serve_stale_ttl; /* for cache DB's only */
 	isc_mem_t	*mctx;
 	isc_refcount_t	 references;
-	ISC_LIST(dns_dbonupdatelistener_t) update_listeners;
+	struct cds_lfht *update_listeners;
 };
 
 enum {
@@ -221,9 +222,11 @@ enum {
 };
 
 struct dns_dbonupdatelistener {
+	isc_mem_t	       *mctx;
 	dns_dbupdate_callback_t onupdate;
 	void		       *onupdate_arg;
-	ISC_LINK(dns_dbonupdatelistener_t) link;
+	struct cds_lfht_node	ht_node;
+	struct rcu_head		rcu_head;
 };
 
 /*@{*/
@@ -1617,7 +1620,7 @@ dns_db_setcachestats(dns_db_t *db, isc_stats_t *stats);
  *	dns_rdatasetstats_create(); otherwise NULL.
  */
 
-isc_result_t
+void
 dns_db_updatenotify_register(dns_db_t *db, dns_dbupdate_callback_t fn,
 			     void *fn_arg);
 /*%<
@@ -1631,7 +1634,7 @@ dns_db_updatenotify_register(dns_db_t *db, dns_dbupdate_callback_t fn,
  *
  */
 
-isc_result_t
+void
 dns_db_updatenotify_unregister(dns_db_t *db, dns_dbupdate_callback_t fn,
 			       void *fn_arg);
 /*%<

lib/dns/include/dns/rpz.h

@@ -402,6 +402,10 @@ dns_rpz_new_zone(dns_rpz_zones_t *rpzs, dns_rpz_zone_t **rpzp);
 
 isc_result_t
 dns_rpz_dbupdate_callback(dns_db_t *db, void *fn_arg);
+void
+dns_rpz_dbupdate_unregister(dns_db_t *db, dns_rpz_zone_t *rpz);
+void
+dns_rpz_dbupdate_register(dns_db_t *db, dns_rpz_zone_t *rpz);
 
 void
 dns_rpz_zones_shutdown(dns_rpz_zones_t *rpzs);

lib/dns/rbtdb.c

@@ -578,7 +578,9 @@ free_rbtdb(dns_rbtdb_t *rbtdb, bool log) {
 	rbtdb->common.impmagic = 0;
 	isc_mem_detach(&rbtdb->hmctx);
 
-	INSIST(ISC_LIST_EMPTY(rbtdb->common.update_listeners));
+	if (rbtdb->common.update_listeners != NULL) {
+		INSIST(!cds_lfht_destroy(rbtdb->common.update_listeners, NULL));
+	}
 
 	isc_mem_putanddetach(&rbtdb->common.mctx, rbtdb, sizeof(*rbtdb));
 }
@@ -3750,7 +3752,6 @@ dns__rbtdb_create(isc_mem_t *mctx, const dns_name_t *origin, dns_dbtype_t type,
 	*rbtdb = (dns_rbtdb_t){
 		.common.origin = DNS_NAME_INITEMPTY,
 		.common.rdclass = rdclass,
-		.common.update_listeners = ISC_LIST_INITIALIZER,
 		.current_serial = 1,
 		.least_serial = 1,
 		.next_serial = 2,
@@ -3799,6 +3800,9 @@ dns__rbtdb_create(isc_mem_t *mctx, const dns_name_t *origin, dns_dbtype_t type,
 	rbtdb->node_locks = isc_mem_get(mctx, rbtdb->node_lock_count *
 						      sizeof(rbtdb_nodelock_t));
 
+	rbtdb->common.update_listeners =
+		cds_lfht_new(16, 16, 0, CDS_LFHT_ACCOUNTING, NULL);
+
 	if (IS_CACHE(rbtdb)) {
 		dns_rdatasetstats_create(mctx, &rbtdb->rrsetstats);
 		rbtdb->lru = isc_mem_get(mctx,

lib/dns/rpz.c

@@ -1624,6 +1624,21 @@ unlock:
 	return (result);
 }
 
+void
+dns_rpz_dbupdate_unregister(dns_db_t *db, dns_rpz_zone_t *rpz) {
+	REQUIRE(DNS_DB_VALID(db));
+	REQUIRE(DNS_RPZ_ZONE_VALID(rpz));
+
+	dns_db_updatenotify_unregister(db, dns_rpz_dbupdate_callback, rpz);
+}
+
+void
+dns_rpz_dbupdate_register(dns_db_t *db, dns_rpz_zone_t *rpz) {
+	REQUIRE(DNS_DB_VALID(db));
+	REQUIRE(DNS_RPZ_ZONE_VALID(rpz));
+
+	dns_db_updatenotify_register(db, dns_rpz_dbupdate_callback, rpz);
+}
 static void
 dns__rpz_timer_start(dns_rpz_zone_t *rpz) {
 	uint64_t tdiff;

lib/dns/zone.c

@@ -1896,14 +1896,11 @@ dns_zone_get_rpz_num(dns_zone_t *zone) {
  */
 void
 dns_zone_rpz_enable_db(dns_zone_t *zone, dns_db_t *db) {
-	isc_result_t result;
 	if (zone->rpz_num == DNS_RPZ_INVALID_NUM) {
 		return;
 	}
 	REQUIRE(zone->rpzs != NULL);
-	result = dns_db_updatenotify_register(db, dns_rpz_dbupdate_callback,
-					      zone->rpzs->zones[zone->rpz_num]);
-	REQUIRE(result == ISC_R_SUCCESS);
+	dns_rpz_dbupdate_register(db, zone->rpzs->zones[zone->rpz_num]);
 }
 
 static void
@@ -1912,8 +1909,7 @@ dns_zone_rpz_disable_db(dns_zone_t *zone, dns_db_t *db) {
 		return;
 	}
 	REQUIRE(zone->rpzs != NULL);
-	(void)dns_db_updatenotify_unregister(db, dns_rpz_dbupdate_callback,
-					     zone->rpzs->zones[zone->rpz_num]);
+	dns_rpz_dbupdate_unregister(db, zone->rpzs->zones[zone->rpz_num]);
 }
 
 /*