- Fix RPZ concurrency issue when using auth_zone_reload.

diff --git a/doc/Changelog b/doc/Changelog
index 316dfacdf93a3317048353d47bd290fecd66e8e8..0ecda426fb32e87bc97a5c41f008f8582fb8e75c 100644 (file)
--- 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 c496653c41dd3bb5158ddef3b75ac94bee3b0cd9..6fa4f18851fdefbf6cc5325f60cabd5f837c4189 100644 (file)
--- 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 c5803757ac1d75ad2ed58caddc23a36470933a2c..70fe27a5edc60afa8ec47fa82d5e7f965d425e00 100644 (file)
--- 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 9bb131ad8b39523074dfa8f2226f6c4ff1961932..3d94f30d6202c38100802372300bd3934d528ea5 100644 (file)
--- 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 efb7ad5a8828325e0980e7abaf6a501cf5696712..105f238d0a6d0b4b5d0a1601fac598f6603c580e 100644 (file)
--- 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 a404350d31cbf4f67f7d4c9ce8bbc400109d5901..77a2db55ced429b8daaae02872fec8f5ab805cf5 100644 (file)
--- 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;
 };