From 232140edae6b80f8344db234cda28f8456269a6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 1 Aug 2019 11:42:58 +0200 Subject: [PATCH 1/3] 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) --- 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 8015b916be..06a6f8751f 100644 --- 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 46f26e925f..4f47bb7ff6 100644 --- 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 From e3946327035832ec03c064410e5c6881841f8f6f Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Tue, 6 Aug 2019 12:40:04 +1000 Subject: [PATCH 2/3] Have the view hold a weakref until all external references are removed so that cleanup can all be done in dns_view_weakattach(). (cherry picked from commit be8af3afb711a04ac140b1debce3b950922d714f) --- lib/dns/view.c | 62 +++++++++----------------------------------------- 1 file changed, 11 insertions(+), 51 deletions(-) diff --git a/lib/dns/view.c b/lib/dns/view.c index 4f47bb7ff6..463f78b275 100644 --- 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); - isc_refcount_init(&view->weakrefs, 0); + isc_refcount_init(&view->weakrefs, 1); view->attributes = (DNS_VIEWATTR_RESSHUTDOWN|DNS_VIEWATTR_ADBSHUTDOWN| DNS_VIEWATTR_REQSHUTDOWN); view->statickeys = NULL; @@ -318,6 +318,7 @@ dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, } cleanup_weakrefs: + isc_refcount_decrement(&view->weakrefs); isc_refcount_destroy(&view->weakrefs); isc_refcount_decrement(&view->references); @@ -566,23 +567,6 @@ destroy(dns_view_t *view) { isc_mem_putanddetach(&view->mctx, view, sizeof(*view)); } -/* - * Return true iff 'view' may be freed. - * The caller must be holding the view lock. - */ -static bool -all_done(dns_view_t *view) { - - if (isc_refcount_current(&view->references) == 0 && - isc_refcount_current(&view->weakrefs) == 0 && - RESSHUTDOWN(view) && ADBSHUTDOWN(view) && REQSHUTDOWN(view)) - { - return (true); - } - - return (false); -} - void dns_view_attach(dns_view_t *source, dns_view_t **targetp) { @@ -604,7 +588,6 @@ view_flushanddetach(dns_view_t **viewp, bool flush) { view->flush = flush; } - bool done = false; if (isc_refcount_decrement(&view->references) == 1) { dns_zone_t *mkzone = NULL, *rdzone = NULL; @@ -642,7 +625,6 @@ view_flushanddetach(dns_view_t **viewp, bool flush) { if (view->catzs != NULL) { dns_catz_catzs_detach(&view->catzs); } - done = all_done(view); UNLOCK(&view->lock); /* Need to detach zones outside view lock */ @@ -653,12 +635,8 @@ view_flushanddetach(dns_view_t **viewp, bool flush) { if (rdzone != NULL) { dns_zone_detach(&rdzone); } - } - *viewp = NULL; - - if (done) { - destroy(view); + dns_view_weakdetach(&view); } } @@ -693,7 +671,7 @@ dns_view_weakattach(dns_view_t *source, dns_view_t **targetp) { REQUIRE(DNS_VIEW_VALID(source)); REQUIRE(targetp != NULL && *targetp == NULL); - isc_refcount_increment0(&source->weakrefs); + isc_refcount_increment(&source->weakrefs); *targetp = source; } @@ -708,20 +686,13 @@ dns_view_weakdetach(dns_view_t **viewp) { *viewp = NULL; if (isc_refcount_decrement(&view->weakrefs) == 1) { - bool done = false; - LOCK(&view->lock); - done = all_done(view); - UNLOCK(&view->lock); - if (done) { - destroy(view); - } + destroy(view); } } static void resolver_shutdown(isc_task_t *task, isc_event_t *event) { dns_view_t *view = event->ev_arg; - bool done; REQUIRE(event->ev_type == DNS_EVENT_VIEWRESSHUTDOWN); REQUIRE(DNS_VIEW_VALID(view)); @@ -732,20 +703,15 @@ resolver_shutdown(isc_task_t *task, isc_event_t *event) { isc_event_free(&event); LOCK(&view->lock); - view->attributes |= DNS_VIEWATTR_RESSHUTDOWN; - done = all_done(view); - UNLOCK(&view->lock); - if (done) - destroy(view); + dns_view_weakdetach(&view); } static void adb_shutdown(isc_task_t *task, isc_event_t *event) { dns_view_t *view = event->ev_arg; - bool done; REQUIRE(event->ev_type == DNS_EVENT_VIEWADBSHUTDOWN); REQUIRE(DNS_VIEW_VALID(view)); @@ -756,20 +722,15 @@ adb_shutdown(isc_task_t *task, isc_event_t *event) { isc_event_free(&event); LOCK(&view->lock); - view->attributes |= DNS_VIEWATTR_ADBSHUTDOWN; - done = all_done(view); - UNLOCK(&view->lock); - if (done) - destroy(view); + dns_view_weakdetach(&view); } static void req_shutdown(isc_task_t *task, isc_event_t *event) { dns_view_t *view = event->ev_arg; - bool done; REQUIRE(event->ev_type == DNS_EVENT_VIEWREQSHUTDOWN); REQUIRE(DNS_VIEW_VALID(view)); @@ -780,14 +741,10 @@ req_shutdown(isc_task_t *task, isc_event_t *event) { isc_event_free(&event); LOCK(&view->lock); - view->attributes |= DNS_VIEWATTR_REQSHUTDOWN; - done = all_done(view); - UNLOCK(&view->lock); - if (done) - destroy(view); + dns_view_weakdetach(&view); } isc_result_t @@ -836,6 +793,7 @@ dns_view_createresolver(dns_view_t *view, event = &view->resevent; dns_resolver_whenshutdown(view->resolver, view->task, &event); view->attributes &= ~DNS_VIEWATTR_RESSHUTDOWN; + isc_refcount_increment(&view->weakrefs); result = isc_mem_create(0, 0, &mctx); if (result != ISC_R_SUCCESS) { @@ -853,6 +811,7 @@ dns_view_createresolver(dns_view_t *view, event = &view->adbevent; dns_adb_whenshutdown(view->adb, view->task, &event); view->attributes &= ~DNS_VIEWATTR_ADBSHUTDOWN; + isc_refcount_increment(&view->weakrefs); result = dns_requestmgr_create(view->mctx, timermgr, socketmgr, dns_resolver_taskmgr(view->resolver), @@ -867,6 +826,7 @@ dns_view_createresolver(dns_view_t *view, event = &view->reqevent; dns_requestmgr_whenshutdown(view->requestmgr, view->task, &event); view->attributes &= ~DNS_VIEWATTR_REQSHUTDOWN; + isc_refcount_increment(&view->weakrefs); return (ISC_R_SUCCESS); } From ebc48cda26ff3935406f48bb745561cc00fed046 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 6 Aug 2019 17:35:20 +0200 Subject: [PATCH 3/3] Have the dns_client hold a .references until all external references are removed so that cleanup can all be done in dns_client_destroy(). (cherry picked from commit e80c4c3431504a462ce03bffbb9229b9b34395d1) --- lib/dns/client.c | 114 +++++++++++++++++++++-------------------------- 1 file changed, 50 insertions(+), 64 deletions(-) diff --git a/lib/dns/client.c b/lib/dns/client.c index 422a763617..57038befe5 100644 --- a/lib/dns/client.c +++ b/lib/dns/client.c @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -94,8 +95,9 @@ struct dns_client { unsigned int find_timeout; unsigned int find_udpretries; + isc_refcount_t references; + /* Locked */ - unsigned int references; dns_viewlist_t viewlist; ISC_LIST(struct resctx) resctxs; ISC_LIST(struct reqctx) reqctxs; @@ -502,12 +504,13 @@ dns_client_createx(isc_mem_t *mctx, isc_appctx_t *actx, client->task = NULL; result = isc_task_create(client->taskmgr, 0, &client->task); - if (result != ISC_R_SUCCESS) - goto cleanup; + if (result != ISC_R_SUCCESS) { + goto cleanup_lock; + } result = dns_dispatchmgr_create(mctx, &dispatchmgr); if (result != ISC_R_SUCCESS) - goto cleanup; + goto cleanup_task; client->dispatchmgr = dispatchmgr; (void)setsourceports(mctx, dispatchmgr); @@ -520,8 +523,9 @@ dns_client_createx(isc_mem_t *mctx, isc_appctx_t *actx, result = getudpdispatch(AF_INET, dispatchmgr, socketmgr, taskmgr, true, &dispatchv4, localaddr4); - if (result == ISC_R_SUCCESS) + if (result == ISC_R_SUCCESS) { client->dispatchv4 = dispatchv4; + } } client->dispatchv6 = NULL; @@ -529,22 +533,27 @@ dns_client_createx(isc_mem_t *mctx, isc_appctx_t *actx, result = getudpdispatch(AF_INET6, dispatchmgr, socketmgr, taskmgr, true, &dispatchv6, localaddr6); - if (result == ISC_R_SUCCESS) + if (result == ISC_R_SUCCESS) { client->dispatchv6 = dispatchv6; + } } /* We need at least one of the dispatchers */ if (dispatchv4 == NULL && dispatchv6 == NULL) { INSIST(result != ISC_R_SUCCESS); - goto cleanup; + goto cleanup_dispatchmgr; } + isc_refcount_init(&client->references, 1); + /* Create the default view for class IN */ result = createview(mctx, dns_rdataclass_in, options, taskmgr, RESOLVER_NTASKS, socketmgr, timermgr, dispatchmgr, dispatchv4, dispatchv6, &view); - if (result != ISC_R_SUCCESS) - goto cleanup; + if (result != ISC_R_SUCCESS) { + goto cleanup_references; + } + ISC_LIST_INIT(client->viewlist); ISC_LIST_APPEND(client->viewlist, view, link); @@ -564,32 +573,38 @@ dns_client_createx(isc_mem_t *mctx, isc_appctx_t *actx, client->find_udpretries = DEF_FIND_UDPRETRIES; client->attributes = 0; - client->references = 1; client->magic = DNS_CLIENT_MAGIC; *clientp = client; return (ISC_R_SUCCESS); - cleanup: - if (dispatchv4 != NULL) + cleanup_references: + isc_refcount_decrement(&client->references); + isc_refcount_destroy(&client->references); + cleanup_dispatchmgr: + if (dispatchv4 != NULL) { dns_dispatch_detach(&dispatchv4); - if (dispatchv6 != NULL) + } + if (dispatchv6 != NULL) { dns_dispatch_detach(&dispatchv6); - if (dispatchmgr != NULL) - dns_dispatchmgr_destroy(&dispatchmgr); - if (client->task != NULL) - isc_task_detach(&client->task); + } + dns_dispatchmgr_destroy(&dispatchmgr); + cleanup_task: + isc_task_detach(&client->task); + cleanup_lock: + isc_mutex_destroy(&client->lock); isc_mem_put(mctx, client, sizeof(*client)); return (result); } static void -destroyclient(dns_client_t **clientp) { - dns_client_t *client = *clientp; +destroyclient(dns_client_t *client) { dns_view_t *view; + isc_refcount_destroy(&client->references); + while ((view = ISC_LIST_HEAD(client->viewlist)) != NULL) { ISC_LIST_UNLINK(client->viewlist, view, link); dns_view_detach(&view); @@ -621,32 +636,20 @@ destroyclient(dns_client_t **clientp) { client->magic = 0; isc_mem_putanddetach(&client->mctx, client, sizeof(*client)); - - *clientp = NULL; } void dns_client_destroy(dns_client_t **clientp) { dns_client_t *client; - bool destroyok = false; REQUIRE(clientp != NULL); client = *clientp; + *clientp = NULL; REQUIRE(DNS_CLIENT_VALID(client)); - LOCK(&client->lock); - client->references--; - if (client->references == 0 && ISC_LIST_EMPTY(client->resctxs) && - ISC_LIST_EMPTY(client->reqctxs) && - ISC_LIST_EMPTY(client->updatectxs)) { - destroyok = true; + if (isc_refcount_decrement(&client->references) == 1) { + destroyclient(client); } - UNLOCK(&client->lock); - - if (destroyok) - destroyclient(&client); - - *clientp = NULL; } isc_result_t @@ -1436,6 +1439,7 @@ dns_client_startresolve(dns_client_t *client, const dns_name_t *name, rctx->event = event; rctx->magic = RCTX_MAGIC; + isc_refcount_increment(&client->references); LOCK(&client->lock); ISC_LIST_APPEND(client->resctxs, rctx, link); @@ -1506,10 +1510,10 @@ dns_client_destroyrestrans(dns_clientrestrans_t **transp) { resctx_t *rctx; isc_mem_t *mctx; dns_client_t *client; - bool need_destroyclient = false; REQUIRE(transp != NULL); rctx = (resctx_t *)*transp; + *transp = NULL; REQUIRE(RCTX_VALID(rctx)); REQUIRE(rctx->fetch == NULL); REQUIRE(rctx->event == NULL); @@ -1531,11 +1535,6 @@ dns_client_destroyrestrans(dns_clientrestrans_t **transp) { INSIST(ISC_LINK_LINKED(rctx, link)); ISC_LIST_UNLINK(client->resctxs, rctx, link); - if (client->references == 0 && ISC_LIST_EMPTY(client->resctxs) && - ISC_LIST_EMPTY(client->reqctxs) && - ISC_LIST_EMPTY(client->updatectxs)) - need_destroyclient = true; - UNLOCK(&client->lock); INSIST(ISC_LIST_EMPTY(rctx->namelist)); @@ -1545,10 +1544,8 @@ dns_client_destroyrestrans(dns_clientrestrans_t **transp) { isc_mem_put(mctx, rctx, sizeof(*rctx)); - if (need_destroyclient) - destroyclient(&client); + dns_client_destroy(&client); - *transp = NULL; } isc_result_t @@ -1814,6 +1811,7 @@ dns_client_startrequest(dns_client_t *client, dns_message_t *qmessage, LOCK(&client->lock); ISC_LIST_APPEND(client->reqctxs, ctx, link); + isc_refcount_increment(&client->references); UNLOCK(&client->lock); ctx->request = NULL; @@ -1828,6 +1826,8 @@ dns_client_startrequest(dns_client_t *client, dns_message_t *qmessage, return (ISC_R_SUCCESS); } + isc_refcount_decrement(&client->references); + cleanup: if (ctx != NULL) { LOCK(&client->lock); @@ -1868,10 +1868,10 @@ dns_client_destroyreqtrans(dns_clientreqtrans_t **transp) { reqctx_t *ctx; isc_mem_t *mctx; dns_client_t *client; - bool need_destroyclient = false; REQUIRE(transp != NULL); ctx = (reqctx_t *)*transp; + *transp = NULL; REQUIRE(REQCTX_VALID(ctx)); client = ctx->client; REQUIRE(DNS_CLIENT_VALID(client)); @@ -1886,12 +1886,6 @@ dns_client_destroyreqtrans(dns_clientreqtrans_t **transp) { INSIST(ISC_LINK_LINKED(ctx, link)); ISC_LIST_UNLINK(client->reqctxs, ctx, link); - if (client->references == 0 && ISC_LIST_EMPTY(client->resctxs) && - ISC_LIST_EMPTY(client->reqctxs) && - ISC_LIST_EMPTY(client->updatectxs)) { - need_destroyclient = true; - } - UNLOCK(&client->lock); isc_mutex_destroy(&ctx->lock); @@ -1899,10 +1893,7 @@ dns_client_destroyreqtrans(dns_clientreqtrans_t **transp) { isc_mem_put(mctx, ctx, sizeof(*ctx)); - if (need_destroyclient) - destroyclient(&client); - - *transp = NULL; + dns_client_destroy(&client); } /*% @@ -2988,6 +2979,7 @@ dns_client_startupdate(dns_client_t *client, dns_rdataclass_t rdclass, LOCK(&client->lock); ISC_LIST_APPEND(client->updatectxs, uctx, link); + isc_refcount_increment(&client->references); UNLOCK(&client->lock); *transp = (dns_clientupdatetrans_t *)uctx; @@ -3006,6 +2998,8 @@ dns_client_startupdate(dns_client_t *client, dns_rdataclass_t rdclass, } if (result == ISC_R_SUCCESS) return (result); + + isc_refcount_decrement(&client->references); *transp = NULL; fail: @@ -3063,11 +3057,11 @@ dns_client_destroyupdatetrans(dns_clientupdatetrans_t **transp) { updatectx_t *uctx; isc_mem_t *mctx; dns_client_t *client; - bool need_destroyclient = false; isc_sockaddr_t *sa; REQUIRE(transp != NULL); uctx = (updatectx_t *)*transp; + *transp = NULL; REQUIRE(UCTX_VALID(uctx)); client = uctx->client; REQUIRE(DNS_CLIENT_VALID(client)); @@ -3088,11 +3082,6 @@ dns_client_destroyupdatetrans(dns_clientupdatetrans_t **transp) { INSIST(ISC_LINK_LINKED(uctx, link)); ISC_LIST_UNLINK(client->updatectxs, uctx, link); - if (client->references == 0 && ISC_LIST_EMPTY(client->resctxs) && - ISC_LIST_EMPTY(client->reqctxs) && - ISC_LIST_EMPTY(client->updatectxs)) - need_destroyclient = true; - UNLOCK(&client->lock); isc_mutex_destroy(&uctx->lock); @@ -3100,10 +3089,7 @@ dns_client_destroyupdatetrans(dns_clientupdatetrans_t **transp) { isc_mem_put(mctx, uctx, sizeof(*uctx)); - if (need_destroyclient) - destroyclient(&client); - - *transp = NULL; + dns_client_destroy(&client); } isc_mem_t *