From bb43ecaf51b7ecacf5b9b2e469e32e92a1711cc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Wed, 3 Jun 2026 11:27:14 +0200 Subject: [PATCH] Fix use-after-free when destroying the bad and unreachable caches Eviction of an entry owned by another loop was bounced to that loop via isc_async_run(), so a queued list removal could run after the cache had freed its LRU lists. Use a single mutex-guarded LRU list instead, removing entries synchronously under the lock, and let each entry hold its own memory-context reference so the RCU free never touches a gone loop. --- lib/dns/badcache.c | 131 +++++++++++++++-------------------------- lib/dns/unreachcache.c | 113 +++++++++++++---------------------- 2 files changed, 91 insertions(+), 153 deletions(-) diff --git a/lib/dns/badcache.c b/lib/dns/badcache.c index 06de7aa4dc..6f89e4fbae 100644 --- a/lib/dns/badcache.c +++ b/lib/dns/badcache.c @@ -16,15 +16,12 @@ #include #include -#include #include #include #include #include #include #include -#include -#include #include #include #include @@ -49,8 +46,8 @@ struct dns_badcache { unsigned int magic; isc_mem_t *mctx; struct cds_lfht *ht; - struct cds_list_head *lru; - uint32_t nloops; + isc_mutex_t lru_lock; + struct cds_list_head lru; }; #define BADCACHE_MAGIC ISC_MAGIC('B', 'd', 'C', 'a') @@ -60,7 +57,8 @@ struct dns_badcache { #define BADCACHE_MIN_SIZE (1 << 8) /* Must be power of 2 */ struct dns_bcentry { - isc_loop_t *loop; + isc_mem_t *mctx; + isc_stdtime_t expire; uint32_t flags; @@ -79,25 +77,21 @@ static void bcentry_destroy(struct rcu_head *rcu_head); static bool -bcentry_alive(struct cds_lfht *ht, dns_bcentry_t *bad, isc_stdtime_t now); +bcentry_alive(dns_badcache_t *bc, dns_bcentry_t *bad, isc_stdtime_t now); dns_badcache_t * dns_badcache_new(isc_mem_t *mctx) { - uint32_t nloops = isc_loopmgr_nloops(); dns_badcache_t *bc = isc_mem_get(mctx, sizeof(*bc)); *bc = (dns_badcache_t){ .magic = BADCACHE_MAGIC, - .nloops = nloops, }; bc->ht = cds_lfht_new(BADCACHE_INIT_SIZE, BADCACHE_MIN_SIZE, 0, CDS_LFHT_AUTO_RESIZE | CDS_LFHT_ACCOUNTING, NULL); INSIST(bc->ht != NULL); - bc->lru = isc_mem_cget(mctx, bc->nloops, sizeof(bc->lru[0])); - for (size_t i = 0; i < bc->nloops; i++) { - CDS_INIT_LIST_HEAD(&bc->lru[i]); - } + isc_mutex_init(&bc->lru_lock); + CDS_INIT_LIST_HEAD(&bc->lru); isc_mem_attach(mctx, &bc->mctx); @@ -121,7 +115,7 @@ dns_badcache_destroy(dns_badcache_t **bcp) { } RUNTIME_CHECK(!cds_lfht_destroy(bc->ht, NULL)); - isc_mem_cput(bc->mctx, bc->lru, bc->nloops, sizeof(bc->lru[0])); + isc_mutex_destroy(&bc->lru_lock); isc_mem_putanddetach(&bc->mctx, bc, sizeof(dns_badcache_t)); } @@ -164,7 +158,7 @@ bcentry_new(isc_loop_t *loop, const dns_name_t *name, .type = type, .flags = flags, .expire = expire, - .loop = isc_loop_ref(loop), + .mctx = isc_mem_ref(mctx), .lru_head = CDS_LIST_HEAD_INIT(bad->lru_head), }; @@ -178,43 +172,31 @@ static void bcentry_destroy(struct rcu_head *rcu_head) { dns_bcentry_t *bad = caa_container_of(rcu_head, dns_bcentry_t, rcu_head); - isc_loop_t *loop = bad->loop; - isc_mem_t *mctx = isc_loop_getmctx(loop); - - dns_name_free(&bad->name, mctx); - isc_mem_put(mctx, bad, sizeof(*bad)); - - isc_loop_unref(loop); + dns_name_free(&bad->name, bad->mctx); + isc_mem_putanddetach(&bad->mctx, bad, sizeof(*bad)); } static void -bcentry_evict_async(void *arg) { - dns_bcentry_t *bad = arg; - - RUNTIME_CHECK(bad->loop == isc_loop()); - - cds_list_del(&bad->lru_head); - call_rcu(&bad->rcu_head, bcentry_destroy); -} - -static void -bcentry_evict(struct cds_lfht *ht, dns_bcentry_t *bad) { - if (!cds_lfht_del(ht, &bad->ht_node)) { - if (bad->loop == isc_loop()) { - bcentry_evict_async(bad); - return; - } - - isc_async_run(bad->loop, bcentry_evict_async, bad); +bcentry_evict_locked(dns_badcache_t *bc, dns_bcentry_t *bad) { + if (!cds_lfht_del(bc->ht, &bad->ht_node)) { + cds_list_del_rcu(&bad->lru_head); + call_rcu(&bad->rcu_head, bcentry_destroy); } } +static void +bcentry_evict(dns_badcache_t *bc, dns_bcentry_t *bad) { + LOCK(&bc->lru_lock); + bcentry_evict_locked(bc, bad); + UNLOCK(&bc->lru_lock); +} + static bool -bcentry_alive(struct cds_lfht *ht, dns_bcentry_t *bad, isc_stdtime_t now) { +bcentry_alive(dns_badcache_t *bc, dns_bcentry_t *bad, isc_stdtime_t now) { if (cds_lfht_is_node_deleted(&bad->ht_node)) { return false; } else if (bad->expire < now) { - bcentry_evict(ht, bad); + bcentry_evict(bc, bad); return false; } @@ -231,12 +213,11 @@ bcentry_alive(struct cds_lfht *ht, dns_bcentry_t *bad, isc_stdtime_t now) { __typeof__(*(pos)), member)) static void -bcentry_purge(struct cds_lfht *ht, struct cds_list_head *lru, - isc_stdtime_t now) { +bcentry_purge(dns_badcache_t *bc, isc_stdtime_t now) { size_t count = 10; dns_bcentry_t *bad; - cds_list_for_each_entry_rcu(bad, lru, lru_head) { - if (bcentry_alive(ht, bad, now)) { + cds_list_for_each_entry_rcu(bad, &bc->lru, lru_head) { + if (bcentry_alive(bc, bad, now)) { break; } if (--count == 0) { @@ -252,17 +233,13 @@ dns_badcache_add(dns_badcache_t *bc, const dns_name_t *name, REQUIRE(name != NULL); isc_loop_t *loop = isc_loop(); - isc_tid_t tid = isc_tid(); - struct cds_list_head *lru = &bc->lru[tid]; - isc_stdtime_t now = isc_stdtime_now(); + if (expire < now) { expire = now; } rcu_read_lock(); - struct cds_lfht *ht = rcu_dereference(bc->ht); - INSIST(ht != NULL); dns__bckey_t key = { .name = name, @@ -272,21 +249,23 @@ dns_badcache_add(dns_badcache_t *bc, const dns_name_t *name, /* struct cds_lfht_iter iter; */ dns_bcentry_t *bad = bcentry_new(loop, name, type, flags, expire); + + LOCK(&bc->lru_lock); struct cds_lfht_node *ht_node; do { - ht_node = cds_lfht_add_unique(ht, hashval, bcentry_match, &key, - &bad->ht_node); + ht_node = cds_lfht_add_unique(bc->ht, hashval, bcentry_match, + &key, &bad->ht_node); if (ht_node != &bad->ht_node) { dns_bcentry_t *found = caa_container_of( ht_node, dns_bcentry_t, ht_node); - bcentry_evict(ht, found); + bcentry_evict_locked(bc, found); } } while (ht_node != &bad->ht_node); - /* No locking, instead we are using per-thread lists */ - cds_list_add_tail_rcu(&bad->lru_head, lru); + cds_list_add_tail_rcu(&bad->lru_head, &bc->lru); + UNLOCK(&bc->lru_lock); - bcentry_purge(ht, lru, now); + bcentry_purge(bc, now); rcu_read_unlock(); } @@ -300,8 +279,6 @@ dns_badcache_find(dns_badcache_t *bc, const dns_name_t *name, isc_result_t result = ISC_R_NOTFOUND; rcu_read_lock(); - struct cds_lfht *ht = rcu_dereference(bc->ht); - INSIST(ht != NULL); dns__bckey_t key = { .name = name, @@ -309,16 +286,14 @@ dns_badcache_find(dns_badcache_t *bc, const dns_name_t *name, }; uint32_t hashval = bcentry_hash(&key); - dns_bcentry_t *found = bcentry_lookup(ht, hashval, &key); + dns_bcentry_t *found = bcentry_lookup(bc->ht, hashval, &key); - if (found != NULL && bcentry_alive(ht, found, now)) { + if (found != NULL && bcentry_alive(bc, found, now)) { result = ISC_R_SUCCESS; SET_IF_NOT_NULL(flagp, found->flags); } - isc_tid_t tid = isc_tid(); - struct cds_list_head *lru = &bc->lru[tid]; - bcentry_purge(ht, lru, now); + bcentry_purge(bc, now); rcu_read_unlock(); @@ -330,14 +305,12 @@ dns_badcache_flush(dns_badcache_t *bc) { REQUIRE(VALID_BADCACHE(bc)); rcu_read_lock(); - struct cds_lfht *ht = rcu_dereference(bc->ht); - INSIST(ht != NULL); /* Flush the hash table */ dns_bcentry_t *bad; struct cds_lfht_iter iter; - cds_lfht_for_each_entry(ht, &iter, bad, ht_node) { - bcentry_evict(ht, bad); + cds_lfht_for_each_entry(bc->ht, &iter, bad, ht_node) { + bcentry_evict(bc, bad); } rcu_read_unlock(); @@ -351,19 +324,17 @@ dns_badcache_flushname(dns_badcache_t *bc, const dns_name_t *name) { isc_stdtime_t now = isc_stdtime_now(); rcu_read_lock(); - struct cds_lfht *ht = rcu_dereference(bc->ht); - INSIST(ht != NULL); dns_bcentry_t *bad; struct cds_lfht_iter iter; - cds_lfht_for_each_entry(ht, &iter, bad, ht_node) { + cds_lfht_for_each_entry(bc->ht, &iter, bad, ht_node) { if (dns_name_equal(&bad->name, name)) { - bcentry_evict(ht, bad); + bcentry_evict(bc, bad); continue; } /* Flush all the expired entries */ - (void)bcentry_alive(ht, bad, now); + (void)bcentry_alive(bc, bad, now); } rcu_read_unlock(); @@ -377,19 +348,17 @@ dns_badcache_flushtree(dns_badcache_t *bc, const dns_name_t *name) { isc_stdtime_t now = isc_stdtime_now(); rcu_read_lock(); - struct cds_lfht *ht = rcu_dereference(bc->ht); - INSIST(ht != NULL); dns_bcentry_t *bad; struct cds_lfht_iter iter; - cds_lfht_for_each_entry(ht, &iter, bad, ht_node) { + cds_lfht_for_each_entry(bc->ht, &iter, bad, ht_node) { if (dns_name_issubdomain(&bad->name, name)) { - bcentry_evict(ht, bad); + bcentry_evict(bc, bad); continue; } /* Flush all the expired entries */ - (void)bcentry_alive(ht, bad, now); + (void)bcentry_alive(bc, bad, now); } rcu_read_unlock(); @@ -417,12 +386,10 @@ dns_badcache_print(dns_badcache_t *bc, const char *cachename, FILE *fp) { fprintf(fp, ";\n; %s\n;\n", cachename); rcu_read_lock(); - struct cds_lfht *ht = rcu_dereference(bc->ht); - INSIST(ht != NULL); struct cds_lfht_iter iter; - cds_lfht_for_each_entry(ht, &iter, bad, ht_node) { - if (bcentry_alive(ht, bad, now)) { + cds_lfht_for_each_entry(bc->ht, &iter, bad, ht_node) { + if (bcentry_alive(bc, bad, now)) { bcentry_print(bad, now, fp); } } diff --git a/lib/dns/unreachcache.c b/lib/dns/unreachcache.c index d24e2d8a8b..b3a526daf9 100644 --- a/lib/dns/unreachcache.c +++ b/lib/dns/unreachcache.c @@ -16,16 +16,13 @@ #include #include -#include #include #include #include #include #include #include -#include #include -#include #include #include #include @@ -53,8 +50,8 @@ struct dns_unreachcache { uint16_t expire_max_s; uint16_t backoff_eligible_s; struct cds_lfht *ht; - struct cds_list_head *lru; - uint32_t nloops; + isc_mutex_t lru_lock; + struct cds_list_head lru; }; #define UNREACHCACHE_MAGIC ISC_MAGIC('U', 'R', 'C', 'a') @@ -64,7 +61,8 @@ struct dns_unreachcache { #define UNREACHCACHE_MIN_SIZE (1 << 5) /* Must be power of 2 */ struct dns_ucentry { - isc_loop_t *loop; + isc_mem_t *mctx; + isc_stdtime_t expire; unsigned int exp_backoff_n; uint16_t wait_time; @@ -82,7 +80,7 @@ static void ucentry_destroy(struct rcu_head *rcu_head); static bool -ucentry_alive(struct cds_lfht *ht, dns_ucentry_t *unreach, isc_stdtime_t now, +ucentry_alive(dns_unreachcache_t *uc, dns_ucentry_t *unreach, isc_stdtime_t now, bool alive_or_waiting); dns_unreachcache_t * @@ -92,24 +90,20 @@ dns_unreachcache_new(isc_mem_t *mctx, const uint16_t expire_min_s, REQUIRE(expire_min_s > 0); REQUIRE(expire_min_s <= expire_max_s); - uint32_t nloops = isc_loopmgr_nloops(); dns_unreachcache_t *uc = isc_mem_get(mctx, sizeof(*uc)); *uc = (dns_unreachcache_t){ .magic = UNREACHCACHE_MAGIC, .expire_min_s = expire_min_s, .expire_max_s = expire_max_s, .backoff_eligible_s = backoff_eligible_s, - .nloops = nloops, }; uc->ht = cds_lfht_new(UNREACHCACHE_INIT_SIZE, UNREACHCACHE_MIN_SIZE, 0, CDS_LFHT_AUTO_RESIZE | CDS_LFHT_ACCOUNTING, NULL); INSIST(uc->ht != NULL); - uc->lru = isc_mem_cget(mctx, uc->nloops, sizeof(uc->lru[0])); - for (size_t i = 0; i < uc->nloops; i++) { - CDS_INIT_LIST_HEAD(&uc->lru[i]); - } + isc_mutex_init(&uc->lru_lock); + CDS_INIT_LIST_HEAD(&uc->lru); isc_mem_attach(mctx, &uc->mctx); @@ -132,7 +126,7 @@ dns_unreachcache_destroy(dns_unreachcache_t **ucp) { } RUNTIME_CHECK(!cds_lfht_destroy(uc->ht, NULL)); - isc_mem_cput(uc->mctx, uc->lru, uc->nloops, sizeof(uc->lru[0])); + isc_mutex_destroy(&uc->lru_lock); isc_mem_putanddetach(&uc->mctx, uc, sizeof(dns_unreachcache_t)); } @@ -174,7 +168,7 @@ ucentry_new(isc_loop_t *loop, const isc_sockaddr_t *remote, .local = *local, .expire = expire, .wait_time = wait_time, - .loop = isc_loop_ref(loop), + .mctx = isc_mem_ref(mctx), .lru_head = CDS_LIST_HEAD_INIT(unreach->lru_head), }; @@ -185,36 +179,26 @@ static void ucentry_destroy(struct rcu_head *rcu_head) { dns_ucentry_t *unreach = caa_container_of(rcu_head, dns_ucentry_t, rcu_head); - isc_loop_t *loop = unreach->loop; - isc_mem_t *mctx = isc_loop_getmctx(loop); - - isc_mem_put(mctx, unreach, sizeof(*unreach)); - isc_loop_unref(loop); + isc_mem_putanddetach(&unreach->mctx, unreach, sizeof(*unreach)); } static void -ucentry_evict_async(void *arg) { - dns_ucentry_t *unreach = arg; - - RUNTIME_CHECK(unreach->loop == isc_loop()); - - cds_list_del(&unreach->lru_head); - call_rcu(&unreach->rcu_head, ucentry_destroy); -} - -static void -ucentry_evict(struct cds_lfht *ht, dns_ucentry_t *unreach) { - if (!cds_lfht_del(ht, &unreach->ht_node)) { - if (unreach->loop == isc_loop()) { - ucentry_evict_async(unreach); - return; - } - isc_async_run(unreach->loop, ucentry_evict_async, unreach); +ucentry_evict_locked(dns_unreachcache_t *uc, dns_ucentry_t *unreach) { + if (!cds_lfht_del(uc->ht, &unreach->ht_node)) { + cds_list_del_rcu(&unreach->lru_head); + call_rcu(&unreach->rcu_head, ucentry_destroy); } } +static void +ucentry_evict(dns_unreachcache_t *uc, dns_ucentry_t *unreach) { + LOCK(&uc->lru_lock); + ucentry_evict_locked(uc, unreach); + UNLOCK(&uc->lru_lock); +} + static bool -ucentry_alive(struct cds_lfht *ht, dns_ucentry_t *unreach, isc_stdtime_t now, +ucentry_alive(dns_unreachcache_t *uc, dns_ucentry_t *unreach, isc_stdtime_t now, bool alive_or_waiting) { if (cds_lfht_is_node_deleted(&unreach->ht_node)) { return false; @@ -236,7 +220,7 @@ ucentry_alive(struct cds_lfht *ht, dns_ucentry_t *unreach, isc_stdtime_t now, } /* The entry is already expired, evict it before returning. */ - ucentry_evict(ht, unreach); + ucentry_evict(uc, unreach); return false; } @@ -244,12 +228,11 @@ ucentry_alive(struct cds_lfht *ht, dns_ucentry_t *unreach, isc_stdtime_t now, } static void -ucentry_purge(struct cds_lfht *ht, struct cds_list_head *lru, - isc_stdtime_t now) { +ucentry_purge(dns_unreachcache_t *uc, isc_stdtime_t now) { size_t count = 10; dns_ucentry_t *unreach; - cds_list_for_each_entry_rcu(unreach, lru, lru_head) { - if (ucentry_alive(ht, unreach, now, true)) { + cds_list_for_each_entry_rcu(unreach, &uc->lru, lru_head) { + if (ucentry_alive(uc, unreach, now, true)) { break; } if (--count == 0) { @@ -290,15 +273,11 @@ dns_unreachcache_add(dns_unreachcache_t *uc, const isc_sockaddr_t *remote, REQUIRE(local != NULL); isc_loop_t *loop = isc_loop(); - isc_tid_t tid = isc_tid(); - struct cds_list_head *lru = &uc->lru[tid]; isc_stdtime_t now = isc_stdtime_now(); isc_stdtime_t expire = now + uc->expire_min_s; bool exp_backoff_activated = false; rcu_read_lock(); - struct cds_lfht *ht = rcu_dereference(uc->ht); - INSIST(ht != NULL); dns__uckey_t key = { .remote = remote, @@ -308,10 +287,12 @@ dns_unreachcache_add(dns_unreachcache_t *uc, const isc_sockaddr_t *remote, dns_ucentry_t *unreach = ucentry_new(loop, remote, local, expire, uc->backoff_eligible_s); + + LOCK(&uc->lru_lock); struct cds_lfht_node *ht_node; do { - ht_node = cds_lfht_add_unique(ht, hashval, ucentry_match, &key, - &unreach->ht_node); + ht_node = cds_lfht_add_unique(uc->ht, hashval, ucentry_match, + &key, &unreach->ht_node); if (ht_node != &unreach->ht_node) { /* The entry already exists, get it. */ dns_ucentry_t *found = caa_container_of( @@ -337,14 +318,14 @@ dns_unreachcache_add(dns_unreachcache_t *uc, const isc_sockaddr_t *remote, * Evict the old entry, so we can try to insert the new * one again. */ - ucentry_evict(ht, found); + ucentry_evict_locked(uc, found); } } while (ht_node != &unreach->ht_node); - /* No locking, instead we are using per-thread lists */ - cds_list_add_tail_rcu(&unreach->lru_head, lru); + cds_list_add_tail_rcu(&unreach->lru_head, &uc->lru); + UNLOCK(&uc->lru_lock); - ucentry_purge(ht, lru, now); + ucentry_purge(uc, now); rcu_read_unlock(); } @@ -360,8 +341,6 @@ dns_unreachcache_find(dns_unreachcache_t *uc, const isc_sockaddr_t *remote, isc_stdtime_t now = isc_stdtime_now(); rcu_read_lock(); - struct cds_lfht *ht = rcu_dereference(uc->ht); - INSIST(ht != NULL); dns__uckey_t key = { .remote = remote, @@ -369,16 +348,14 @@ dns_unreachcache_find(dns_unreachcache_t *uc, const isc_sockaddr_t *remote, }; uint32_t hashval = ucentry_hash(&key); - dns_ucentry_t *found = ucentry_lookup(ht, hashval, &key); + dns_ucentry_t *found = ucentry_lookup(uc->ht, hashval, &key); if (found != NULL && found->confirmed && - ucentry_alive(ht, found, now, false)) + ucentry_alive(uc, found, now, false)) { result = ISC_R_SUCCESS; } - isc_tid_t tid = isc_tid(); - struct cds_list_head *lru = &uc->lru[tid]; - ucentry_purge(ht, lru, now); + ucentry_purge(uc, now); rcu_read_unlock(); @@ -395,8 +372,6 @@ dns_unreachcache_remove(dns_unreachcache_t *uc, const isc_sockaddr_t *remote, isc_stdtime_t now = isc_stdtime_now(); rcu_read_lock(); - struct cds_lfht *ht = rcu_dereference(uc->ht); - INSIST(ht != NULL); dns__uckey_t key = { .remote = remote, @@ -404,14 +379,12 @@ dns_unreachcache_remove(dns_unreachcache_t *uc, const isc_sockaddr_t *remote, }; uint32_t hashval = ucentry_hash(&key); - dns_ucentry_t *found = ucentry_lookup(ht, hashval, &key); + dns_ucentry_t *found = ucentry_lookup(uc->ht, hashval, &key); if (found != NULL) { - ucentry_evict(ht, found); + ucentry_evict(uc, found); } - isc_tid_t tid = isc_tid(); - struct cds_list_head *lru = &uc->lru[tid]; - ucentry_purge(ht, lru, now); + ucentry_purge(uc, now); rcu_read_unlock(); } @@ -421,14 +394,12 @@ dns_unreachcache_flush(dns_unreachcache_t *uc) { REQUIRE(VALID_UNREACHCACHE(uc)); rcu_read_lock(); - struct cds_lfht *ht = rcu_dereference(uc->ht); - INSIST(ht != NULL); /* Flush the hash table */ dns_ucentry_t *unreach; struct cds_lfht_iter iter; - cds_lfht_for_each_entry(ht, &iter, unreach, ht_node) { - ucentry_evict(ht, unreach); + cds_lfht_for_each_entry(uc->ht, &iter, unreach, ht_node) { + ucentry_evict(uc, unreach); } rcu_read_unlock();