From cd9bbe6dea9c2cc8a7d95ceaee6845108c90361c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= <ondrej@sury.org>
Date: Thu, 1 Aug 2019 11:42:58 +0200
Subject: [PATCH] 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)
(cherry picked from commit 232140edae6b80f8344db234cda28f8456269a6e)
---
 lib/dns/include/dns/view.h |  2 +-
 lib/dns/view.c             | 46 ++++++++++++++++++++------------------
 2 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/lib/dns/include/dns/view.h b/lib/dns/include/dns/view.h
index c4671b47bf..785627225a 100644
--- a/lib/dns/include/dns/view.h
+++ b/lib/dns/include/dns/view.h
@@ -197,9 +197,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 d906ee6be3..239f9d2741 100644
--- a/lib/dns/view.c
+++ b/lib/dns/view.c
@@ -139,7 +139,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;
@@ -149,7 +149,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;
@@ -304,8 +304,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) {
@@ -337,12 +339,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)
@@ -535,6 +538,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) {
@@ -554,9 +559,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);
 }
@@ -671,9 +678,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;
 }
@@ -681,24 +686,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