Update locking management for iter_fwd and iter_hints methods. (#1054)

fast reload, move most of the locking management to iter_fwd and
iter_hints methods. The caller still has the ability to handle its
own locking, if desired, for atomic operations on sets of different
structs.

Co-authored-by: Wouter Wijngaards <wcawijngaards@users.noreply.github.com>

Author: gthess

daemon/cachedump.c

@@ -839,6 +839,7 @@ int print_deleg_lookup(RES* ssl, struct worker* worker, uint8_t* nm,
 	char b[260];
 	struct query_info qinfo;
 	struct iter_hints_stub* stub;
+	int nolock = 0;
 	regional_free_all(region);
 	qinfo.qname = nm;
 	qinfo.qname_len = nmlen;
@@ -851,8 +852,7 @@ int print_deleg_lookup(RES* ssl, struct worker* worker, uint8_t* nm,
 		"of %s\n", b)) 
 		return 0;
 
-	lock_rw_rdlock(&worker->env.fwds->lock);
-	dp = forwards_lookup(worker->env.fwds, nm, qinfo.qclass);
+	dp = forwards_lookup(worker->env.fwds, nm, qinfo.qclass, nolock);
 	if(dp) {
 		if(!ssl_printf(ssl, "forwarding request:\n")) {
 			lock_rw_unlock(&worker->env.fwds->lock);
@@ -863,7 +863,6 @@ int print_deleg_lookup(RES* ssl, struct worker* worker, uint8_t* nm,
 		lock_rw_unlock(&worker->env.fwds->lock);
 		return 1;
 	}
-	lock_rw_unlock(&worker->env.fwds->lock);
 
 	while(1) {
 		dp = dns_cache_find_delegation(&worker->env, nm, nmlen, 
@@ -898,9 +897,8 @@ int print_deleg_lookup(RES* ssl, struct worker* worker, uint8_t* nm,
 				continue;
 			}
 		}
-		lock_rw_rdlock(&worker->env.hints->lock);
 		stub = hints_lookup_stub(worker->env.hints, nm, qinfo.qclass,
-			dp);
+			dp, nolock);
 		if(stub) {
 			if(stub->noprime) {
 				if(!ssl_printf(ssl, "The noprime stub servers "
@@ -919,7 +917,6 @@ int print_deleg_lookup(RES* ssl, struct worker* worker, uint8_t* nm,
 			print_dp_details(ssl, worker, stub->dp);
 			lock_rw_unlock(&worker->env.hints->lock);
 		} else {
-			lock_rw_unlock(&worker->env.hints->lock);
 			print_dp_main(ssl, dp, msg);
 			print_dp_details(ssl, worker, dp);
 		}

daemon/remote.c

@@ -1992,10 +1992,9 @@ static int
 print_root_fwds(RES* ssl, struct iter_forwards* fwds, uint8_t* root)
 {
 	struct delegpt* dp;
-	lock_rw_rdlock(&fwds->lock);
-	dp = forwards_lookup(fwds, root, LDNS_RR_CLASS_IN);
+	int nolock = 0;
+	dp = forwards_lookup(fwds, root, LDNS_RR_CLASS_IN, nolock);
 	if(!dp) {
-		lock_rw_unlock(&fwds->lock);
 		return ssl_printf(ssl, "off (using root hints)\n");
 	}
 	/* if dp is returned it must be the root */
@@ -2077,6 +2076,7 @@ do_forward(RES* ssl, struct worker* worker, char* args)
 {
 	struct iter_forwards* fwd = worker->env.fwds;
 	uint8_t* root = (uint8_t*)"\000";
+	int nolock = 0;
 	if(!fwd) {
 		(void)ssl_printf(ssl, "error: structure not allocated\n");
 		return;
@@ -2090,20 +2090,15 @@ do_forward(RES* ssl, struct worker* worker, char* args)
 	/* delete all the existing queries first */
 	mesh_delete_all(worker->env.mesh);
 	if(strcmp(args, "off") == 0) {
-		lock_rw_wrlock(&fwd->lock);
-		forwards_delete_zone(fwd, LDNS_RR_CLASS_IN, root);
-		lock_rw_unlock(&fwd->lock);
+		forwards_delete_zone(fwd, LDNS_RR_CLASS_IN, root, nolock);
 	} else {
 		struct delegpt* dp;
 		if(!(dp = parse_delegpt(ssl, args, root)))
 			return;
-		lock_rw_wrlock(&fwd->lock);
-		if(!forwards_add_zone(fwd, LDNS_RR_CLASS_IN, dp)) {
-			lock_rw_unlock(&fwd->lock);
+		if(!forwards_add_zone(fwd, LDNS_RR_CLASS_IN, dp, nolock)) {
 			(void)ssl_printf(ssl, "error out of memory\n");
 			return;
 		}
-		lock_rw_unlock(&fwd->lock);
 	}
 	send_ok(ssl);
 }
@@ -2162,10 +2157,12 @@ do_forward_add(RES* ssl, struct worker* worker, char* args)
 	int insecure = 0, tls = 0;
 	uint8_t* nm = NULL;
 	struct delegpt* dp = NULL;
+	int nolock = 1;
 	if(!parse_fs_args(ssl, args, &nm, &dp, &insecure, NULL, &tls))
 		return;
 	if(tls)
 		dp->ssl_upstream = 1;
+	/* prelock forwarders for atomic operation with anchors */
 	lock_rw_wrlock(&fwd->lock);
 	if(insecure && worker->env.anchors) {
 		if(!anchors_add_insecure(worker->env.anchors, LDNS_RR_CLASS_IN,
@@ -2177,7 +2174,7 @@ do_forward_add(RES* ssl, struct worker* worker, char* args)
 			return;
 		}
 	}
-	if(!forwards_add_zone(fwd, LDNS_RR_CLASS_IN, dp)) {
+	if(!forwards_add_zone(fwd, LDNS_RR_CLASS_IN, dp, nolock)) {
 		lock_rw_unlock(&fwd->lock);
 		(void)ssl_printf(ssl, "error out of memory\n");
 		free(nm);
@@ -2195,13 +2192,15 @@ do_forward_remove(RES* ssl, struct worker* worker, char* args)
 	struct iter_forwards* fwd = worker->env.fwds;
 	int insecure = 0;
 	uint8_t* nm = NULL;
+	int nolock = 1;
 	if(!parse_fs_args(ssl, args, &nm, NULL, &insecure, NULL, NULL))
 		return;
+	/* prelock forwarders for atomic operation with anchors */
 	lock_rw_wrlock(&fwd->lock);
 	if(insecure && worker->env.anchors)
 		anchors_delete_insecure(worker->env.anchors, LDNS_RR_CLASS_IN,
 			nm);
-	forwards_delete_zone(fwd, LDNS_RR_CLASS_IN, nm);
+	forwards_delete_zone(fwd, LDNS_RR_CLASS_IN, nm, nolock);
 	lock_rw_unlock(&fwd->lock);
 	free(nm);
 	send_ok(ssl);
@@ -2215,10 +2214,12 @@ do_stub_add(RES* ssl, struct worker* worker, char* args)
 	int insecure = 0, prime = 0, tls = 0;
 	uint8_t* nm = NULL;
 	struct delegpt* dp = NULL;
+	int nolock = 1;
 	if(!parse_fs_args(ssl, args, &nm, &dp, &insecure, &prime, &tls))
 		return;
 	if(tls)
 		dp->ssl_upstream = 1;
+	/* prelock forwarders and hints for atomic operation with anchors */
 	lock_rw_wrlock(&fwd->lock);
 	lock_rw_wrlock(&worker->env.hints->lock);
 	if(insecure && worker->env.anchors) {
@@ -2232,7 +2233,7 @@ do_stub_add(RES* ssl, struct worker* worker, char* args)
 			return;
 		}
 	}
-	if(!forwards_add_stub_hole(fwd, LDNS_RR_CLASS_IN, nm)) {
+	if(!forwards_add_stub_hole(fwd, LDNS_RR_CLASS_IN, nm, nolock)) {
 		if(insecure && worker->env.anchors)
 			anchors_delete_insecure(worker->env.anchors,
 				LDNS_RR_CLASS_IN, nm);
@@ -2243,9 +2244,10 @@ do_stub_add(RES* ssl, struct worker* worker, char* args)
 		free(nm);
 		return;
 	}
-	if(!hints_add_stub(worker->env.hints, LDNS_RR_CLASS_IN, dp, !prime)) {
+	if(!hints_add_stub(worker->env.hints, LDNS_RR_CLASS_IN, dp, !prime,
+		nolock)) {
 		(void)ssl_printf(ssl, "error out of memory\n");
-		forwards_delete_stub_hole(fwd, LDNS_RR_CLASS_IN, nm);
+		forwards_delete_stub_hole(fwd, LDNS_RR_CLASS_IN, nm, nolock);
 		if(insecure && worker->env.anchors)
 			anchors_delete_insecure(worker->env.anchors,
 				LDNS_RR_CLASS_IN, nm);
@@ -2267,15 +2269,17 @@ do_stub_remove(RES* ssl, struct worker* worker, char* args)
 	struct iter_forwards* fwd = worker->env.fwds;
 	int insecure = 0;
 	uint8_t* nm = NULL;
+	int nolock = 1;
 	if(!parse_fs_args(ssl, args, &nm, NULL, &insecure, NULL, NULL))
 		return;
+	/* prelock forwarders and hints for atomic operation with anchors */
 	lock_rw_wrlock(&fwd->lock);
 	lock_rw_wrlock(&worker->env.hints->lock);
 	if(insecure && worker->env.anchors)
 		anchors_delete_insecure(worker->env.anchors, LDNS_RR_CLASS_IN,
 			nm);
-	forwards_delete_stub_hole(fwd, LDNS_RR_CLASS_IN, nm);
-	hints_delete_stub(worker->env.hints, LDNS_RR_CLASS_IN, nm);
+	forwards_delete_stub_hole(fwd, LDNS_RR_CLASS_IN, nm, nolock);
+	hints_delete_stub(worker->env.hints, LDNS_RR_CLASS_IN, nm, nolock);
 	lock_rw_unlock(&fwd->lock);
 	lock_rw_unlock(&worker->env.hints->lock);
 	free(nm);

iterator/iter_fwd.c

@@ -359,39 +359,50 @@ forwards_apply_cfg(struct iter_forwards* fwd, struct config_file* cfg)
 }
 
 struct delegpt* 
-forwards_find(struct iter_forwards* fwd, uint8_t* qname, uint16_t qclass)
+forwards_find(struct iter_forwards* fwd, uint8_t* qname, uint16_t qclass,
+	int nolock)
 {
-	rbnode_type* res = NULL;
+	struct iter_forward_zone* res;
 	struct iter_forward_zone key;
+	int has_dp;
 	key.node.key = &key;
 	key.dclass = qclass;
 	key.name = qname;
 	key.namelabs = dname_count_size_labels(qname, &key.namelen);
-	res = rbtree_search(fwd->tree, &key);
-	if(res) return ((struct iter_forward_zone*)res)->dp;
-	return NULL;
+	/* lock_() calls are macros that could be nothing, surround in {} */
+	if(!nolock) { lock_rw_rdlock(&fwd->lock); }
+	res = (struct iter_forward_zone*)rbtree_search(fwd->tree, &key);
+	has_dp = res && res->dp;
+	if(!has_dp && !nolock) { lock_rw_unlock(&fwd->lock); }
+	return has_dp?res->dp:NULL;
 }
 
 struct delegpt* 
-forwards_lookup(struct iter_forwards* fwd, uint8_t* qname, uint16_t qclass)
+forwards_lookup(struct iter_forwards* fwd, uint8_t* qname, uint16_t qclass,
+	int nolock)
 {
 	/* lookup the forward zone in the tree */
 	rbnode_type* res = NULL;
 	struct iter_forward_zone *result;
 	struct iter_forward_zone key;
+	int has_dp;
 	key.node.key = &key;
 	key.dclass = qclass;
 	key.name = qname;
 	key.namelabs = dname_count_size_labels(qname, &key.namelen);
+	/* lock_() calls are macros that could be nothing, surround in {} */
+	if(!nolock) { lock_rw_rdlock(&fwd->lock); }
 	if(rbtree_find_less_equal(fwd->tree, &key, &res)) {
 		/* exact */
 		result = (struct iter_forward_zone*)res;
 	} else {
 		/* smaller element (or no element) */
 		int m;
 		result = (struct iter_forward_zone*)res;
-		if(!result || result->dclass != qclass)
+		if(!result || result->dclass != qclass) {
+			if(!nolock) { lock_rw_unlock(&fwd->lock); }
 			return NULL;
+		}
 		/* count number of labels matched */
 		(void)dname_lab_cmp(result->name, result->namelabs, key.name,
 			key.namelabs, &m);
@@ -401,20 +412,22 @@ forwards_lookup(struct iter_forwards* fwd, uint8_t* qname, uint16_t qclass)
 			result = result->parent;
 		}
 	}
-	if(result)
-		return result->dp;
-	return NULL;
+	has_dp = result && result->dp;
+	if(!has_dp && !nolock) { lock_rw_unlock(&fwd->lock); }
+	return has_dp?result->dp:NULL;
 }
 
 struct delegpt* 
-forwards_lookup_root(struct iter_forwards* fwd, uint16_t qclass)
+forwards_lookup_root(struct iter_forwards* fwd, uint16_t qclass, int nolock)
 {
 	uint8_t root = 0;
-	return forwards_lookup(fwd, &root, qclass);
+	return forwards_lookup(fwd, &root, qclass, nolock);
 }
 
-int
-forwards_next_root(struct iter_forwards* fwd, uint16_t* dclass)
+/* Finds next root item in forwards lookup tree.
+ * Caller needs to handle locking of the forwards structure. */
+static int
+next_root_locked(struct iter_forwards* fwd, uint16_t* dclass)
 {
 	struct iter_forward_zone key;
 	rbnode_type* n;
@@ -431,7 +444,7 @@ forwards_next_root(struct iter_forwards* fwd, uint16_t* dclass)
 		}
 		/* root not first item? search for higher items */
 		*dclass = p->dclass + 1;
-		return forwards_next_root(fwd, dclass);
+		return next_root_locked(fwd, dclass);
 	}
 	/* find class n in tree, we may get a direct hit, or if we don't
 	 * this is the last item of the previous class so rbtree_next() takes
@@ -459,10 +472,21 @@ forwards_next_root(struct iter_forwards* fwd, uint16_t* dclass)
 		}
 		/* not a root node, return next higher item */
 		*dclass = p->dclass+1;
-		return forwards_next_root(fwd, dclass);
+		return next_root_locked(fwd, dclass);
 	}
 }
 
+int
+forwards_next_root(struct iter_forwards* fwd, uint16_t* dclass, int nolock)
+{
+	int ret;
+	/* lock_() calls are macros that could be nothing, surround in {} */
+	if(!nolock) { lock_rw_rdlock(&fwd->lock); }
+	ret = next_root_locked(fwd, dclass);
+	if(!nolock) { lock_rw_unlock(&fwd->lock); }
+	return ret;
+}
+
 size_t 
 forwards_get_mem(struct iter_forwards* fwd)
 {
@@ -491,51 +515,78 @@ fwd_zone_find(struct iter_forwards* fwd, uint16_t c, uint8_t* nm)
 }
 
 int 
-forwards_add_zone(struct iter_forwards* fwd, uint16_t c, struct delegpt* dp)
+forwards_add_zone(struct iter_forwards* fwd, uint16_t c, struct delegpt* dp,
+	int nolock)
 {
 	struct iter_forward_zone *z;
+	/* lock_() calls are macros that could be nothing, surround in {} */
+	if(!nolock) { lock_rw_wrlock(&fwd->lock); }
 	if((z=fwd_zone_find(fwd, c, dp->name)) != NULL) {
 		(void)rbtree_delete(fwd->tree, &z->node);
 		fwd_zone_free(z);
 	}
-	if(!forwards_insert(fwd, c, dp))
+	if(!forwards_insert(fwd, c, dp)) {
+		if(!nolock) { lock_rw_unlock(&fwd->lock); }
 		return 0;
+	}
 	fwd_init_parents(fwd);
+	if(!nolock) { lock_rw_unlock(&fwd->lock); }
 	return 1;
 }
 
 void 
-forwards_delete_zone(struct iter_forwards* fwd, uint16_t c, uint8_t* nm)
+forwards_delete_zone(struct iter_forwards* fwd, uint16_t c, uint8_t* nm,
+	int nolock)
 {
 	struct iter_forward_zone *z;
-	if(!(z=fwd_zone_find(fwd, c, nm)))
+	/* lock_() calls are macros that could be nothing, surround in {} */
+	if(!nolock) { lock_rw_wrlock(&fwd->lock); }
+	if(!(z=fwd_zone_find(fwd, c, nm))) {
+		if(!nolock) { lock_rw_unlock(&fwd->lock); }
 		return; /* nothing to do */
+	}
 	(void)rbtree_delete(fwd->tree, &z->node);
 	fwd_zone_free(z);
 	fwd_init_parents(fwd);
+	if(!nolock) { lock_rw_unlock(&fwd->lock); }
 }
 
 int
-forwards_add_stub_hole(struct iter_forwards* fwd, uint16_t c, uint8_t* nm)
+forwards_add_stub_hole(struct iter_forwards* fwd, uint16_t c, uint8_t* nm,
+	int nolock)
 {
-	if(fwd_zone_find(fwd, c, nm) != NULL)
+	/* lock_() calls are macros that could be nothing, surround in {} */
+	if(!nolock) { lock_rw_wrlock(&fwd->lock); }
+	if(fwd_zone_find(fwd, c, nm) != NULL) {
+		if(!nolock) { lock_rw_unlock(&fwd->lock); }
 		return 1; /* already a stub zone there */
+	}
 	if(!fwd_add_stub_hole(fwd, c, nm)) {
+		if(!nolock) { lock_rw_unlock(&fwd->lock); }
 		return 0;
 	}
 	fwd_init_parents(fwd);
+	if(!nolock) { lock_rw_unlock(&fwd->lock); }
 	return 1;
 }
 
 void
-forwards_delete_stub_hole(struct iter_forwards* fwd, uint16_t c, uint8_t* nm)
+forwards_delete_stub_hole(struct iter_forwards* fwd, uint16_t c,
+	uint8_t* nm, int nolock)
 {
 	struct iter_forward_zone *z;
-	if(!(z=fwd_zone_find(fwd, c, nm)))
+	/* lock_() calls are macros that could be nothing, surround in {} */
+	if(!nolock) { lock_rw_wrlock(&fwd->lock); }
+	if(!(z=fwd_zone_find(fwd, c, nm))) {
+		if(!nolock) { lock_rw_unlock(&fwd->lock); }
 		return; /* nothing to do */
-	if(z->dp != NULL)
+	}
+	if(z->dp != NULL) {
+		if(!nolock) { lock_rw_unlock(&fwd->lock); }
 		return; /* not a stub hole */
+	}
 	(void)rbtree_delete(fwd->tree, &z->node);
 	fwd_zone_free(z);
 	fwd_init_parents(fwd);
+	if(!nolock) { lock_rw_unlock(&fwd->lock); }
 }