From f326d45135c79545cf9dcca89fdb504887ba42f8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= <ondrej@isc.org>
Date: Mon, 3 Jan 2022 21:37:46 +0100
Subject: [PATCH] Lock view while accessing its zone table

Commit 308bc46a59302c88ecff11d4831475ecfa8b8fb0 introduced a change to
the view_flushanddetach() function which makes the latter access
view->zonetable without holding view->lock.  As confirmed by TSAN, this
enables races between threads for view->zonetable accesses.

Swap the view->zonetable pointer under view lock and then detach the
local swapped dns_zt_t later when the view lock is already unlocked.

This commit also changes the dns_zt interfaces, so the setting the
zonetable "flush" flag is separate operation to dns_zt_detach,
e.g. instead of doing:

    if (view->flush) {
        dns_zt_flushanddetach(&zt);
    } else {
        dns_zt_detach(&zt);
    }

the code is now:

    if (view->flush) {
        dns_zt_flush(zt);
    }
    dns_zt_detach(&zt);

making the code more consistent with how we handle flushing and
detaching dns_zone_t pointers from the view.
---
 lib/dns/include/dns/zt.h |  9 ++++-----
 lib/dns/view.c           | 20 ++++++++++++++------
 lib/dns/zt.c             | 19 ++++++-------------
 3 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/lib/dns/include/dns/zt.h b/lib/dns/include/dns/zt.h
index 03ead096e1..e325370de2 100644
--- a/lib/dns/include/dns/zt.h
+++ b/lib/dns/include/dns/zt.h
@@ -117,14 +117,13 @@ dns_zt_detach(dns_zt_t **ztp);
  */
 
 void
-dns_zt_flushanddetach(dns_zt_t **ztp);
+dns_zt_flush(dns_zt_t *ztp);
 /*%<
- * Detach the given zonetable, if the reference count goes to zero the
- * zonetable will be flushed and then freed.  In either case 'ztp' is
- * set to NULL.
+ * Schedule flushing of the given zonetable, when reference count goes
+ * to zero.
  *
  * Requires:
- * \li	'*ztp' to be valid
+ * \li	'ztp' to be valid
  */
 
 void
diff --git a/lib/dns/view.c b/lib/dns/view.c
index b27be5c512..e86a69046b 100644
--- a/lib/dns/view.c
+++ b/lib/dns/view.c
@@ -632,6 +632,7 @@ view_flushanddetach(dns_view_t **viewp, bool flush) {
 
 	if (isc_refcount_decrement(&view->references) == 1) {
 		dns_zone_t *mkzone = NULL, *rdzone = NULL;
+		dns_zt_t *zt = NULL;
 
 		isc_refcount_destroy(&view->references);
 
@@ -645,13 +646,16 @@ view_flushanddetach(dns_view_t **viewp, bool flush) {
 			dns_requestmgr_shutdown(view->requestmgr);
 		}
 
-		if (view->zonetable != NULL && view->flush) {
-			dns_zt_flushanddetach(&view->zonetable);
-		} else if (view->zonetable != NULL) {
-			dns_zt_detach(&view->zonetable);
+		LOCK(&view->lock);
+
+		if (view->zonetable != NULL) {
+			zt = view->zonetable;
+			view->zonetable = NULL;
+			if (view->flush) {
+				dns_zt_flush(zt);
+			}
 		}
 
-		LOCK(&view->lock);
 		if (view->managed_keys != NULL) {
 			mkzone = view->managed_keys;
 			view->managed_keys = NULL;
@@ -674,7 +678,11 @@ view_flushanddetach(dns_view_t **viewp, bool flush) {
 		}
 		UNLOCK(&view->lock);
 
-		/* Need to detach zones outside view lock */
+		/* Need to detach zt and zones outside view lock */
+		if (zt != NULL) {
+			dns_zt_detach(&zt);
+		}
+
 		if (mkzone != NULL) {
 			dns_zone_detach(&mkzone);
 		}
diff --git a/lib/dns/zt.c b/lib/dns/zt.c
index 0d235a21bb..d4ff6fa53f 100644
--- a/lib/dns/zt.c
+++ b/lib/dns/zt.c
@@ -225,14 +225,15 @@ zt_destroy(dns_zt_t *zt) {
 	if (atomic_load_acquire(&zt->flush)) {
 		(void)dns_zt_apply(zt, false, NULL, flush, NULL);
 	}
+
 	dns_rbt_destroy(&zt->table);
 	isc_rwlock_destroy(&zt->rwlock);
 	zt->magic = 0;
 	isc_mem_putanddetach(&zt->mctx, zt, sizeof(*zt));
 }
 
-static void
-zt_flushanddetach(dns_zt_t **ztp, bool need_flush) {
+void
+dns_zt_detach(dns_zt_t **ztp) {
 	dns_zt_t *zt;
 
 	REQUIRE(ztp != NULL && VALID_ZT(*ztp));
@@ -240,23 +241,15 @@ zt_flushanddetach(dns_zt_t **ztp, bool need_flush) {
 	zt = *ztp;
 	*ztp = NULL;
 
-	if (need_flush) {
-		atomic_store_release(&zt->flush, true);
-	}
-
 	if (isc_refcount_decrement(&zt->references) == 1) {
 		zt_destroy(zt);
 	}
 }
 
 void
-dns_zt_flushanddetach(dns_zt_t **ztp) {
-	zt_flushanddetach(ztp, true);
-}
-
-void
-dns_zt_detach(dns_zt_t **ztp) {
-	zt_flushanddetach(ztp, false);
+dns_zt_flush(dns_zt_t *zt) {
+	REQUIRE(VALID_ZT(zt));
+	atomic_store_release(&zt->flush, true);
 }
 
 isc_result_t