From 176b23b6cd98e5b58f832902fdbe964ee5f762d0 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Wed, 4 Dec 2019 21:41:04 +1100 Subject: [PATCH 1/2] Testing node->down requires the tree lock to be held. In decrement_reference only test node->down if the tree lock is held. As node->down is not always tested in decrement_reference we need to test that it is non NULL in cleanup_dead_nodes prior to removing the node from the rbt tree. Additionally it is not always possible to aquire the node lock and reactivate a node when adding parent nodes. Reactivate such nodes in cleanup_dead_nodes if required. --- lib/dns/rbtdb.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index 7ce3052286..e0914f0b95 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -1844,6 +1844,10 @@ cleanup_dead_nodes(dns_rbtdb_t *rbtdb, int bucketnum) { /* * Since we're holding a tree write lock, it should be * impossible for this node to be referenced by others. + * + * decrement_reference may not have tested node->down, as + * the tree_lock was not held, before adding the node to + * deadnodes so we test it here. */ INSIST(isc_refcount_current(&node->references) == 0 && node->data == NULL); @@ -1864,8 +1868,19 @@ cleanup_dead_nodes(dns_rbtdb_t *rbtdb, int bucketnum) { attach((dns_db_t *)rbtdb, &db); ev->ev_sender = db; isc_task_send(rbtdb->task, &ev); - } else { + } else if (node->down == NULL && node->data == NULL) { + /* + * Not a interior node and not needing to be + * reactivated. + */ delete_node(rbtdb, node); + } else if (node->data == NULL) { + /* + * A interior node without data. Leave linked to + * to be cleaned up when node->down becomes NULL. + */ + ISC_LIST_APPEND(rbtdb->deadnodes[bucketnum], + node, deadlink); } node = ISC_LIST_HEAD(rbtdb->deadnodes[bucketnum]); count--; @@ -1945,6 +1960,7 @@ decrement_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, { isc_result_t result; bool write_locked; + bool locked = tlock != isc_rwlocktype_none; rbtdb_nodelock_t *nodelock; int bucket = node->locknum; bool no_reference = true; @@ -1952,12 +1968,12 @@ decrement_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, nodelock = &rbtdb->node_locks[bucket]; -#define KEEP_NODE(n, r) \ - ((n)->data != NULL || (n)->down != NULL || \ +#define KEEP_NODE(n, r, l) \ + ((n)->data != NULL || ((l) && (n)->down != NULL) || \ (n) == (r)->origin_node || (n) == (r)->nsec3_origin_node) /* Handle easy and typical case first. */ - if (!node->dirty && KEEP_NODE(node, rbtdb)) { + if (!node->dirty && KEEP_NODE(node, rbtdb, locked)) { if (isc_refcount_decrement(&node->references) == 1) { refs = isc_refcount_decrement(&nodelock->references); INSIST(refs > 0); @@ -2024,8 +2040,9 @@ decrement_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, refs = isc_refcount_decrement(&nodelock->references); INSIST(refs > 0); - if (KEEP_NODE(node, rbtdb)) + if (KEEP_NODE(node, rbtdb, locked || write_locked)) { goto restore_locks; + } #undef KEEP_NODE From c6efc0e50fd806f48c243baed69b70a7e6473347 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Fri, 6 Dec 2019 10:08:52 +1100 Subject: [PATCH 2/2] Add is_leaf and send_to_prune_tree. Add is_leaf and send_to_prune_tree to make the logic easier to understand in cleanup_dead_nodes and decrement_reference. --- lib/dns/rbtdb.c | 60 ++++++++++++++++++++++++------------------------- 1 file changed, 29 insertions(+), 31 deletions(-) diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index e0914f0b95..d181427013 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -1824,6 +1824,31 @@ new_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) { } } +/*% + * The tree lock must be held for the result to be valid. + */ +static inline bool +is_leaf(dns_rbtnode_t *node) { + return (node->parent != NULL && node->parent->down == node && + node->left == NULL && node->right == NULL); +} + +static inline void +send_to_prune_tree(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) { + isc_event_t *ev; + dns_db_t *db; + + ev = isc_event_allocate(rbtdb->common.mctx, NULL, + DNS_EVENT_RBTPRUNE, + prune_tree, node, + sizeof(isc_event_t)); + new_reference(rbtdb, node); + db = NULL; + attach((dns_db_t *)rbtdb, &db); + ev->ev_sender = db; + isc_task_send(rbtdb->task, &ev); +} + /*% * Clean up dead nodes. These are nodes which have no references, and * have no data. They are dead but we could not or chose not to delete @@ -1852,22 +1877,8 @@ cleanup_dead_nodes(dns_rbtdb_t *rbtdb, int bucketnum) { INSIST(isc_refcount_current(&node->references) == 0 && node->data == NULL); - if (node->parent != NULL && - node->parent->down == node && node->left == NULL && - node->right == NULL && rbtdb->task != NULL) - { - isc_event_t *ev; - dns_db_t *db; - - ev = isc_event_allocate(rbtdb->common.mctx, NULL, - DNS_EVENT_RBTPRUNE, - prune_tree, node, - sizeof(isc_event_t)); - new_reference(rbtdb, node); - db = NULL; - attach((dns_db_t *)rbtdb, &db); - ev->ev_sender = db; - isc_task_send(rbtdb->task, &ev); + if (is_leaf(node) && rbtdb->task != NULL) { + send_to_prune_tree(rbtdb, node); } else if (node->down == NULL && node->data == NULL) { /* * Not a interior node and not needing to be @@ -2067,21 +2078,8 @@ decrement_reference(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, * it's their responsibility to purge stale leaves (e.g. by * periodic walk-through). */ - if (!pruning && node->parent != NULL && - node->parent->down == node && node->left == NULL && - node->right == NULL && rbtdb->task != NULL) { - isc_event_t *ev; - dns_db_t *db; - - ev = isc_event_allocate(rbtdb->common.mctx, NULL, - DNS_EVENT_RBTPRUNE, - prune_tree, node, - sizeof(isc_event_t)); - new_reference(rbtdb, node); - db = NULL; - attach((dns_db_t *)rbtdb, &db); - ev->ev_sender = db; - isc_task_send(rbtdb->task, &ev); + if (!pruning && is_leaf(node) && rbtdb->task != NULL) { + send_to_prune_tree(rbtdb, node); no_reference = false; } else { delete_node(rbtdb, node);