From 5b57f580ce54201c6105300ea6636677531163f9 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Wed, 15 Feb 2012 02:00:47 +0000 Subject: [PATCH] 3284. [bug] Address race conditions with the handling of rbtnode.deadlink. [RT #27738] --- CHANGES | 3 ++ lib/dns/rbtdb.c | 91 +++++++++++++++++++++++++------------------------ 2 files changed, 49 insertions(+), 45 deletions(-) diff --git a/CHANGES b/CHANGES index a12008b661..2f237e5337 100644 --- 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 ca4d727669..b043fee404 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -15,7 +15,7 @@ * PERFORMANCE OF THIS SOFTWARE. */ -/* $Id: rbtdb.c,v 1.328 2012/02/09 23:47:18 tbox Exp $ */ +/* $Id: rbtdb.c,v 1.329 2012/02/15 02:00:47 marka Exp $ */ /*! \file */ @@ -1608,8 +1608,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) { @@ -1640,31 +1639,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); } /* @@ -1688,7 +1696,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]; @@ -1708,6 +1716,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) { @@ -1717,7 +1726,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 { @@ -1735,19 +1744,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 @@ -1771,13 +1767,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. */ /* @@ -1852,13 +1853,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);