From: Ralph Dolmans <ralph@nlnetlabs.nl>
Date: Thu, 26 Mar 2020 18:11:57 +0000 (+0100)
Subject: - Fix RPZ concurrency issue when using auth_zone_reload.
X-Git-Tag: release-1.11.0~77
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e4eb76a5f33f406b468b88fd1f18ebdeaeb21feb;p=thirdparty%2Funbound.git

- Fix RPZ concurrency issue when using auth_zone_reload.
---

diff --git a/doc/Changelog b/doc/Changelog
index 316dfacdf..0ecda426f 100644
--- a/doc/Changelog
+++ b/doc/Changelog
@@ -1,3 +1,6 @@
+26 March 2020: Ralph
+	- Fix RPZ concurrency issue when using auth_zone_reload.
+
 25 March 2020: George
 	- Merge PR #201 from noloader: Fix OpenSSL cross-compaile warnings.
 	- Fix on #201.
diff --git a/respip/respip.c b/respip/respip.c
index c496653c4..6fa4f1885 100644
--- a/respip/respip.c
+++ b/respip/respip.c
@@ -914,6 +914,7 @@ respip_rewrite_reply(const struct query_info* qinfo,
 	int ret = 1;
 	struct ub_packed_rrset_key* redirect_rrset = NULL;
 	struct rpz* r;
+	struct auth_zone* a;
 	struct ub_packed_rrset_key* data = NULL;
 	int rpz_used = 0;
 	int rpz_log = 0;
@@ -955,6 +956,10 @@ respip_rewrite_reply(const struct query_info* qinfo,
 		}
 		if(!raddr && !view->isfirst)
 			goto done;
+		if(!raddr && view->isfirst) {
+			lock_rw_unlock(&view->lock);
+			view = NULL;
+		}
 	}
 	if(!raddr && (raddr = respip_addr_lookup(rep, ipset,
 		&rrset_id))) {
@@ -965,7 +970,9 @@ respip_rewrite_reply(const struct query_info* qinfo,
 			ipset->tagname, ipset->num_tags);
 	}
 	lock_rw_rdlock(&az->rpz_lock);
