From ace7c879a82eeebc9113380ea4842256a727fe54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Mon, 17 Feb 2025 14:58:28 +0100 Subject: [PATCH 1/2] Add isc_timer_running() function to check status of timer In the next commit, we need to know whether the timer has been started or stopped. Add isc_timer_running() function that returns true if the timer has been started. (cherry picked from commit b9e3cd5d2a75f962a1e88cbe676cf875a796543d) --- lib/isc/include/isc/timer.h | 10 ++++++++++ lib/isc/timer.c | 8 ++++++++ 2 files changed, 18 insertions(+) diff --git a/lib/isc/include/isc/timer.h b/lib/isc/include/isc/timer.h index 0c0cf1a8d6..b039b33cf0 100644 --- a/lib/isc/include/isc/timer.h +++ b/lib/isc/include/isc/timer.h @@ -156,4 +156,14 @@ isc_timer_destroy(isc_timer_t **timerp); *\li *timerp is NULL. */ +bool +isc_timer_running(isc_timer_t *timer); +/*%< + * Return true if the timer has been started. + * + * Requires: + * + *\li 'timer' is a valid timer* + */ + ISC_LANG_ENDDECLS diff --git a/lib/isc/timer.c b/lib/isc/timer.c index 4d30409084..bfd3377f4c 100644 --- a/lib/isc/timer.c +++ b/lib/isc/timer.c @@ -16,6 +16,7 @@ #include #include +#include #include #include #include @@ -196,3 +197,10 @@ isc_timer_async_destroy(isc_timer_t **timerp) { isc_timer_stop(timer); isc_async_run(timer->loop, timer_destroy, timer); } + +bool +isc_timer_running(isc_timer_t *timer) { + REQUIRE(VALID_TIMER(timer)); + + return atomic_load_acquire(&timer->running); +} From eec7b79ee085829a16574ec42b71e703fd16c87a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Wed, 19 Feb 2025 06:28:46 +0100 Subject: [PATCH 2/2] Fix the fetch context hash table lock ordering The order of the fetch context hash table rwlock and the individual fetch context was reversed when calling the release_fctx() function. This was causing a problem when iterating the hash table, and thus the ordering has been corrected in a way that the hash table rwlock is now always locked on the outside and the fctx lock is the interior lock. (cherry picked from commit cf078fadebcf73184a64cf46d28c3f40b54f1867) --- lib/dns/resolver.c | 145 +++++++++++++++++++++------------------------ 1 file changed, 67 insertions(+), 78 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index b6ba7a5a3f..f2f97055b9 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -357,7 +357,6 @@ struct fetchctx { /*% Locked by lock. */ isc_mutex_t lock; fetchstate_t state; - bool hashed; bool cloned; bool spilled; ISC_LINK(struct fetchctx) link; @@ -599,9 +598,6 @@ struct dns_resolver { /* Locked by primelock. */ dns_fetch_t *primefetch; - /* Atomic. */ - atomic_uint_fast32_t nfctx; - uint32_t nloops; isc_mempool_t **namepools; @@ -717,8 +713,6 @@ get_attached_fctx(dns_resolver_t *res, isc_loop_t *loop, const dns_name_t *name, dns_rdataset_t *nameservers, const isc_sockaddr_t *client, unsigned int options, unsigned int depth, isc_counter_t *qc, isc_counter_t *gqc, fetchctx_t **fctxp, bool *new_fctx); -static void -release_fctx(fetchctx_t *fctx); /*% * The structure and functions defined below implement the resolver @@ -1656,6 +1650,25 @@ fctx_sendevents(fetchctx_t *fctx, isc_result_t result) { } } +static uint32_t +fctx_hash(fetchctx_t *fctx) { + isc_hash32_t hash32; + isc_hash32_init(&hash32); + isc_hash32_hash(&hash32, fctx->name->ndata, fctx->name->length, false); + isc_hash32_hash(&hash32, &fctx->options, sizeof(fctx->options), true); + isc_hash32_hash(&hash32, &fctx->type, sizeof(fctx->type), true); + return isc_hash32_finalize(&hash32); +} + +static bool +fctx_match(void *node, const void *key) { + const fetchctx_t *fctx0 = node; + const fetchctx_t *fctx1 = key; + + return fctx0->options == fctx1->options && fctx0->type == fctx1->type && + dns_name_equal(fctx0->name, fctx1->name); +} + static bool fctx__done(fetchctx_t *fctx, isc_result_t result, const char *func, const char *file, unsigned int line) { @@ -1683,11 +1696,15 @@ fctx__done(fetchctx_t *fctx, isc_result_t result, const char *func, return false; } fctx->state = fetchstate_done; - release_fctx(fctx); - FCTX_ATTR_CLR(fctx, FCTX_ATTR_ADDRWAIT); UNLOCK(&fctx->lock); + /* The fctx will get deleted either here or in get_attached_fctx() */ + RWLOCK(&fctx->res->fctxs_lock, isc_rwlocktype_write); + (void)isc_hashmap_delete(fctx->res->fctxs, fctx_hash(fctx), match_ptr, + fctx); + RWUNLOCK(&fctx->res->fctxs_lock, isc_rwlocktype_write); + if (result == ISC_R_SUCCESS) { if (fctx->qmin_warning != ISC_R_SUCCESS) { isc_log_write(dns_lctx, DNS_LOGCATEGORY_LAME_SERVERS, @@ -4330,7 +4347,6 @@ fctx_destroy(fetchctx_t *fctx) { dns_resolver_t *res = NULL; isc_sockaddr_t *sa = NULL, *next_sa = NULL; struct tried *tried = NULL; - uint_fast32_t nfctx; REQUIRE(VALID_FCTX(fctx)); REQUIRE(ISC_LIST_EMPTY(fctx->resps)); @@ -4349,9 +4365,6 @@ fctx_destroy(fetchctx_t *fctx) { dec_stats(res, dns_resstatscounter_nfetch); - nfctx = atomic_fetch_sub_release(&res->nfctx, 1); - INSIST(nfctx > 0); - /* Free bad */ for (sa = ISC_LIST_HEAD(fctx->bad); sa != NULL; sa = next_sa) { next_sa = ISC_LIST_NEXT(sa, link); @@ -4539,7 +4552,6 @@ fctx_create(dns_resolver_t *res, isc_loop_t *loop, const dns_name_t *name, isc_interval_t interval; unsigned int findoptions = 0; char buf[DNS_NAME_FORMATSIZE + DNS_RDATATYPE_FORMATSIZE + 1]; - uint_fast32_t nfctx; isc_mem_t *mctx = isc_loop_getmctx(loop); size_t p; @@ -4796,9 +4808,6 @@ fctx_create(dns_resolver_t *res, isc_loop_t *loop, const dns_name_t *name, fctx_minimize_qname(fctx); } - nfctx = atomic_fetch_add_relaxed(&res->nfctx, 1); - INSIST(nfctx < UINT32_MAX); - inc_stats(res, dns_resstatscounter_nfetch); isc_timer_create(fctx->loop, fctx_expired, fctx, &fctx->timer); @@ -7037,43 +7046,6 @@ ISC_REFCOUNT_TRACE_IMPL(fetchctx, fctx_destroy); ISC_REFCOUNT_IMPL(fetchctx, fctx_destroy); #endif -static uint32_t -fctx_hash(fetchctx_t *fctx) { - isc_hash32_t hash32; - isc_hash32_init(&hash32); - isc_hash32_hash(&hash32, fctx->name->ndata, fctx->name->length, false); - isc_hash32_hash(&hash32, &fctx->options, sizeof(fctx->options), true); - isc_hash32_hash(&hash32, &fctx->type, sizeof(fctx->type), true); - return isc_hash32_finalize(&hash32); -} - -static bool -fctx_match(void *node, const void *key) { - const fetchctx_t *fctx0 = node; - const fetchctx_t *fctx1 = key; - - return fctx0->options == fctx1->options && fctx0->type == fctx1->type && - dns_name_equal(fctx0->name, fctx1->name); -} - -/* Must be fctx locked */ -static void -release_fctx(fetchctx_t *fctx) { - isc_result_t result; - dns_resolver_t *res = fctx->res; - - if (!fctx->hashed) { - return; - } - - RWLOCK(&res->fctxs_lock, isc_rwlocktype_write); - result = isc_hashmap_delete(res->fctxs, fctx_hash(fctx), match_ptr, - fctx); - INSIST(result == ISC_R_SUCCESS); - fctx->hashed = false; - RWUNLOCK(&res->fctxs_lock, isc_rwlocktype_write); -} - static void resume_dslookup(void *arg) { dns_fetchresponse_t *resp = (dns_fetchresponse_t *)arg; @@ -9952,8 +9924,6 @@ dns_resolver__destroy(dns_resolver_t *res) { RTRACE("destroy"); - REQUIRE(atomic_load_acquire(&res->nfctx) == 0); - res->magic = 0; dns_nametree_detach(&res->algorithms); @@ -10433,7 +10403,8 @@ again: result = fctx_create(res, loop, name, type, domain, nameservers, client, options, depth, qc, gqc, &fctx); if (result != ISC_R_SUCCESS) { - goto unlock; + RWUNLOCK(&res->fctxs_lock, locktype); + return result; } UPGRADELOCK(&res->fctxs_lock, locktype); @@ -10443,39 +10414,57 @@ again: fctx, &found); if (result == ISC_R_SUCCESS) { *new_fctx = true; - fctx->hashed = true; } else { - fctx_done_detach(&fctx, result); + /* + * The fctx_done() tries to acquire the fctxs_lock. + * Destroy the newly created fetchctx directly. + */ + fctx->state = fetchstate_done; + isc_timer_destroy(&fctx->timer); + + fetchctx_detach(&fctx); fctx = found; result = ISC_R_SUCCESS; } - INSIST(result == ISC_R_SUCCESS); break; default: UNREACHABLE(); } + INSIST(result == ISC_R_SUCCESS); fetchctx_ref(fctx); -unlock: - RWUNLOCK(&res->fctxs_lock, locktype); - if (result == ISC_R_SUCCESS) { - LOCK(&fctx->lock); - if (SHUTTINGDOWN(fctx) || fctx->cloned) { - /* - * This is the single place where fctx might get - * accesses from a different thread, so we need to - * double check whether fctxs is done (or cloned) and - * help with the release if the fctx has been cloned. - */ - release_fctx(fctx); - UNLOCK(&fctx->lock); - fetchctx_detach(&fctx); - goto again; - } - INSIST(!SHUTTINGDOWN(fctx)); - *fctxp = fctx; + /* + * We need to lock the fetch context before unlocking the hash table to + * prevent other threads from looking up this thread before it has been + * properly initialized and started. + */ + LOCK(&fctx->lock); + RWUNLOCK(&res->fctxs_lock, locktype); + + if (SHUTTINGDOWN(fctx) || fctx->cloned) { + /* + * This is the single place where fctx might get + * accesses from a different thread, so we need to + * double check whether fctxs is done (or cloned) and + * help with the release if the fctx has been cloned. + */ + UNLOCK(&fctx->lock); + + /* The fctx will get deleted either here or in fctx__done() */ + RWLOCK(&res->fctxs_lock, isc_rwlocktype_write); + (void)isc_hashmap_delete(res->fctxs, fctx_hash(fctx), match_ptr, + fctx); + RWUNLOCK(&res->fctxs_lock, isc_rwlocktype_write); + + fetchctx_detach(&fctx); + goto again; } + /* + * The function returns a locked fetch context, + */ + *fctxp = fctx; + return result; } @@ -10578,7 +10567,7 @@ dns_resolver_createfetch(dns_resolver_t *res, const dns_name_t *name, result = fctx_create(res, loop, name, type, domain, nameservers, client, options, depth, qc, gqc, &fctx); if (result != ISC_R_SUCCESS) { - goto unlock; + goto fail; } new_fctx = true; }