lib/dns/resolver.c: Convert (dns_view_t *)->weakrefs to isc_refcount_t

There's a deadlock in BIND 9 code where (dns_view_t){ .lock } and
(dns_resolver_t){ .buckets[i].lock } gets locked in different order.  When
view->weakrefs gets converted to a reference counting we can reduce the locking
in dns_view_weakdetach only to cases where it's the last instance of the
dns_view_t object.

(cherry picked from commit a7c9a52c89146490a0e797df2119026a268f294c)

diff --git a/lib/dns/include/dns/view.h b/lib/dns/include/dns/view.h
index 8015b916bea5e7ee36bc0eca126402abda91bcc5..06a6f8751f06d1bd559bff7c70af6334f6b796f1 100644 (file)
--- a/lib/dns/include/dns/view.h
+++ b/lib/dns/include/dns/view.h
@@ -198,9 +198,9 @@ struct dns_view {
 
        /* Locked by themselves. */
        isc_refcount_t                  references;
+       isc_refcount_t                  weakrefs;
 
        /* Locked by lock. */
-       unsigned int                    weakrefs;
        unsigned int                    attributes;
        /* Under owner's locking control. */
        ISC_LINK(struct dns_view)       link;

diff --git a/lib/dns/view.c b/lib/dns/view.c
index 46f26e925fe33f51d7841736d70ba0e8254e140e..4f47bb7ff635661bb229df711e248a873557e056 100644 (file)
--- a/lib/dns/view.c
+++ b/lib/dns/view.c
@@ -151,7 +151,7 @@ dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass,
        view->frozen = false;
        view->task = NULL;
        isc_refcount_init(&view->references, 1);
-       view->weakrefs = 0;
+       isc_refcount_init(&view->weakrefs, 0);
        view->attributes = (DNS_VIEWATTR_RESSHUTDOWN|DNS_VIEWATTR_ADBSHUTDOWN|
                            DNS_VIEWATTR_REQSHUTDOWN);
        view->statickeys = NULL;
@@ -161,7 +161,7 @@ dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass,
        view->matchrecursiveonly = false;
        result = dns_tsigkeyring_create(view->mctx, &view->dynamickeys);
        if (result != ISC_R_SUCCESS)
-               goto cleanup_references;
+               goto cleanup_weakrefs;
        view->peers = NULL;
        view->order = NULL;
        view->delonly = NULL;
@@ -317,8 +317,10 @@ dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass,
                dns_tsigkeyring_detach(&view->dynamickeys);
        }
 
- cleanup_references:
-       INSIST(isc_refcount_decrement(&view->references) > 0);
+ cleanup_weakrefs:
+       isc_refcount_destroy(&view->weakrefs);
+
+       isc_refcount_decrement(&view->references);
        isc_refcount_destroy(&view->references);
 
        if (view->fwdtable != NULL) {
@@ -352,12 +354,13 @@ destroy(dns_view_t *view) {
        dns_dlzdb_t *dlzdb;
 
        REQUIRE(!ISC_LINK_LINKED(view, link));
-       REQUIRE(isc_refcount_current(&view->references) == 0);
-       REQUIRE(view->weakrefs == 0);
        REQUIRE(RESSHUTDOWN(view));
        REQUIRE(ADBSHUTDOWN(view));
        REQUIRE(REQSHUTDOWN(view));
 
+       isc_refcount_destroy(&view->references);
+       isc_refcount_destroy(&view->weakrefs);
+
        if (view->order != NULL)
                dns_order_detach(&view->order);
        if (view->peers != NULL)
@@ -550,6 +553,8 @@ destroy(dns_view_t *view) {
                dns_badcache_destroy(&view->failcache);
        isc_mutex_destroy(&view->new_zone_lock);
        isc_mutex_destroy(&view->lock);
+       isc_refcount_destroy(&view->references);
+       isc_refcount_destroy(&view->weakrefs);
        isc_mem_free(view->mctx, view->nta_file);
        isc_mem_free(view->mctx, view->name);
        if (view->hooktable != NULL && view->hooktable_free != NULL) {
@@ -569,9 +574,11 @@ static bool
 all_done(dns_view_t *view) {
 
        if (isc_refcount_current(&view->references) == 0 &&
-           view->weakrefs == 0 &&
+           isc_refcount_current(&view->weakrefs) == 0 &&
            RESSHUTDOWN(view) && ADBSHUTDOWN(view) && REQSHUTDOWN(view))
+       {
                return (true);
+       }
 
        return (false);
 }
@@ -686,9 +693,7 @@ dns_view_weakattach(dns_view_t *source, dns_view_t **targetp) {
        REQUIRE(DNS_VIEW_VALID(source));
        REQUIRE(targetp != NULL && *targetp == NULL);
 
-       LOCK(&source->lock);
-       source->weakrefs++;
-       UNLOCK(&source->lock);
+       isc_refcount_increment0(&source->weakrefs);
 
        *targetp = source;
 }
@@ -696,24 +701,21 @@ dns_view_weakattach(dns_view_t *source, dns_view_t **targetp) {
 void
 dns_view_weakdetach(dns_view_t **viewp) {
        dns_view_t *view;
-       bool done = false;
 
        REQUIRE(viewp != NULL);
        view = *viewp;
        REQUIRE(DNS_VIEW_VALID(view));
-
-       LOCK(&view->lock);
-
-       INSIST(view->weakrefs > 0);
-       view->weakrefs--;
-       done = all_done(view);
-
-       UNLOCK(&view->lock);
-
        *viewp = NULL;
 
-       if (done)
-               destroy(view);
+       if (isc_refcount_decrement(&view->weakrefs) == 1) {
+               bool done = false;
+               LOCK(&view->lock);
+               done = all_done(view);
+               UNLOCK(&view->lock);
+               if (done) {
+                       destroy(view);
+               }
+       }
 }
 
 static void