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.

diff --git a/lib/dns/include/dns/zt.h b/lib/dns/include/dns/zt.h
index 03ead096e113e02a50c3e1f3edaf7576236b2e2c..e325370de29c530eddd99e126d92a10e26f1ba59 100644 (file)
--- 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 b27be5c51273caa49f600d0a057ca92f44c37a18..e86a69046b3b960e18df16a5ed5dd7a7e4dd3d74 100644 (file)
--- 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 0d235a21bbbb6d7bfc58fe5631d89657a8520caa..d4ff6fa53fa4f765cc6b7fddeeb1e8c47e244264 100644 (file)
--- 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