mir/bind
2
0
Fork 0
mirror of https://gitlab.isc.org/isc-projects/bind9 synced 2025-08-22 10:10:06 +00:00
Code Issues Releases Wiki Activity

Extract the resigning heap into a separate struct

Browse Source 
In the current implementation, the resigning heap is part of the zone
database. This leads to a cycle, as the database has a reference to its
nodes, but each node needs a reference to the database.

This MR splits the resigning heap into its own separate struct, in order
to help breaking the cycle.
This commit is contained in:
Alessio Podda 2025-07-08 16:29:56 +02:00
parent c2a84bb17a
commit 0b1785ec10
2 changed files with 104 additions and 38 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

140
lib/dns/qpzone.c
View File

@ -159,10 +159,23 @@ struct qpz_version {
typedef ISC_LIST(qpz_version_t) qpz_versionlist_t; typedef ISC_LIST(qpz_version_t) qpz_versionlist_t;
/* Resigning heap indirection to allow ref counting */
typedef struct qpz_heap {
isc_mem_t *mctx;
isc_refcount_t references;
/* Locks the data in this struct */
isc_mutex_t lock;
isc_heap_t *heap;
} qpz_heap_t;
ISC_REFCOUNT_STATIC_DECL(qpz_heap);
struct qpznode { struct qpznode {
dns_name_t name; dns_name_t name;
isc_mem_t *mctx; isc_mem_t *mctx;
qpz_heap_t *heap;
/* /*
* 'erefs' counts external references held by a caller: for * 'erefs' counts external references held by a caller: for
* example, it could be incremented by dns_db_findnode(), * example, it could be incremented by dns_db_findnode(),
@ -235,7 +248,7 @@ struct qpzonedb {
isc_loop_t *loop; isc_loop_t *loop;
struct rcu_head rcu_head; struct rcu_head rcu_head;
isc_heap_t *heap; /* Resigning heap */ qpz_heap_t *heap; /* Resigning heap */
dns_qpmulti_t *tree; /* Main QP trie for data storage */ dns_qpmulti_t *tree; /* Main QP trie for data storage */
dns_qpmulti_t *nsec; /* NSEC nodes only */ dns_qpmulti_t *nsec; /* NSEC nodes only */
@ -518,11 +531,12 @@ free_db_rcu(struct rcu_head *rcu_head) {
if (dns_name_dynamic(&qpdb->common.origin)) { if (dns_name_dynamic(&qpdb->common.origin)) {
dns_name_free(&qpdb->common.origin, qpdb->common.mctx); dns_name_free(&qpdb->common.origin, qpdb->common.mctx);
} }
for (size_t i = 0; i < DEFAULT_BUCKETS_COUNT; i++) { for (size_t i = 0; i < DEFAULT_BUCKETS_COUNT; i++) {
NODE_DESTROYLOCK(&qpdb->buckets[i].lock); NODE_DESTROYLOCK(&qpdb->buckets[i].lock);
} }
isc_heap_destroy(&qpdb->heap); qpz_heap_detach(&qpdb->heap);
if (qpdb->gluecachestats != NULL) { if (qpdb->gluecachestats != NULL) {
isc_stats_detach(&qpdb->gluecachestats); isc_stats_detach(&qpdb->gluecachestats);
@ -598,17 +612,65 @@ qpdb_destroy(dns_db_t *arg) {
qpzonedb_detach(&qpdb); qpzonedb_detach(&qpdb);
} }
static qpz_heap_t *
new_qpz_heap(isc_mem_t *mctx) {
qpz_heap_t *new_heap = isc_mem_get(mctx, sizeof(*new_heap));
*new_heap = (qpz_heap_t){
.references = ISC_REFCOUNT_INITIALIZER(1),
};
isc_mutex_init(&new_heap->lock);
isc_heap_create(mctx, resign_sooner, set_index, 0, &new_heap->heap);
isc_mem_attach(mctx, &new_heap->mctx);
return new_heap;
}
/*
* This function accesses the heap lock through the header and node rather than
* directly through &qpdb->heap->lock to handle a critical race condition.
*
* Consider this scenario:
* 1. A reference is taken to a qpznode
* 2. The database containing that node is freed
* 3. The qpznode reference is finally released
*
* When the qpznode reference is released, it needs to unregister all its
* slabheaders from the resigning heap. The heap is a separate refcounted
* object with references from both the database and every qpznode. This
* design ensures that even after the database is destroyed, if nodes are
* still alive, the heap remains accessible for safe cleanup.
*
* Accessing the heap lock through the database (&qpdb->heap->lock) would
* cause a segfault in this scenario, even though the heap itself is still
* alive. By going through the node's heap reference, we maintain safe access
* to the heap lock regardless of the database's lifecycle.
*/
static isc_mutex_t *
get_heap_lock(dns_slabheader_t *header) {
return &HEADERNODE(header)->heap->lock;
}
static void
qpz_heap_destroy(qpz_heap_t *qpheap) {
isc_mutex_destroy(&qpheap->lock);
isc_heap_destroy(&qpheap->heap);
isc_mem_putanddetach(&qpheap->mctx, qpheap, sizeof(*qpheap));
}
static qpznode_t * static qpznode_t *
new_qpznode(qpzonedb_t *qpdb, const dns_name_t *name) { new_qpznode(qpzonedb_t *qpdb, const dns_name_t *name) {
qpznode_t *newdata = isc_mem_get(qpdb->common.mctx, sizeof(*newdata)); qpznode_t *newdata = isc_mem_get(qpdb->common.mctx, sizeof(*newdata));
*newdata = (qpznode_t){ *newdata = (qpznode_t){
.name = DNS_NAME_INITEMPTY, .name = DNS_NAME_INITEMPTY,
.heap = qpdb->heap,
.references = ISC_REFCOUNT_INITIALIZER(1), .references = ISC_REFCOUNT_INITIALIZER(1),
.locknum = qpzone_get_locknum(), .locknum = qpzone_get_locknum(),
}; };
isc_mem_attach(qpdb->common.mctx, &newdata->mctx); isc_mem_attach(qpdb->common.mctx, &newdata->mctx);
dns_name_dup(name, qpdb->common.mctx, &newdata->name); dns_name_dup(name, qpdb->common.mctx, &newdata->name);
qpz_heap_ref(newdata->heap);
#if DNS_DB_NODETRACE #if DNS_DB_NODETRACE
fprintf(stderr, "new_qpznode:%s:%s:%d:%p->references = 1\n", __func__, fprintf(stderr, "new_qpznode:%s:%s:%d:%p->references = 1\n", __func__,
@ -666,7 +728,7 @@ dns__qpzone_create(isc_mem_t *mctx, const dns_name_t *origin, dns_dbtype_t type,
qpdb->common.update_listeners = cds_lfht_new(16, 16, 0, 0, NULL); qpdb->common.update_listeners = cds_lfht_new(16, 16, 0, 0, NULL);
isc_heap_create(mctx, resign_sooner, set_index, 0, &qpdb->heap); qpdb->heap = new_qpz_heap(mctx);
for (size_t i = 0; i < DEFAULT_BUCKETS_COUNT; i++) { for (size_t i = 0; i < DEFAULT_BUCKETS_COUNT; i++) {
NODE_INITLOCK(&qpdb->buckets[i].lock); NODE_INITLOCK(&qpdb->buckets[i].lock);
@ -1249,15 +1311,13 @@ newversion(dns_db_t *db, dns_dbversion_t **versionp) {
} }
static void static void
resigninsert(qpzonedb_t *qpdb, dns_slabheader_t *newheader) { resigninsert(dns_slabheader_t *newheader) {
REQUIRE(newheader->heap_index == 0); REQUIRE(newheader->heap_index == 0);
REQUIRE(!ISC_LINK_LINKED(newheader, link)); REQUIRE(!ISC_LINK_LINKED(newheader, link));
RWLOCK(&qpdb->lock, isc_rwlocktype_write); LOCK(get_heap_lock(newheader));
isc_heap_insert(qpdb->heap, newheader); isc_heap_insert(HEADERNODE(newheader)->heap->heap, newheader);
RWUNLOCK(&qpdb->lock, isc_rwlocktype_write); UNLOCK(get_heap_lock(newheader));
newheader->heap = qpdb->heap;
} }
static void static void
@ -1267,9 +1327,9 @@ resigndelete(qpzonedb_t *qpdb, qpz_version_t *version,
return; return;
} }
RWLOCK(&qpdb->lock, isc_rwlocktype_write); LOCK(get_heap_lock(header));
isc_heap_delete(qpdb->heap, header->heap_index); isc_heap_delete(HEADERNODE(header)->heap->heap, header->heap_index);
RWUNLOCK(&qpdb->lock, isc_rwlocktype_write); UNLOCK(get_heap_lock(header));
header->heap_index = 0; header->heap_index = 0;
qpznode_acquire(qpdb, HEADERNODE(header) DNS__DB_FLARG_PASS); qpznode_acquire(qpdb, HEADERNODE(header) DNS__DB_FLARG_PASS);
@ -1507,7 +1567,7 @@ closeversion(dns_db_t *db, dns_dbversion_t **versionp,
nlock = qpzone_get_lock(qpdb, HEADERNODE(header)); nlock = qpzone_get_lock(qpdb, HEADERNODE(header));
NODE_WRLOCK(nlock, &nlocktype); NODE_WRLOCK(nlock, &nlocktype);
if (rollback && !IGNORE(header)) { if (rollback && !IGNORE(header)) {
resigninsert(qpdb, header); resigninsert(header);
} }
qpznode_release(qpdb, HEADERNODE(header), least_serial, qpznode_release(qpdb, HEADERNODE(header), least_serial,
&nlocktype DNS__DB_FLARG_PASS); &nlocktype DNS__DB_FLARG_PASS);
@ -1925,7 +1985,7 @@ add(qpzonedb_t *qpdb, qpznode_t *node, const dns_name_t *nodename,
if (loading) { if (loading) {
newheader->down = NULL; newheader->down = NULL;
if (RESIGN(newheader)) { if (RESIGN(newheader)) {
resigninsert(qpdb, newheader); resigninsert(newheader);
/* resigndelete not needed here */ /* resigndelete not needed here */
} }
@ -1946,7 +2006,7 @@ add(qpzonedb_t *qpdb, qpznode_t *node, const dns_name_t *nodename,
dns_slabheader_destroy(&header); dns_slabheader_destroy(&header);
} else { } else {
if (RESIGN(newheader)) { if (RESIGN(newheader)) {
resigninsert(qpdb, newheader); resigninsert(newheader);
resigndelete(qpdb, version, resigndelete(qpdb, version,
header DNS__DB_FLARG_PASS); header DNS__DB_FLARG_PASS);
} }
@ -1978,7 +2038,7 @@ add(qpzonedb_t *qpdb, qpznode_t *node, const dns_name_t *nodename,
} }
if (RESIGN(newheader)) { if (RESIGN(newheader)) {
resigninsert(qpdb, newheader); resigninsert(newheader);
resigndelete(qpdb, version, header DNS__DB_FLARG_PASS); resigndelete(qpdb, version, header DNS__DB_FLARG_PASS);
} }
@ -2400,20 +2460,22 @@ setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, isc_stdtime_t resign) {
} }
if (header->heap_index != 0) { if (header->heap_index != 0) {
INSIST(RESIGN(header)); INSIST(RESIGN(header));
RWLOCK(&qpdb->lock, isc_rwlocktype_write); LOCK(get_heap_lock(header));
if (resign == 0) { if (resign == 0) {
isc_heap_delete(qpdb->heap, header->heap_index); isc_heap_delete(HEADERNODE(header)->heap->heap,
header->heap_index);
header->heap_index = 0; header->heap_index = 0;
header->heap = NULL;
} else if (resign_sooner(header, &oldheader)) { } else if (resign_sooner(header, &oldheader)) {
isc_heap_increased(qpdb->heap, header->heap_index); isc_heap_increased(HEADERNODE(header)->heap->heap,
header->heap_index);
} else if (resign_sooner(&oldheader, header)) { } else if (resign_sooner(&oldheader, header)) {
isc_heap_decreased(qpdb->heap, header->heap_index); isc_heap_decreased(HEADERNODE(header)->heap->heap,
header->heap_index);
} }
RWUNLOCK(&qpdb->lock, isc_rwlocktype_write); UNLOCK(get_heap_lock(header));
} else if (resign != 0) { } else if (resign != 0) {
DNS_SLABHEADER_SETATTR(header, DNS_SLABHEADERATTR_RESIGN); DNS_SLABHEADER_SETATTR(header, DNS_SLABHEADERATTR_RESIGN);
resigninsert(qpdb, header); resigninsert(header);
} }
NODE_UNLOCK(nlock, &nlocktype); NODE_UNLOCK(nlock, &nlocktype);
return ISC_R_SUCCESS; return ISC_R_SUCCESS;
@ -2433,24 +2495,25 @@ getsigningtime(dns_db_t *db, isc_stdtime_t *resign, dns_name_t *foundname,
REQUIRE(foundname != NULL); REQUIRE(foundname != NULL);
REQUIRE(typepair != NULL); REQUIRE(typepair != NULL);
RWLOCK(&qpdb->lock, isc_rwlocktype_read); LOCK(&qpdb->heap->lock);
header = isc_heap_element(qpdb->heap, 1); header = isc_heap_element(qpdb->heap->heap, 1);
if (header == NULL) { if (header == NULL) {
RWUNLOCK(&qpdb->lock, isc_rwlocktype_read); UNLOCK(&qpdb->heap->lock);
return ISC_R_NOTFOUND; return ISC_R_NOTFOUND;
} }
nlock = qpzone_get_lock(qpdb, HEADERNODE(header)); nlock = qpzone_get_lock(qpdb, HEADERNODE(header));
RWUNLOCK(&qpdb->lock, isc_rwlocktype_read); UNLOCK(&qpdb->heap->lock);
again: again:
NODE_RDLOCK(nlock, &nlocktype); NODE_RDLOCK(nlock, &nlocktype);
RWLOCK(&qpdb->lock, isc_rwlocktype_read); LOCK(&qpdb->heap->lock);
header = isc_heap_element(qpdb->heap, 1); header = isc_heap_element(qpdb->heap->heap, 1);
if (header != NULL && if (header != NULL &&
qpzone_get_lock(qpdb, HEADERNODE(header)) != nlock) qpzone_get_lock(qpdb, HEADERNODE(header)) != nlock)
{ {
RWUNLOCK(&qpdb->lock, isc_rwlocktype_read); UNLOCK(&qpdb->heap->lock);
NODE_UNLOCK(nlock, &nlocktype); NODE_UNLOCK(nlock, &nlocktype);
nlock = qpzone_get_lock(qpdb, HEADERNODE(header)); nlock = qpzone_get_lock(qpdb, HEADERNODE(header));
@ -2465,7 +2528,7 @@ again:
*typepair = header->type; *typepair = header->type;
result = ISC_R_SUCCESS; result = ISC_R_SUCCESS;
} }
RWUNLOCK(&qpdb->lock, isc_rwlocktype_read); UNLOCK(&qpdb->heap->lock);
NODE_UNLOCK(nlock, &nlocktype); NODE_UNLOCK(nlock, &nlocktype);
return result; return result;
@ -4038,13 +4101,13 @@ unlocknode(dns_db_t *db, dns_dbnode_t *dbnode, isc_rwlocktype_t type) {
static void static void
deletedata(dns_db_t *db ISC_ATTR_UNUSED, dns_dbnode_t *node ISC_ATTR_UNUSED, deletedata(dns_db_t *db ISC_ATTR_UNUSED, dns_dbnode_t *node ISC_ATTR_UNUSED,
void *data) { void *data) {
qpzonedb_t *qpdb = (qpzonedb_t *)db;
dns_slabheader_t *header = data; dns_slabheader_t *header = data;
if (header->heap != NULL && header->heap_index != 0) { if (header->heap_index != 0) {
RWLOCK(&qpdb->lock, isc_rwlocktype_write); LOCK(get_heap_lock(header));
isc_heap_delete(header->heap, header->heap_index); isc_heap_delete(HEADERNODE(header)->heap->heap,
RWUNLOCK(&qpdb->lock, isc_rwlocktype_write); header->heap_index);
UNLOCK(get_heap_lock(header));
} }
header->heap_index = 0; header->heap_index = 0;
} }
@ -4857,7 +4920,7 @@ qpzone_subtractrdataset(dns_db_t *db, dns_dbnode_t *dbnode,
newheader, DNS_SLABHEADERATTR_RESIGN); newheader, DNS_SLABHEADERATTR_RESIGN);
newheader->resign = header->resign; newheader->resign = header->resign;
newheader->resign_lsb = header->resign_lsb; newheader->resign_lsb = header->resign_lsb;
resigninsert(qpdb, newheader); resigninsert(newheader);
} }
/* /*
* We have to set the serial since the rdataslab * We have to set the serial since the rdataslab
@ -5393,6 +5456,7 @@ destroy_qpznode(qpznode_t *node) {
dns_slabheader_destroy(&current); dns_slabheader_destroy(&current);
} }
qpz_heap_unref(node->heap);
dns_name_free(&node->name, node->mctx); dns_name_free(&node->name, node->mctx);
isc_mem_putanddetach(&node->mctx, node, sizeof(qpznode_t)); isc_mem_putanddetach(&node->mctx, node, sizeof(qpznode_t));
} }
@ -5409,6 +5473,8 @@ ISC_REFCOUNT_STATIC_TRACE_IMPL(qpzonedb, qpzone_destroy);
ISC_REFCOUNT_STATIC_IMPL(qpzonedb, qpzone_destroy); ISC_REFCOUNT_STATIC_IMPL(qpzonedb, qpzone_destroy);
#endif #endif
ISC_REFCOUNT_STATIC_IMPL(qpz_heap, qpz_heap_destroy);
static void static void
qp_attach(void *uctx ISC_ATTR_UNUSED, void *pval, qp_attach(void *uctx ISC_ATTR_UNUSED, void *pval,
uint32_t ival ISC_ATTR_UNUSED) { uint32_t ival ISC_ATTR_UNUSED) {

2
tests/dns/qpzone_test.c
View File

@ -39,7 +39,7 @@
#pragma GCC diagnostic push #pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wshadow" #pragma GCC diagnostic ignored "-Wshadow"
#undef CHECK #undef CHECK
#define DEFAULT_BUCKETS_COUNT 1 /*%< Should be prime. */ #define DEFAULT_BUCKETS_COUNT 1
#include "qpzone.c" #include "qpzone.c"
#pragma GCC diagnostic pop #pragma GCC diagnostic pop