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 dcacb1dcef16d2677f9872f4ce3d5d4038e83349..ff3381c91f5802ff2fb86c63d01d407e1cf0a550 100644 (file)
--- 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 de9f1a79a9bae2fcf8b38e8741aa9f68f424cb73..dc2b3d073d1d9c4fcc84fa46fe106c53ea64ede5 100644 (file)
--- 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,