From: Ondřej Surý Date: Mon, 24 Feb 2025 14:55:18 +0000 (+0100) Subject: Acquire the database reference before possibly last node release X-Git-Tag: ondrej/lock-free-qpzone-reads-v1~35^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d1ef6a93c112137ab0682afb9a3240d47285d408;p=thirdparty%2Fbind9.git Acquire the database reference before possibly last node release Acquire the database refernce in the detachnode() to prevent the last reference to be release while the NODE_LOCK being locked. The NODE_LOCK is locked/unlocked inside the RCU critical section, thus it is most probably this should not pose a problem as the database uses call_rcu memory reclamation, but this it is still safer to acquire the reference before releasing the node. --- diff --git a/lib/dns/qpcache.c b/lib/dns/qpcache.c index dcacb1dcef1..ff3381c91f5 100644 --- a/lib/dns/qpcache.c +++ b/lib/dns/qpcache.c @@ -307,7 +307,7 @@ struct qpcache { #ifdef DNS_DB_NODETRACE #define qpcache_ref(ptr) qpcache__ref(ptr, __func__, __FILE__, __LINE__) -#define qpcache_unref(ptr) qpcache_unref(ptr, __func__, __FILE__, __LINE__) +#define qpcache_unref(ptr) qpcache__unref(ptr, __func__, __FILE__, __LINE__) #define qpcache_attach(ptr, ptrp) \ qpcache__attach(ptr, ptrp, __func__, __FILE__, __LINE__) #define qpcache_detach(ptrp) qpcache__detach(ptrp, __func__, __FILE__, __LINE__) @@ -797,6 +797,10 @@ qpcnode_release(qpcache_t *qpdb, qpcnode_t *node, isc_rwlocktype_t *nlocktypep, * acquired a reference in the meantime, so we increment * erefs (but NOT references!), upgrade the node lock, * decrement erefs again, and see if it's still zero. + * + * We can't really assume anything about the result code of + * erefs_increment. If another thread acquires reference it + * will be larger than 0, if it doesn't it is going to be 0. */ isc_rwlock_t *nlock = &qpdb->buckets[node->locknum].lock; qpcnode_erefs_increment(qpdb, node, *nlocktypep, @@ -2511,7 +2515,8 @@ cleanup_deadnodes(void *arg) { RUNTIME_CHECK(isc_queue_splice(&deadnodes, &qpdb->buckets[locknum].deadnodes)); isc_queue_for_each_entry_safe(&deadnodes, qpnode, qpnext, deadlink) { - qpcnode_release(qpdb, qpnode, &nlocktype, &tlocktype, false); + qpcnode_release(qpdb, qpnode, &nlocktype, &tlocktype, + false DNS__DB_FILELINE); } NODE_UNLOCK(nlock, &nlocktype); @@ -2624,16 +2629,19 @@ qpcache_detachnode(dns_db_t *db, dns_dbnode_t **nodep DNS__DB_FLARG) { nlock = &qpdb->buckets[node->locknum].lock; /* - * We can't destroy qpcache while holding a nodelock, so - * we need to reference it before acquiring the lock - * and release it afterward. + * We can't destroy qpcache while holding a nodelock, so we need to + * reference it before acquiring the lock and release it afterward. + * Additionally, we must ensure that we don't destroy the database while + * the NODE_LOCK is locked. */ qpcache_ref(qpdb); + rcu_read_lock(); NODE_RDLOCK(nlock, &nlocktype); qpcnode_release(qpdb, node, &nlocktype, &tlocktype, true DNS__DB_FLARG_PASS); NODE_UNLOCK(nlock, &nlocktype); + rcu_read_unlock(); qpcache_detach(&qpdb); } diff --git a/lib/dns/qpzone.c b/lib/dns/qpzone.c index de9f1a79a9b..dc2b3d073d1 100644 --- a/lib/dns/qpzone.c +++ b/lib/dns/qpzone.c @@ -185,7 +185,7 @@ struct qpznode { void *data; }; -typedef struct qpcache_bucket { +typedef struct qpzone_bucket { /* Per-bucket lock. */ isc_rwlock_t lock; @@ -245,7 +245,7 @@ struct qpzonedb { #ifdef DNS_DB_NODETRACE #define qpzonedb_ref(ptr) qpzonedb__ref(ptr, __func__, __FILE__, __LINE__) -#define qpzonedb_unref(ptr) qpzonedb_unref(ptr, __func__, __FILE__, __LINE__) +#define qpzonedb_unref(ptr) qpzonedb__unref(ptr, __func__, __FILE__, __LINE__) #define qpzonedb_attach(ptr, ptrp) \ qpzonedb__attach(ptr, ptrp, __func__, __FILE__, __LINE__) #define qpzonedb_detach(ptrp) \ @@ -940,6 +940,10 @@ qpznode_release(qpzonedb_t *qpdb, qpznode_t *node, uint32_t least_serial, * acquired a reference in the meantime, so we increment * erefs (but NOT references!), upgrade the node lock, * decrement erefs again, and see if it's still zero. + * + * We can't really assume anything about the result code of + * erefs_increment. If another thread acquires reference it + * will be larger than 0, if it doesn't it is going to be 0. */ isc_rwlock_t *nlock = &qpdb->buckets[node->locknum].lock; qpznode_erefs_increment(qpdb, node DNS__DB_FLARG_PASS); @@ -3977,8 +3981,8 @@ qpzone_allrdatasets(dns_db_t *db, dns_dbnode_t *dbnode, } static void -attachnode(dns_db_t *db, dns_dbnode_t *source, - dns_dbnode_t **targetp DNS__DB_FLARG) { +qpzone_attachnode(dns_db_t *db, dns_dbnode_t *source, + dns_dbnode_t **targetp DNS__DB_FLARG) { qpzonedb_t *qpdb = (qpzonedb_t *)db; qpznode_t *node = (qpznode_t *)source; @@ -3991,7 +3995,7 @@ attachnode(dns_db_t *db, dns_dbnode_t *source, } static void -detachnode(dns_db_t *db, dns_dbnode_t **nodep DNS__DB_FLARG) { +qpzone_detachnode(dns_db_t *db, dns_dbnode_t **nodep DNS__DB_FLARG) { qpzonedb_t *qpdb = (qpzonedb_t *)db; qpznode_t *node = NULL; isc_rwlocktype_t nlocktype = isc_rwlocktype_none; @@ -4005,15 +4009,21 @@ detachnode(dns_db_t *db, dns_dbnode_t **nodep DNS__DB_FLARG) { nlock = &qpdb->buckets[node->locknum].lock; /* - * qpzone_destroy() uses call_rcu() API to destroy the node locks, - * so it is safe to call it in the middle of NODE_LOCK. + * qpzone_destroy() uses call_rcu() API to destroy the node locks, so it + * is safe to call it in the middle of NODE_LOCK, but we need to acquire + * the database reference to prevent destroying the database while the + * NODE_LOCK is locked. */ + qpzonedb_ref(qpdb); + rcu_read_lock(); NODE_RDLOCK(nlock, &nlocktype); qpznode_release(qpdb, node, 0, &nlocktype DNS__DB_FLARG_PASS); NODE_UNLOCK(nlock, &nlocktype); rcu_read_unlock(); + + qpzonedb_unref(qpdb); } static unsigned int @@ -5405,8 +5415,8 @@ static dns_dbmethods_t qpdb_zonemethods = { .closeversion = closeversion, .findnode = qpzone_findnode, .find = qpzone_find, - .attachnode = attachnode, - .detachnode = detachnode, + .attachnode = qpzone_attachnode, + .detachnode = qpzone_detachnode, .createiterator = qpzone_createiterator, .findrdataset = qpzone_findrdataset, .allrdatasets = qpzone_allrdatasets,