From: Mark Andrews Date: Tue, 2 Jun 2020 12:38:40 +1000 Subject: Remove INSIST from from new_reference RBTDB node can now appear on the deadnodes lists following the changes to decrement_reference in 176b23b6cd98e5b58f832902fdbe964ee5f762d0 to defer checking of node->down when the tree write lock is not held. The node should be unlinked instead. (cherry picked from commit b8c4efb10fc8ef1489120a8169fea42adf97025e) --- lib/dns/rbtdb.c | 238 +++++++++++++++++++++++++++++++++----------------------- 1 file changed, 142 insertions(+), 96 deletions(-) diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index 0861139..792c443 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -2069,11 +2069,16 @@ delete_node(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) { * Caller must be holding the node lock. */ static inline void -new_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) { +new_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, + isc_rwlocktype_t locktype) { unsigned int lockrefs, noderefs; isc_refcount_t *lockref; - INSIST(!ISC_LINK_LINKED(node, deadlink)); + if (locktype == isc_rwlocktype_write && ISC_LINK_LINKED(node, deadlink)) + { + ISC_LIST_UNLINK(rbtdb->deadnodes[node->locknum], node, + deadlink); + } dns_rbtnode_refincrement0(node, &noderefs); if (noderefs == 1) { /* this is the first reference to the node */ lockref = &rbtdb->node_locks[node->locknum].references; @@ -2119,7 +2124,7 @@ cleanup_dead_nodes(dns_rbtdb_t *rbtdb, int bucketnum) { prune_tree, node, sizeof(isc_event_t)); if (ev != NULL) { - new_reference(rbtdb, node); + new_reference(rbtdb, node, isc_rwlocktype_write); db = NULL; attach((dns_db_t *)rbtdb, &db); ev->ev_sender = db; @@ -2183,7 +2188,7 @@ reactivate_node(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, cleanup_dead_nodes(rbtdb, node->locknum); } - new_reference(rbtdb, node); + new_reference(rbtdb, node, locktype); NODE_WEAKUNLOCK(nodelock, locktype); NODE_STRONGUNLOCK(nodelock); @@ -2329,7 +2334,7 @@ decrement_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, prune_tree, node, sizeof(isc_event_t)); if (ev != NULL) { - new_reference(rbtdb, node); + new_reference(rbtdb, node, isc_rwlocktype_write); db = NULL; attach((dns_db_t *)rbtdb, &db); ev->ev_sender = db; @@ -2361,8 +2366,10 @@ decrement_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, } } else { INSIST(node->data == NULL); - INSIST(!ISC_LINK_LINKED(node, deadlink)); - ISC_LIST_APPEND(rbtdb->deadnodes[bucket], node, deadlink); + if (!ISC_LINK_LINKED(node, deadlink)) { + ISC_LIST_APPEND(rbtdb->deadnodes[bucket], node, + deadlink); + } } restore_locks: @@ -2427,17 +2434,16 @@ prune_tree(isc_task_t *task, isc_event_t *event) { /* * We need to gain a reference to the node before - * decrementing it in the next iteration. In addition, - * if the node is in the dead-nodes list, extract it - * from the list beforehand as we do in - * reactivate_node(). + * decrementing it in the next iteration. */ - if (ISC_LINK_LINKED(parent, deadlink)) + if (ISC_LINK_LINKED(parent, deadlink)) { ISC_LIST_UNLINK(rbtdb->deadnodes[locknum], parent, deadlink); - new_reference(rbtdb, parent); - } else + } + new_reference(rbtdb, parent, isc_rwlocktype_write); + } else { parent = NULL; + } node = parent; } while (node != NULL); @@ -3219,7 +3225,7 @@ zone_zonecut_callback(dns_rbtnode_t *node, dns_name_t *name, void *arg) { * We increment the reference count on node to ensure that * search->zonecut_rdataset will still be valid later. */ - new_reference(search->rbtdb, node); + new_reference(search->rbtdb, node, isc_rwlocktype_read); search->zonecut = node; search->zonecut_rdataset = found; search->need_cleanup = true; @@ -3270,11 +3276,10 @@ zone_zonecut_callback(dns_rbtnode_t *node, dns_name_t *name, void *arg) { } static inline void -bind_rdataset(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, - rdatasetheader_t *header, isc_stdtime_t now, - dns_rdataset_t *rdataset) -{ - unsigned char *raw; /* RDATASLAB */ +bind_rdataset(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, rdatasetheader_t *header, + isc_stdtime_t now, isc_rwlocktype_t locktype, + dns_rdataset_t *rdataset) { + unsigned char *raw; /* RDATASLAB */ /* * Caller must be holding the node reader lock. @@ -3287,7 +3292,7 @@ bind_rdataset(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, if (rdataset == NULL) return; - new_reference(rbtdb, node); + new_reference(rbtdb, node, locktype); INSIST(rdataset->methods == NULL); /* We must be disassociated. */ @@ -3382,11 +3387,13 @@ setup_delegation(rbtdb_search_t *search, dns_dbnode_t **nodep, NODE_LOCK(&(search->rbtdb->node_locks[node->locknum].lock), isc_rwlocktype_read); bind_rdataset(search->rbtdb, node, search->zonecut_rdataset, - search->now, rdataset); + search->now, isc_rwlocktype_read, rdataset); if (sigrdataset != NULL && search->zonecut_sigrdataset != NULL) + { bind_rdataset(search->rbtdb, node, - search->zonecut_sigrdataset, - search->now, sigrdataset); + search->zonecut_sigrdataset, search->now, + isc_rwlocktype_read, sigrdataset); + } NODE_UNLOCK(&(search->rbtdb->node_locks[node->locknum].lock), isc_rwlocktype_read); } @@ -4045,19 +4052,22 @@ find_closest_nsec(rbtdb_search_t *search, dns_dbnode_t **nodep, foundname, NULL); if (result == ISC_R_SUCCESS) { if (nodep != NULL) { - new_reference(search->rbtdb, - node); + new_reference( + search->rbtdb, node, + isc_rwlocktype_read); *nodep = node; } bind_rdataset(search->rbtdb, node, found, search->now, + isc_rwlocktype_read, rdataset); - if (foundsig != NULL) - bind_rdataset(search->rbtdb, - node, - foundsig, - search->now, - sigrdataset); + if (foundsig != NULL) { + bind_rdataset( + search->rbtdb, node, + foundsig, search->now, + isc_rwlocktype_read, + sigrdataset); + } } } else if (found == NULL && foundsig == NULL) { /* @@ -4331,7 +4341,8 @@ zone_find(dns_db_t *db, dns_name_t *name, dns_dbversion_t *version, * ensure that search->zonecut_rdataset will * still be valid later. */ - new_reference(search.rbtdb, node); + new_reference(search.rbtdb, node, + isc_rwlocktype_read); search.zonecut = node; search.zonecut_rdataset = header; search.zonecut_sigrdataset = NULL; @@ -4504,18 +4515,19 @@ zone_find(dns_db_t *db, dns_name_t *name, dns_dbversion_t *version, goto node_exit; } if (nodep != NULL) { - new_reference(search.rbtdb, node); + new_reference(search.rbtdb, node, isc_rwlocktype_read); *nodep = node; } if ((search.rbtversion->secure == dns_db_secure && !search.rbtversion->havensec3) || (search.options & DNS_DBFIND_FORCENSEC) != 0) { - bind_rdataset(search.rbtdb, node, nsecheader, - 0, rdataset); - if (nsecsig != NULL) - bind_rdataset(search.rbtdb, node, - nsecsig, 0, sigrdataset); + bind_rdataset(search.rbtdb, node, nsecheader, 0, + isc_rwlocktype_read, rdataset); + if (nsecsig != NULL) { + bind_rdataset(search.rbtdb, node, nsecsig, 0, + isc_rwlocktype_read, sigrdataset); + } } if (wild) foundname->attributes |= DNS_NAMEATTR_WILDCARD; @@ -4581,18 +4593,21 @@ zone_find(dns_db_t *db, dns_name_t *name, dns_dbversion_t *version, } if (nodep != NULL) { - if (!at_zonecut) - new_reference(search.rbtdb, node); - else + if (!at_zonecut) { + new_reference(search.rbtdb, node, isc_rwlocktype_read); + } else { search.need_cleanup = false; + } *nodep = node; } if (type != dns_rdatatype_any) { - bind_rdataset(search.rbtdb, node, found, 0, rdataset); - if (foundsig != NULL) + bind_rdataset(search.rbtdb, node, found, 0, isc_rwlocktype_read, + rdataset); + if (foundsig != NULL) { bind_rdataset(search.rbtdb, node, foundsig, 0, - sigrdataset); + isc_rwlocktype_read, sigrdataset); + } } if (wild) @@ -4762,8 +4777,7 @@ cache_zonecut_callback(dns_rbtnode_t *node, dns_name_t *name, void *arg) { * We increment the reference count on node to ensure that * search->zonecut_rdataset will still be valid later. */ - new_reference(search->rbtdb, node); - INSIST(!ISC_LINK_LINKED(node, deadlink)); + new_reference(search->rbtdb, node, locktype); search->zonecut = node; search->zonecut_rdataset = dname_header; search->zonecut_sigrdataset = sigdname_header; @@ -4869,14 +4883,16 @@ find_deepest_zonecut(rbtdb_search_t *search, dns_rbtnode_t *node, } result = DNS_R_DELEGATION; if (nodep != NULL) { - new_reference(search->rbtdb, node); + new_reference(search->rbtdb, node, locktype); *nodep = node; } bind_rdataset(search->rbtdb, node, found, search->now, - rdataset); - if (foundsig != NULL) + locktype, rdataset); + if (foundsig != NULL) { bind_rdataset(search->rbtdb, node, foundsig, - search->now, sigrdataset); + search->now, locktype, + sigrdataset); + } if (need_headerupdate(found, search->now) || (foundsig != NULL && need_headerupdate(foundsig, search->now))) { @@ -4968,14 +4984,16 @@ find_coveringnsec(rbtdb_search_t *search, dns_dbnode_t **nodep, if (found != NULL) { result = dns_name_concatenate(name, origin, foundname, NULL); - if (result != ISC_R_SUCCESS) + if (result != ISC_R_SUCCESS) { goto unlock_node; - bind_rdataset(search->rbtdb, node, found, - now, rdataset); - if (foundsig != NULL) + } + bind_rdataset(search->rbtdb, node, found, now, locktype, + rdataset); + if (foundsig != NULL) { bind_rdataset(search->rbtdb, node, foundsig, - now, sigrdataset); - new_reference(search->rbtdb, node); + now, locktype, sigrdataset); + } + new_reference(search->rbtdb, node, locktype); *nodep = node; result = DNS_R_COVERINGNSEC; } else if (!empty_node) { @@ -5230,19 +5248,21 @@ cache_find(dns_db_t *db, dns_name_t *name, dns_dbversion_t *version, */ if (nsheader != NULL) { if (nodep != NULL) { - new_reference(search.rbtdb, node); - INSIST(!ISC_LINK_LINKED(node, deadlink)); + new_reference(search.rbtdb, node, locktype); *nodep = node; } bind_rdataset(search.rbtdb, node, nsheader, search.now, - rdataset); - if (need_headerupdate(nsheader, search.now)) + locktype, rdataset); + if (need_headerupdate(nsheader, search.now)) { update = nsheader; + } if (nssig != NULL) { bind_rdataset(search.rbtdb, node, nssig, - search.now, sigrdataset); - if (need_headerupdate(nssig, search.now)) + search.now, locktype, + sigrdataset); + if (need_headerupdate(nssig, search.now)) { updatesig = nssig; + } } result = DNS_R_DELEGATION; goto node_exit; @@ -5260,8 +5280,7 @@ cache_find(dns_db_t *db, dns_name_t *name, dns_dbversion_t *version, */ if (nodep != NULL) { - new_reference(search.rbtdb, node); - INSIST(!ISC_LINK_LINKED(node, deadlink)); + new_reference(search.rbtdb, node, locktype); *nodep = node; } @@ -5290,16 +5309,19 @@ cache_find(dns_db_t *db, dns_name_t *name, dns_dbversion_t *version, } if (type != dns_rdatatype_any || result == DNS_R_NCACHENXDOMAIN || - result == DNS_R_NCACHENXRRSET) { - bind_rdataset(search.rbtdb, node, found, search.now, + result == DNS_R_NCACHENXRRSET) + { + bind_rdataset(search.rbtdb, node, found, search.now, locktype, rdataset); - if (need_headerupdate(found, search.now)) + if (need_headerupdate(found, search.now)) { update = found; + } if (!NEGATIVE(found) && foundsig != NULL) { bind_rdataset(search.rbtdb, node, foundsig, search.now, - sigrdataset); - if (need_headerupdate(foundsig, search.now)) + locktype, sigrdataset); + if (need_headerupdate(foundsig, search.now)) { updatesig = foundsig; + } } } @@ -5445,15 +5467,16 @@ cache_findzonecut(dns_db_t *db, dns_name_t *name, unsigned int options, } if (nodep != NULL) { - new_reference(search.rbtdb, node); - INSIST(!ISC_LINK_LINKED(node, deadlink)); + new_reference(search.rbtdb, node, locktype); *nodep = node; } - bind_rdataset(search.rbtdb, node, found, search.now, rdataset); - if (foundsig != NULL) + bind_rdataset(search.rbtdb, node, found, search.now, locktype, + rdataset); + if (foundsig != NULL) { bind_rdataset(search.rbtdb, node, foundsig, search.now, - sigrdataset); + locktype, sigrdataset); + } if (need_headerupdate(found, search.now) || (foundsig != NULL && need_headerupdate(foundsig, search.now))) { @@ -5804,10 +5827,12 @@ zone_findrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, } } if (found != NULL) { - bind_rdataset(rbtdb, rbtnode, found, now, rdataset); - if (foundsig != NULL) + bind_rdataset(rbtdb, rbtnode, found, now, isc_rwlocktype_read, + rdataset); + if (foundsig != NULL) { bind_rdataset(rbtdb, rbtnode, foundsig, now, - sigrdataset); + isc_rwlocktype_read, sigrdataset); + } } NODE_UNLOCK(&rbtdb->node_locks[rbtnode->locknum].lock, @@ -5892,10 +5917,11 @@ cache_findrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, } } if (found != NULL) { - bind_rdataset(rbtdb, rbtnode, found, now, rdataset); - if (!NEGATIVE(found) && foundsig != NULL) - bind_rdataset(rbtdb, rbtnode, foundsig, now, + bind_rdataset(rbtdb, rbtnode, found, now, locktype, rdataset); + if (!NEGATIVE(found) && foundsig != NULL) { + bind_rdataset(rbtdb, rbtnode, foundsig, now, locktype, sigrdataset); + } } NODE_UNLOCK(lock, locktype); @@ -6061,6 +6087,9 @@ resign_insert(dns_rbtdb_t *rbtdb, int idx, rdatasetheader_t *newheader) { return (result); } +/* + * node write lock must be held. + */ static void resign_delete(dns_rbtdb_t *rbtdb, rbtdb_version_t *version, rdatasetheader_t *header) @@ -6073,7 +6102,8 @@ resign_delete(dns_rbtdb_t *rbtdb, rbtdb_version_t *version, header->heap_index); header->heap_index = 0; if (version != NULL) { - new_reference(rbtdb, header->node); + new_reference(rbtdb, header->node, + isc_rwlocktype_write); ISC_LIST_APPEND(version->resigned_list, header, link); } } @@ -6095,6 +6125,9 @@ update_recordsandbytes(bool add, rbtdb_version_t *rbtversion, } } +/* + * write lock on rbtnode must be held. + */ static isc_result_t add32(dns_rbtdb_t *rbtdb, dns_rbtnode_t *rbtnode, rbtdb_version_t *rbtversion, rdatasetheader_t *newheader, unsigned int options, bool loading, @@ -6218,10 +6251,13 @@ add32(dns_rbtdb_t *rbtdb, dns_rbtnode_t *rbtnode, rbtdb_version_t *rbtversion, free_rdataset(rbtdb, rbtdb->common.mctx, newheader); - if (addedrdataset != NULL) - bind_rdataset(rbtdb, rbtnode, - topheader, now, - addedrdataset); + if (addedrdataset != NULL) { + bind_rdataset( + rbtdb, rbtnode, + topheader, now, + isc_rwlocktype_write, + addedrdataset); + } return (DNS_R_UNCHANGED); } /* @@ -6275,6 +6311,7 @@ add32(dns_rbtdb_t *rbtdb, dns_rbtnode_t *rbtnode, rbtdb_version_t *rbtversion, free_rdataset(rbtdb, rbtdb->common.mctx, newheader); if (addedrdataset != NULL) bind_rdataset(rbtdb, rbtnode, header, now, + isc_rwlocktype_write, addedrdataset); return (DNS_R_UNCHANGED); } @@ -6374,6 +6411,7 @@ add32(dns_rbtdb_t *rbtdb, dns_rbtnode_t *rbtnode, rbtdb_version_t *rbtversion, free_rdataset(rbtdb, rbtdb->common.mctx, newheader); if (addedrdataset != NULL) bind_rdataset(rbtdb, rbtnode, header, now, + isc_rwlocktype_write, addedrdataset); return (ISC_R_SUCCESS); } @@ -6420,6 +6458,7 @@ add32(dns_rbtdb_t *rbtdb, dns_rbtnode_t *rbtnode, rbtdb_version_t *rbtversion, free_rdataset(rbtdb, rbtdb->common.mctx, newheader); if (addedrdataset != NULL) bind_rdataset(rbtdb, rbtnode, header, now, + isc_rwlocktype_write, addedrdataset); return (ISC_R_SUCCESS); } @@ -6617,8 +6656,10 @@ add32(dns_rbtdb_t *rbtdb, dns_rbtnode_t *rbtnode, rbtdb_version_t *rbtversion, cname_and_other_data(rbtnode, rbtversion->serial)) return (DNS_R_CNAMEANDOTHER); - if (addedrdataset != NULL) - bind_rdataset(rbtdb, rbtnode, newheader, now, addedrdataset); + if (addedrdataset != NULL) { + bind_rdataset(rbtdb, rbtnode, newheader, now, + isc_rwlocktype_write, addedrdataset); + } return (ISC_R_SUCCESS); } @@ -7141,12 +7182,17 @@ subtractrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, result = DNS_R_UNCHANGED; } - if (result == ISC_R_SUCCESS && newrdataset != NULL) - bind_rdataset(rbtdb, rbtnode, newheader, 0, newrdataset); + if (result == ISC_R_SUCCESS && newrdataset != NULL) { + bind_rdataset(rbtdb, rbtnode, newheader, 0, + isc_rwlocktype_write, newrdataset); + } if (result == DNS_R_NXRRSET && newrdataset != NULL && (options & DNS_DBSUB_WANTOLD) != 0) - bind_rdataset(rbtdb, rbtnode, header, 0, newrdataset); + { + bind_rdataset(rbtdb, rbtnode, header, 0, isc_rwlocktype_write, + newrdataset); + } unlock: NODE_UNLOCK(&rbtdb->node_locks[rbtnode->locknum].lock, @@ -8057,7 +8103,7 @@ getoriginnode(dns_db_t *db, dns_dbnode_t **nodep) { onode = (dns_rbtnode_t *)rbtdb->origin_node; if (onode != NULL) { NODE_STRONGLOCK(&rbtdb->node_locks[onode->locknum].lock); - new_reference(rbtdb, onode); + new_reference(rbtdb, onode, isc_rwlocktype_none); NODE_STRONGUNLOCK(&rbtdb->node_locks[onode->locknum].lock); *nodep = rbtdb->origin_node; @@ -8222,7 +8268,7 @@ getsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, if (header == NULL) goto unlock; - bind_rdataset(rbtdb, header->node, header, 0, rdataset); + bind_rdataset(rbtdb, header->node, header, 0, isc_rwlocktype_read, rdataset); if (foundname != NULL) dns_rbt_fullnamefromnode(header->node, foundname); @@ -9214,7 +9260,7 @@ rdatasetiter_current(dns_rdatasetiter_t *iterator, dns_rdataset_t *rdataset) { isc_rwlocktype_read); bind_rdataset(rbtdb, rbtnode, header, rbtiterator->common.now, - rdataset); + isc_rwlocktype_read, rdataset); NODE_UNLOCK(&rbtdb->node_locks[rbtnode->locknum].lock, isc_rwlocktype_read); @@ -9650,7 +9696,7 @@ dbiterator_current(dns_dbiterator_t *iterator, dns_dbnode_t **nodep, result = ISC_R_SUCCESS; NODE_STRONGLOCK(&rbtdb->node_locks[node->locknum].lock); - new_reference(rbtdb, node); + new_reference(rbtdb, node, isc_rwlocktype_none); NODE_STRONGUNLOCK(&rbtdb->node_locks[node->locknum].lock); *nodep = rbtdbiter->node; @@ -10405,7 +10451,7 @@ expire_header(dns_rbtdb_t *rbtdb, rdatasetheader_t *header, * We first need to gain a new reference to the node to meet a * requirement of decrement_reference(). */ - new_reference(rbtdb, header->node); + new_reference(rbtdb, header->node, isc_rwlocktype_write); decrement_reference(rbtdb, header->node, 0, isc_rwlocktype_write, tree_locked ? isc_rwlocktype_write :