3284. [bug] Address race conditions with the handling of

                        rbtnode.deadlink. [RT #27738]

diff --git a/CHANGES b/CHANGES
index 9d4af497b8afe6d7563f5335adf5617b50d9ddbd..0dd2e9205d08e8447b26fd593ce2b5a78b0b649a 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -1,3 +1,6 @@
+3284.  [bug]           Address race conditions with the handling of
+                       rbtnode.deadlink. [RT #27738]
+
 3283.  [bug]           Raw zones with with more than 512 records in a RRset
                        failed to load. [RT #27863]
 

diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c
index b0dd14c1db5818f99452fb355aabb6089a9e4c41..c7ba894e3bb52e7fae312d7811130afa24b16602 100644 (file)
--- a/lib/dns/rbtdb.c
+++ b/lib/dns/rbtdb.c
@@ -15,7 +15,7 @@
  * PERFORMANCE OF THIS SOFTWARE.
  */
 
-/* $Id: rbtdb.c,v 1.292.8.31 2012/02/09 23:45:56 tbox Exp $ */
+/* $Id: rbtdb.c,v 1.292.8.32 2012/02/15 02:03:39 marka Exp $ */
 
 /*! \file */
 
@@ -1590,8 +1590,7 @@ cleanup_dead_nodes(dns_rbtdb_t *rbtdb, int bucketnum) {
 }
 
 /*
- * Caller must be holding the node lock if its reference must be protected
- * by the lock.
+ * Caller must be holding the node lock.
  */
 static inline void
 new_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) {
@@ -1622,31 +1621,40 @@ static inline void
 reactivate_node(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node,
                isc_rwlocktype_t treelocktype)
 {
-       isc_boolean_t need_relock = ISC_FALSE;
+       isc_rwlocktype_t locktype = isc_rwlocktype_read;
+       nodelock_t *nodelock = &rbtdb->node_locks[node->locknum].lock;
+       isc_boolean_t maybe_cleanup = ISC_FALSE;
 
-       NODE_STRONGLOCK(&rbtdb->node_locks[node->locknum].lock);
-       NODE_WEAKLOCK(&rbtdb->node_locks[node->locknum].lock,
-                     isc_rwlocktype_read);
-       if (ISC_LINK_LINKED(node, deadlink))
-               need_relock = ISC_TRUE;
-       else if (!ISC_LIST_EMPTY(rbtdb->deadnodes[node->locknum]) &&
-                treelocktype == isc_rwlocktype_write)
-               need_relock = ISC_TRUE;
-       NODE_WEAKUNLOCK(&rbtdb->node_locks[node->locknum].lock,
-                       isc_rwlocktype_read);
-       if (need_relock) {
-               NODE_WEAKLOCK(&rbtdb->node_locks[node->locknum].lock,
-                             isc_rwlocktype_write);
+       NODE_STRONGLOCK(nodelock);
+       NODE_WEAKLOCK(nodelock, locktype);
+
+       /*
+        * Check if we can possibly cleanup the dead node.  If so, upgrade
+        * the node lock below to perform the cleanup.
+        */
+       if (!ISC_LIST_EMPTY(rbtdb->deadnodes[node->locknum]) &&
+           treelocktype == isc_rwlocktype_write) {
+               maybe_cleanup = ISC_TRUE;
+       }
+
+       if (ISC_LINK_LINKED(node, deadlink) || maybe_cleanup) {
+               /*
+                * Upgrade the lock and test if we still need to unlink.
+                */
+               NODE_WEAKUNLOCK(nodelock, locktype);
+               locktype = isc_rwlocktype_write;
+               NODE_WEAKLOCK(nodelock, locktype);
                if (ISC_LINK_LINKED(node, deadlink))
                        ISC_LIST_UNLINK(rbtdb->deadnodes[node->locknum],
                                        node, deadlink);
-               if (treelocktype == isc_rwlocktype_write)
+               if (maybe_cleanup)
                        cleanup_dead_nodes(rbtdb, node->locknum);
-               NODE_WEAKUNLOCK(&rbtdb->node_locks[node->locknum].lock,
-                               isc_rwlocktype_write);
        }
+
        new_reference(rbtdb, node);
-       NODE_STRONGUNLOCK(&rbtdb->node_locks[node->locknum].lock);
+
+       NODE_WEAKUNLOCK(nodelock, locktype);
+       NODE_STRONGUNLOCK(nodelock);
 }
 
 /*
@@ -1670,7 +1678,7 @@ decrement_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node,
        rbtdb_nodelock_t *nodelock;
        unsigned int refs, nrefs;
        int bucket = node->locknum;
-       isc_boolean_t no_reference;
+       isc_boolean_t no_reference = ISC_TRUE;
 
        nodelock = &rbtdb->node_locks[bucket];
 
@@ -1690,6 +1698,7 @@ decrement_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node,
                NODE_WEAKUNLOCK(&nodelock->lock, isc_rwlocktype_read);
                NODE_WEAKLOCK(&nodelock->lock, isc_rwlocktype_write);
        }
+
        dns_rbtnode_refdecrement(node, &nrefs);
        INSIST((int)nrefs >= 0);
        if (nrefs > 0) {
@@ -1699,7 +1708,7 @@ decrement_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node,
                return (ISC_FALSE);
        }
 
-       if (node->dirty && dns_rbtnode_refcurrent(node) == 0) {
+       if (node->dirty) {
                if (IS_CACHE(rbtdb))
                        clean_cache_node(rbtdb, node);
                else {
@@ -1717,19 +1726,6 @@ decrement_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node,
                }
        }
 
-       isc_refcount_decrement(&nodelock->references, &refs);
-       INSIST((int)refs >= 0);
-
-       /*
-        * XXXDCL should this only be done for cache zones?
-        */
-       if (node->data != NULL || node->down != NULL) {
-               /* Restore the lock? */
-               if (nlock == isc_rwlocktype_read)
-                       NODE_WEAKDOWNGRADE(&nodelock->lock);
-               return (ISC_TRUE);
-       }
-
        /*
         * Attempt to switch to a write lock on the tree.  If this fails,
         * we will add this node to a linked list of nodes in this locking
@@ -1753,13 +1749,18 @@ decrement_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node,
        } else
                write_locked = ISC_TRUE;
 
-       no_reference = ISC_TRUE;
-       if (write_locked && dns_rbtnode_refcurrent(node) == 0) {
+       isc_refcount_decrement(&nodelock->references, &refs);
+       INSIST((int)refs >= 0);
+
+       /*
+        * XXXDCL should this only be done for cache zones?
+        */
+       if (node->data != NULL || node->down != NULL)
+               goto restore_locks;
+
+       if (write_locked) {
                /*
-                * We can now delete the node if the reference counter is
-                * zero.  This should be typically the case, but a different
-                * thread may still gain a (new) reference just before the
-                * current thread locks the tree (e.g., in findnode()).
+                * We can now delete the node.
                 */
 
                /*
@@ -1834,13 +1835,13 @@ decrement_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node,
 
                        delete_node(rbtdb, node);
                }
-       } else if (dns_rbtnode_refcurrent(node) == 0) {
+       } else {
                INSIST(node->data == NULL);
                INSIST(!ISC_LINK_LINKED(node, deadlink));
                ISC_LIST_APPEND(rbtdb->deadnodes[bucket], node, deadlink);
-       } else
-               no_reference = ISC_FALSE;
+       }
 
+ restore_locks:
        /* Restore the lock? */
        if (nlock == isc_rwlocktype_read)
                NODE_WEAKDOWNGRADE(&nodelock->lock);