-	for(r = az->rpz_first; r && !raddr; r = r->next) {
+	for(a = az->rpz_first; a && !raddr; a = a->rpz_az_next) {
+		lock_rw_rdlock(&a->lock);
+		r = a->rpz;
 		if(!r->taglist || taglist_intersect(r->taglist, 
 			r->taglistlen, ctaglist, ctaglen)) {
 			if((raddr = respip_addr_lookup(rep,
@@ -975,16 +982,21 @@ respip_rewrite_reply(const struct query_info* qinfo,
 					region, &rpz_used)) {
 					log_err("out of memory");
 					lock_rw_unlock(&raddr->lock);
+					lock_rw_unlock(&a->lock);
 					lock_rw_unlock(&az->rpz_lock);
 					return 0;
 				}
-				if(!rpz_used) {
-					lock_rw_unlock(&raddr->lock);
-					raddr = NULL;
-					actinfo->rpz_disabled++;
+				if(rpz_used) {
+					/* break to make sure 'a' stays pointed
+					 * to used auth_zone, and keeps lock */
+					break;
 				}
+				lock_rw_unlock(&raddr->lock);
+				raddr = NULL;
+				actinfo->rpz_disabled++;
 			}
-		}	
+		}
+		lock_rw_unlock(&a->lock);
 	}
 	lock_rw_unlock(&az->rpz_lock);
 	if(raddr && !search_only) {
@@ -1038,6 +1050,9 @@ respip_rewrite_reply(const struct query_info* qinfo,
 	if(raddr) {
 		lock_rw_unlock(&raddr->lock);
 	}
+	if(rpz_used) {
+		lock_rw_unlock(&a->lock);
+	}
 	return ret;
 }
 
diff --git a/services/authzone.c b/services/authzone.c
index c5803757a..70fe27a5e 100644
--- a/services/authzone.c
+++ b/services/authzone.c
@@ -392,12 +392,12 @@ auth_zone_delete(struct auth_zone* z, struct auth_zones* az)
 	if(az && z->rpz) {
 		/* keep RPZ linked list intact */
 		lock_rw_wrlock(&az->rpz_lock);
-		if(z->rpz->prev)
-			z->rpz->prev->next = z->rpz->next;
+		if(z->rpz_az_prev)
+			z->rpz_az_prev->rpz_az_next = z->rpz_az_next;
 		else
-			az->rpz_first = z->rpz->next;
-		if(z->rpz->next)
-			z->rpz->next->prev = z->rpz->prev;
+			az->rpz_first = z->rpz_az_next;
+		if(z->rpz_az_next)
+			z->rpz_az_next->rpz_az_prev = z->rpz_az_prev;
 		lock_rw_unlock(&az->rpz_lock);
 	}
 	if(z->rpz)
@@ -426,9 +426,11 @@ auth_zone_create(struct auth_zones* az, uint8_t* nm, size_t nmlen,
 	}
 	rbtree_init(&z->data, &auth_data_cmp);
 	lock_rw_init(&z->lock);
-	lock_protect(&z->lock, &z->name, sizeof(*z)-sizeof(rbnode_type));
+	lock_protect(&z->lock, &z->name, sizeof(*z)-sizeof(rbnode_type)-
+			sizeof(&z->rpz_az_next)-sizeof(&z->rpz_az_prev));
 	lock_rw_wrlock(&z->lock);
-	/* z lock protects all, except rbtree itself, which is az->lock */
+	/* z lock protects all, except rbtree itself and the rpz linked list
+	 * pointers, which are protected using az->lock */
 	if(!rbtree_insert(&az->ztree, &z->node)) {
 		lock_rw_unlock(&z->lock);
 		auth_zone_delete(z, NULL);
@@ -1897,11 +1899,12 @@ auth_zones_cfg(struct auth_zones* az, struct config_auth* c)
 			fatal_exit("Could not setup RPZ zones");
 			return 0;
 		}
+		lock_protect(&z->lock, &z->rpz->local_zones, sizeof(*z->rpz));
 		lock_rw_wrlock(&az->rpz_lock);
-		z->rpz->next = az->rpz_first;
+		z->rpz_az_next = az->rpz_first;
 		if(az->rpz_first)
-			az->rpz_first->prev = z->rpz;
-		az->rpz_first = z->rpz;
+			az->rpz_first->rpz_az_prev = z;
+		az->rpz_first = z;
 		lock_rw_unlock(&az->rpz_lock);
 	}
 
diff --git a/services/authzone.h b/services/authzone.h
index 9bb131ad8..3d94f30d6 100644
--- a/services/authzone.h
+++ b/services/authzone.h
@@ -82,8 +82,8 @@ struct auth_zones {
 	size_t num_query_up;
 	/** number of queries downstream */
 	size_t num_query_down;
-	/** first rpz item in linked list */
-	struct rpz* rpz_first;
+	/** first auth zone containing rpz item in linked list */
+	struct auth_zone* rpz_first;
 	/** rw lock for rpz linked list, needed when iterating or editing linked
 	 * list. */
 	lock_rw_type rpz_lock;
@@ -138,6 +138,11 @@ struct auth_zone {
 	int zone_deleted;
 	/** deletelist pointer, unused normally except during delete */
 	struct auth_zone* delete_next;
+	/* not protected by auth_zone lock, must be last items in struct */
+	/** next auth zone containing RPZ data, or NULL */
+	struct auth_zone* rpz_az_next;
+	/** previous auth zone containing RPZ data, or NULL */
+	struct auth_zone* rpz_az_prev;
 };
 
 /**
diff --git a/services/rpz.c b/services/rpz.c
index efb7ad5a8..105f238d0 100644
--- a/services/rpz.c
+++ b/services/rpz.c
@@ -834,6 +834,8 @@ rpz_remove_qname_trigger(struct rpz* r, uint8_t* dname, size_t dnamelen,
 		delete_zone = rpz_data_delete_rr(z, dname,
 			dnamelen, rr_type, rdatawl, rdatalen);
 	else if(a != localzone_type_to_rpz_action(z->type)) {
+		lock_rw_unlock(&z->lock);
+		lock_rw_unlock(&r->local_zones->lock);
 		return;
 	}
 	lock_rw_unlock(&z->lock); 
@@ -939,13 +941,16 @@ rpz_apply_qname_trigger(struct auth_zones* az, struct module_env* env,
 	struct regional* temp, struct comm_reply* repinfo,
 	uint8_t* taglist, size_t taglen, struct ub_server_stats* stats)
 {
-	struct rpz* r;
+	struct rpz* r = NULL;
+	struct auth_zone* a;
 	int ret;
 	enum localzone_type lzt;
 	struct local_zone* z = NULL;
 	struct local_data* ld = NULL;
 	lock_rw_rdlock(&az->rpz_lock);
-	for(r = az->rpz_first; r; r = r->next) {
+	for(a = az->rpz_first; a; a = a->rpz_az_next) {
+		lock_rw_rdlock(&a->lock);
+		r = a->rpz;
 		if(!r->taglist || taglist_intersect(r->taglist, 
 			r->taglistlen, taglist, taglen)) {
 			z = rpz_find_zone(r, qinfo->qname, qinfo->qname_len,
@@ -963,13 +968,14 @@ rpz_apply_qname_trigger(struct auth_zones* az, struct module_env* env,
 			}
 			if(z)
 				break;
-		}	
+		}
+		lock_rw_unlock(&a->lock); /* not found in this auth_zone */
 	}
 	lock_rw_unlock(&az->rpz_lock);
 	if(!z)
-		return 0;
+		return 0; /* not holding auth_zone.lock anymore */
 
-	
+	log_assert(r);
 	if(r->action_override == RPZ_NO_OVERRIDE_ACTION)
 		lzt = z->type;
 	else
@@ -980,6 +986,7 @@ rpz_apply_qname_trigger(struct auth_zones* az, struct module_env* env,
 			regional_alloc_zero(temp, sizeof(struct local_rrset));
 		if(!qinfo->local_alias) {
 			lock_rw_unlock(&z->lock);
+			lock_rw_unlock(&a->lock);
 			return 0; /* out of memory */
 		}
 		qinfo->local_alias->rrset =
@@ -987,6 +994,7 @@ rpz_apply_qname_trigger(struct auth_zones* az, struct module_env* env,
 				sizeof(*r->cname_override));
 		if(!qinfo->local_alias->rrset) {
 			lock_rw_unlock(&z->lock);
+			lock_rw_unlock(&a->lock);
 			return 0; /* out of memory */
 		}
 		qinfo->local_alias->rrset->rk.dname = qinfo->qname;
@@ -996,6 +1004,7 @@ rpz_apply_qname_trigger(struct auth_zones* az, struct module_env* env,
 				qinfo, repinfo, r->log_name);
 		stats->rpz_action[RPZ_CNAME_OVERRIDE_ACTION]++;
 		lock_rw_unlock(&z->lock);
+		lock_rw_unlock(&a->lock);
 		return 0;
 	}
 
@@ -1008,6 +1017,7 @@ rpz_apply_qname_trigger(struct auth_zones* az, struct module_env* env,
 				repinfo, r->log_name);
 		stats->rpz_action[localzone_type_to_rpz_action(lzt)]++;
 		lock_rw_unlock(&z->lock);
+		lock_rw_unlock(&a->lock);
 		return !qinfo->local_alias;
 	}
 
@@ -1018,6 +1028,7 @@ rpz_apply_qname_trigger(struct auth_zones* az, struct module_env* env,
 			qinfo, repinfo, r->log_name);
 	stats->rpz_action[localzone_type_to_rpz_action(lzt)]++;
 	lock_rw_unlock(&z->lock);
+	lock_rw_unlock(&a->lock);
 
 	return ret;
 }
diff --git a/services/rpz.h b/services/rpz.h
index a404350d3..77a2db55c 100644
--- a/services/rpz.h
+++ b/services/rpz.h
@@ -86,7 +86,8 @@ enum rpz_action {
 /**
  * RPZ containing policies. Pointed to from corresponding auth-zone. Part of a
  * linked list to keep configuration order. Iterating or changing the linked
- * list requires the rpz_lock from struct auth_zones.
+ * list requires the rpz_lock from struct auth_zones. Changing items in this
+ * struct require the lock from struct auth_zone.
  */
 struct rpz {
 	struct local_zones* local_zones;
@@ -97,8 +98,6 @@ struct rpz {
 	struct ub_packed_rrset_key* cname_override;
 	int log;
 	char* log_name;
-	struct rpz* next;
-	struct rpz* prev;
 	struct regional* region;
 };