From 28d0c37ef0ea310571eb8ecab0f28207edb04881 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Sun, 23 Oct 2022 11:39:44 -0700 Subject: [PATCH] Call dns_resolver_createfetch() asynchronously in zone_refreshkeys() Because dns_resolver_createfetch() locks the view, it was necessary to unlock the zone in zone_refreshkeys() before calling it in order to maintain the lock order, and relock afterward. this permitted a race with dns_zone_synckeyzone(). This commit moves the call to dns_resolver_createfetch() into a separate function which is called asynchronously after the zone has been unlocked. The keyfetch object now attaches to the zone to ensure that it won't be shut down before the asynchronous call completes. This necessitated refactoring dns_zone_detach() so it always runs unlocked. For managed zones it schedules zone_shutdown() to run asynchronously; for unmanaged zones there is no task. --- lib/dns/zone.c | 165 ++++++++++++++++++++++----------------- tests/dns/zonemgr_test.c | 7 +- 2 files changed, 96 insertions(+), 76 deletions(-) diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 8821d1f862..3b280ab495 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -5756,56 +5756,43 @@ dns_zone_attach(dns_zone_t *source, dns_zone_t **target) { void dns_zone_detach(dns_zone_t **zonep) { REQUIRE(zonep != NULL && DNS_ZONE_VALID(*zonep)); + dns_zone_t *zone = *zonep; *zonep = NULL; - bool free_now = false; - dns_zone_t *raw = NULL; - dns_zone_t *secure = NULL; if (isc_refcount_decrement(&zone->erefs) == 1) { isc_refcount_destroy(&zone->erefs); - LOCK_ZONE(zone); - INSIST(zone != zone->raw); /* * We just detached the last external reference. */ if (zone->task != NULL) { /* - * This zone is being managed. Post - * its control event and let it clean - * up synchronously in the context of - * its task. + * This zone has a task; it can clean + * itself up asynchronously. */ isc_event_t *ev = &zone->ctlevent; isc_task_send(zone->task, &ev); - } else { - /* - * This zone is not being managed; it has - * no task and can have no outstanding - * events. Free it immediately. - */ - /* - * Unmanaged zones should not have non-null views; - * we have no way of detaching from the view here - * without causing deadlock because this code is called - * with the view already locked. - */ - INSIST(zone->view == NULL); - free_now = true; - raw = zone->raw; - zone->raw = NULL; - secure = zone->secure; - zone->secure = NULL; + return; } - UNLOCK_ZONE(zone); - } - if (free_now) { - if (raw != NULL) { - dns_zone_detach(&raw); + + /* + * This zone is unmanaged; we're probably running in + * named-checkzone or a unit test. There's no task, + * so we need to free it immediately. + * + * Unmanaged zones must not have null views; + * we have no way of detaching from the view here + * without causing deadlock because this code is + * called with the view already locked. + */ + INSIST(zone->view == NULL); + + if (zone->raw != NULL) { + dns_zone_detach(&zone->raw); } - if (secure != NULL) { - dns_zone_idetach(&secure); + if (zone->secure != NULL) { + dns_zone_idetach(&zone->secure); } zone_free(zone); } @@ -11147,7 +11134,9 @@ cleanup: isc_refcount_decrement(&zone->irefs); - kfetch->zone = NULL; + /* The zone must be managed */ + INSIST(kfetch->zone->task != NULL); + dns_zone_detach(&kfetch->zone); if (dns_rdataset_isassociated(keydataset)) { dns_rdataset_disassociate(keydataset); @@ -11168,6 +11157,7 @@ cleanup: free_needed = exit_check(zone); UNLOCK_ZONE(zone); + if (free_needed) { zone_free(zone); } @@ -11175,6 +11165,62 @@ cleanup: INSIST(ver == NULL); } +static void +retry_keyfetch(dns_keyfetch_t *kfetch, dns_name_t *kname) { + isc_time_t timenow, timethen; + char timebuf[80]; + dns_zone_t *zone = kfetch->zone; + + /* + * Error during a key fetch; cancel and retry in an hour. + */ + zone->refreshkeycount--; + isc_refcount_decrement(&zone->irefs); + dns_db_detach(&kfetch->db); + dns_rdataset_disassociate(&kfetch->keydataset); + dns_name_free(kname, zone->mctx); + isc_mem_put(zone->mctx, kfetch, sizeof(dns_keyfetch_t)); + dnssec_log(zone, ISC_LOG_WARNING, + "Failed to create fetch for DNSKEY update"); + + TIME_NOW(&timenow); + DNS_ZONE_TIME_ADD(&timenow, dns_zone_mkey_hour, &timethen); + + LOCK_ZONE(zone); + zone->refreshkeytime = timethen; + zone_settimer(zone, &timenow); + UNLOCK_ZONE(zone); + + isc_time_formattimestamp(&zone->refreshkeytime, timebuf, 80); + dnssec_log(zone, ISC_LOG_DEBUG(1), "retry key refresh: %s", timebuf); + + dns_zone_detach(&zone); +} + +static void +do_keyfetch(isc_task_t *task, isc_event_t *event) { + isc_result_t result; + dns_keyfetch_t *kfetch = (dns_keyfetch_t *)event->ev_arg; + dns_name_t *kname = dns_fixedname_name(&kfetch->name); + dns_zone_t *zone = kfetch->zone; + + UNUSED(task); + + isc_event_free(&event); + + result = dns_resolver_createfetch( + zone->view->resolver, kname, dns_rdatatype_dnskey, NULL, NULL, + NULL, NULL, 0, + DNS_FETCHOPT_NOVALIDATE | DNS_FETCHOPT_UNSHARED | + DNS_FETCHOPT_NOCACHED, + 0, NULL, zone->task, keyfetch_done, kfetch, &kfetch->dnskeyset, + &kfetch->dnskeysigset, &kfetch->fetch); + + if (result != ISC_R_SUCCESS) { + retry_keyfetch(kfetch, kname); + } +} + /* * Refresh the data in the key zone. Initiate a fetch to look up * DNSKEY records at the trust anchor name. @@ -11191,7 +11237,7 @@ zone_refreshkeys(dns_zone_t *zone) { dns_rdata_keydata_t kd; isc_stdtime_t now; bool commit = false; - bool fetching = false, fetch_err = false; + bool fetching = false; bool timerset = false; ENTER; @@ -11276,9 +11322,10 @@ zone_refreshkeys(dns_zone_t *zone) { dns_rriterator_pause(&rrit); kfetch = isc_mem_get(zone->mctx, sizeof(dns_keyfetch_t)); + *kfetch = (dns_keyfetch_t){ .zone = NULL }; zone->refreshkeycount++; - kfetch->zone = zone; + dns_zone_attach(zone, &kfetch->zone); isc_refcount_increment0(&zone->irefs); kname = dns_fixedname_initname(&kfetch->name); dns_name_dup(name, zone->mctx, kname); @@ -11312,35 +11359,22 @@ zone_refreshkeys(dns_zone_t *zone) { #ifdef ENABLE_AFL if (!dns_fuzzing_resolver) { #endif /* ifdef ENABLE_AFL */ - UNLOCK_ZONE(zone); - result = dns_resolver_createfetch( - zone->view->resolver, kname, - dns_rdatatype_dnskey, NULL, NULL, NULL, NULL, 0, - DNS_FETCHOPT_NOVALIDATE | - DNS_FETCHOPT_UNSHARED | - DNS_FETCHOPT_NOCACHED, - 0, NULL, zone->task, keyfetch_done, kfetch, - &kfetch->dnskeyset, &kfetch->dnskeysigset, - &kfetch->fetch); - LOCK_ZONE(zone); -#ifdef ENABLE_AFL - } else { - result = ISC_R_FAILURE; - } -#endif /* ifdef ENABLE_AFL */ - if (result == ISC_R_SUCCESS) { + isc_event_t *e = isc_event_allocate( + zone->mctx, NULL, DNS_EVENT_ZONE, do_keyfetch, + kfetch, sizeof(isc_event_t)); + isc_task_send(zone->task, &e); fetching = true; +#ifdef ENABLE_AFL } else { zone->refreshkeycount--; isc_refcount_decrement(&zone->irefs); dns_db_detach(&kfetch->db); dns_rdataset_disassociate(&kfetch->keydataset); dns_name_free(kname, zone->mctx); + dns_zone_detach(&kfetch->zone); isc_mem_put(zone->mctx, kfetch, sizeof(dns_keyfetch_t)); - dnssec_log(zone, ISC_LOG_WARNING, - "Failed to create fetch for DNSKEY update"); - fetch_err = true; } +#endif /* ifdef ENABLE_AFL */ } if (!ISC_LIST_EMPTY(diff.tuples)) { CHECK(update_soa_serial(zone, db, ver, &diff, zone->mctx, @@ -11352,22 +11386,7 @@ zone_refreshkeys(dns_zone_t *zone) { } failure: - if (fetch_err) { - /* - * Error during a key fetch; retry in an hour. - */ - isc_time_t timenow, timethen; - char timebuf[80]; - - TIME_NOW(&timenow); - DNS_ZONE_TIME_ADD(&timenow, dns_zone_mkey_hour, &timethen); - zone->refreshkeytime = timethen; - zone_settimer(zone, &timenow); - - isc_time_formattimestamp(&zone->refreshkeytime, timebuf, 80); - dnssec_log(zone, ISC_LOG_DEBUG(1), "retry key refresh: %s", - timebuf); - } else if (!timerset) { + if (!timerset) { isc_time_settoepoch(&zone->refreshkeytime); } diff --git a/tests/dns/zonemgr_test.c b/tests/dns/zonemgr_test.c index c26d3fdebd..7eeaf770ef 100644 --- a/tests/dns/zonemgr_test.c +++ b/tests/dns/zonemgr_test.c @@ -113,9 +113,10 @@ ISC_RUN_TEST_IMPL(dns_zonemgr_createzone) { assert_int_equal(result, ISC_R_SUCCESS); assert_non_null(zone); - if (zone != NULL) { - dns_zone_detach(&zone); - } + result = dns_zonemgr_managezone(myzonemgr, zone); + assert_int_equal(result, ISC_R_SUCCESS); + + dns_zone_detach(&zone); dns_zonemgr_shutdown(myzonemgr); dns_zonemgr_detach(&myzonemgr);