From 8c9fba44a41e3ea23e7e8405029980aba672f7ce Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 3 Jun 2015 18:18:55 -0700 Subject: [PATCH] [master] further RPZ fixes 4131. [bug] Addressed further problems with reloading RPZ zones. [RT #39649] --- CHANGES | 3 +++ bin/named/query.c | 39 ++++++------------------------- doc/arm/notes.xml | 7 ++++++ lib/dns/include/dns/rpz.h | 6 +++-- lib/dns/include/dns/zone.h | 4 ++-- lib/dns/rbtdb.c | 6 +++-- lib/dns/rpz.c | 47 +++++++++++++++++++------------------- lib/dns/zone.c | 16 ++++++------- lib/dns/zt.c | 11 +++++---- 9 files changed, 66 insertions(+), 73 deletions(-) diff --git a/CHANGES b/CHANGES index d0821d3884..64fa271f31 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +4131. [bug] Addressed further problems with reloading RPZ + zones. [RT #39649] + 4130. [bug] The compatability shim for *printf() misprinted some large numbers. [RT #39586] diff --git a/bin/named/query.c b/bin/named/query.c index b23d7521d8..fce9efa759 100644 --- a/bin/named/query.c +++ b/bin/named/query.c @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -4246,9 +4247,6 @@ rpz_rrset_find(ns_client_t *client, dns_name_t *name, dns_rdatatype_t type, CTRACE(ISC_LOG_DEBUG(3), "rpz_rrset_find"); - dns_clientinfomethods_init(&cm, ns_client_sourceip); - dns_clientinfo_init(&ci, client, NULL); - st = client->query.rpz_st; if ((st->state & DNS_RPZ_RECURSING) != 0) { INSIST(st->r.r_type == type); @@ -4640,19 +4638,6 @@ rpz_rewrite_ip(ns_client_t *client, const isc_netaddr_t *netaddr, break; zbits &= (DNS_RPZ_ZMASK(rpz_num) >> 1); - /* - * In case num_zones has changed since zbits was - * originally calculated - */ - if (rpz_num >= rpzs->p.num_zones) { - CTRACE(ISC_LOG_ERROR, - "rpz_rewrite_ip: rpz_num is higher than " - "number of zones"); - rpz_clean(&p_zone, &p_db, &p_node, p_rdatasetp); - st->m.policy = DNS_RPZ_POLICY_ERROR; - return (DNS_R_SERVFAIL); - } - /* * Do not try applying policy zones that cannot replace a * previously found policy zone. @@ -4973,19 +4958,6 @@ rpz_rewrite_name(ns_client_t *client, dns_name_t *trig_name, if ((zbits & 1) == 0) continue; - /* - * In case num_zones has changed since the 'have' - * originally calculated - */ - if (rpz_num >= rpzs->p.num_zones) { - CTRACE(ISC_LOG_ERROR, - "rpz_rewrite_name: rpz_num is higher than " - "number of zones"); - rpz_clean(&p_zone, &p_db, &p_node, rdatasetp); - st->m.policy = DNS_RPZ_POLICY_ERROR; - return (DNS_R_SERVFAIL); - } - /* * Do not check policy zones that cannot replace a previously * found policy. @@ -5152,24 +5124,27 @@ rpz_rewrite(ns_client_t *client, dns_rdatatype_t qtype, (st != NULL && (st->state & DNS_RPZ_REWRITTEN) != 0)) return (DNS_R_DISALLOWED); - LOCK(&rpzs->maint_lock); + RWLOCK(&rpzs->search_lock, isc_rwlocktype_read); if (rpzs->p.num_zones == 0 || (!RECURSIONOK(client) && rpzs->p.no_rd_ok == 0) || !rpz_ck_dnssec(client, qresult, ordataset, osigset)) { - UNLOCK(&rpzs->maint_lock); + RWUNLOCK(&rpzs->search_lock, isc_rwlocktype_read); return (DNS_R_DISALLOWED); } have = rpzs->have; popt = rpzs->p; rpz_ver = rpzs->rpz_ver; - UNLOCK(&rpzs->maint_lock); + RWUNLOCK(&rpzs->search_lock, isc_rwlocktype_read); if (st == NULL) { st = isc_mem_get(client->mctx, sizeof(*st)); if (st == NULL) return (ISC_R_NOMEMORY); st->state = 0; + } + if (st->state == 0) { + st->state |= DNS_RPZ_ACTIVE; memset(&st->m, 0, sizeof(st->m)); st->m.type = DNS_RPZ_TYPE_BAD; st->m.policy = DNS_RPZ_POLICY_MISS; diff --git a/doc/arm/notes.xml b/doc/arm/notes.xml index 34951a53f5..c141e0c250 100644 --- a/doc/arm/notes.xml +++ b/doc/arm/notes.xml @@ -662,6 +662,13 @@ corrected. [RT #39481] + + + The server could crash if a reload of an RPZ zone was + initiated while another reload of the same zone was + already in progress. [RT #39649] + + diff --git a/lib/dns/include/dns/rpz.h b/lib/dns/include/dns/rpz.h index 6205b85296..cae90cbec9 100644 --- a/lib/dns/include/dns/rpz.h +++ b/lib/dns/include/dns/rpz.h @@ -21,11 +21,12 @@ #define DNS_RPZ_H 1 #include +#include +#include #include #include #include -#include ISC_LANG_BEGINDECLS @@ -227,7 +228,7 @@ struct dns_rpz_zones { * A second lock for maintenance that guarantees no other thread * is adding or deleting nodes. */ - isc_mutex_t search_lock; + isc_rwlock_t search_lock; isc_mutex_t maint_lock; dns_rpz_cidr_node_t *cidr; @@ -247,6 +248,7 @@ typedef struct { # define DNS_RPZ_DONE_NSDNAME 0x0010 /* NS name missed; checking addresses */ # define DNS_RPZ_DONE_IPv4 0x0020 # define DNS_RPZ_RECURSING 0x0040 +# define DNS_RPZ_ACTIVE 0x0080 /* * Best match so far. */ diff --git a/lib/dns/include/dns/zone.h b/lib/dns/include/dns/zone.h index 8bdcf6c19e..4f44dd8cbd 100644 --- a/lib/dns/include/dns/zone.h +++ b/lib/dns/include/dns/zone.h @@ -1653,8 +1653,8 @@ dns_zonemgr_managezone(dns_zonemgr_t *zmgr, dns_zone_t *zone); isc_result_t dns_zonemgr_forcemaint(dns_zonemgr_t *zmgr); /*%< - * Force zone maintenance of all zones managed by 'zmgr' at its - * earliest convenience. + * Force zone maintenance of all loaded zones managed by 'zmgr' + * to take place at the system's earliest convenience. */ void diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index 5f3a33e485..df67bbc947 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -1134,8 +1134,10 @@ adjust_quantum(unsigned int old, isc_time_t *start) { /* Smooth */ new = (new + old * 3) / 4; - isc_log_write(dns_lctx, DNS_LOGCATEGORY_DATABASE, DNS_LOGMODULE_CACHE, - ISC_LOG_DEBUG(1), "adjust_quantum -> %d", new); + if (new != old) + isc_log_write(dns_lctx, DNS_LOGCATEGORY_DATABASE, + DNS_LOGMODULE_CACHE, ISC_LOG_DEBUG(1), + "adjust_quantum: old=%d, new=%d", old, new); return (new); } diff --git a/lib/dns/rpz.c b/lib/dns/rpz.c index a49d362643..d72ca0fb16 100644 --- a/lib/dns/rpz.c +++ b/lib/dns/rpz.c @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -1390,7 +1391,7 @@ dns_rpz_new_zones(dns_rpz_zones_t **rpzsp, isc_mem_t *mctx) { return (ISC_R_NOMEMORY); memset(new, 0, sizeof(*new)); - result = isc_mutex_init(&new->search_lock); + result = isc_rwlock_init(&new->search_lock, 0, 0); if (result != ISC_R_SUCCESS) { isc_mem_put(mctx, new, sizeof(*new)); return (result); @@ -1398,7 +1399,7 @@ dns_rpz_new_zones(dns_rpz_zones_t **rpzsp, isc_mem_t *mctx) { result = isc_mutex_init(&new->maint_lock); if (result != ISC_R_SUCCESS) { - DESTROYLOCK(&new->search_lock); + isc_rwlock_destroy(&new->search_lock); isc_mem_put(mctx, new, sizeof(*new)); return (result); } @@ -1406,7 +1407,7 @@ dns_rpz_new_zones(dns_rpz_zones_t **rpzsp, isc_mem_t *mctx) { result = isc_refcount_init(&new->refs, 1); if (result != ISC_R_SUCCESS) { DESTROYLOCK(&new->maint_lock); - DESTROYLOCK(&new->search_lock); + isc_rwlock_destroy(&new->search_lock); isc_mem_put(mctx, new, sizeof(*new)); return (result); } @@ -1416,7 +1417,7 @@ dns_rpz_new_zones(dns_rpz_zones_t **rpzsp, isc_mem_t *mctx) { isc_refcount_decrement(&new->refs, NULL); isc_refcount_destroy(&new->refs); DESTROYLOCK(&new->maint_lock); - DESTROYLOCK(&new->search_lock); + isc_rwlock_destroy(&new->search_lock); isc_mem_put(mctx, new, sizeof(*new)); return (result); } @@ -1535,7 +1536,7 @@ dns_rpz_detach_rpzs(dns_rpz_zones_t **rpzsp) { cidr_free(rpzs); dns_rbt_destroy(&rpzs->rbt); DESTROYLOCK(&rpzs->maint_lock); - DESTROYLOCK(&rpzs->search_lock); + isc_rwlock_destroy(&rpzs->search_lock); isc_refcount_destroy(&rpzs->refs); isc_mem_putanddetach(&rpzs->mctx, rpzs, sizeof(*rpzs)); } @@ -1580,7 +1581,7 @@ dns_rpz_beginload(dns_rpz_zones_t **load_rpzsp, */ tgt = DNS_RPZ_ZBIT(rpz_num); LOCK(&rpzs->maint_lock); - LOCK(&rpzs->search_lock); + RWLOCK(&rpzs->search_lock, isc_rwlocktype_write); if ((rpzs->load_begun & tgt) == 0) { /* * There is no existing version of the target zone. @@ -1604,7 +1605,7 @@ dns_rpz_beginload(dns_rpz_zones_t **load_rpzsp, isc_refcount_increment(&rpz->refs, NULL); } - UNLOCK(&rpzs->search_lock); + RWUNLOCK(&rpzs->search_lock, isc_rwlocktype_write); UNLOCK(&rpzs->maint_lock); return (ISC_R_SUCCESS); @@ -1612,7 +1613,7 @@ dns_rpz_beginload(dns_rpz_zones_t **load_rpzsp, /* * This function updates "have" bits and also the qname_skip_recurse - * mask. It must be called when holding rpzs->search_lock. + * mask. It must be called when holding a write lock on rpzs->search_lock. */ static void fix_triggers(dns_rpz_zones_t *rpzs, dns_rpz_num_t rpz_num) { @@ -1740,16 +1741,16 @@ dns_rpz_ready(dns_rpz_zones_t *rpzs, * This is a successful initial zone loading, perhaps * for a new instance of a view. */ - LOCK(&rpzs->search_lock); + RWLOCK(&rpzs->search_lock, isc_rwlocktype_write); fix_triggers(rpzs, rpz_num); - UNLOCK(&rpzs->search_lock); + RWUNLOCK(&rpzs->search_lock, isc_rwlocktype_write); UNLOCK(&rpzs->maint_lock); dns_rpz_detach_rpzs(load_rpzsp); return (ISC_R_SUCCESS); } LOCK(&load_rpzs->maint_lock); - LOCK(&load_rpzs->search_lock); + RWLOCK(&load_rpzs->search_lock, isc_rwlocktype_write); /* * Unless there is only one policy zone, copy the other policy zones @@ -1848,7 +1849,7 @@ dns_rpz_ready(dns_rpz_zones_t *rpzs, /* * Exchange the summary databases. */ - LOCK(&rpzs->search_lock); + RWLOCK(&rpzs->search_lock, isc_rwlocktype_write); rpzs->triggers[rpz_num] = load_rpzs->triggers[rpz_num]; fix_triggers(rpzs, rpz_num); @@ -1861,13 +1862,13 @@ dns_rpz_ready(dns_rpz_zones_t *rpzs, rpzs->rbt = load_rpzs->rbt; load_rpzs->rbt = rbt; - UNLOCK(&rpzs->search_lock); + RWUNLOCK(&rpzs->search_lock, isc_rwlocktype_write); result = ISC_R_SUCCESS; unlock_and_detach: UNLOCK(&rpzs->maint_lock); - UNLOCK(&load_rpzs->search_lock); + RWUNLOCK(&load_rpzs->search_lock, isc_rwlocktype_write); UNLOCK(&load_rpzs->maint_lock); dns_rpz_detach_rpzs(load_rpzsp); return (result); @@ -1890,7 +1891,7 @@ dns_rpz_add(dns_rpz_zones_t *rpzs, dns_rpz_num_t rpz_num, dns_name_t *src_name) rpz_type = type_from_name(rpz, src_name); LOCK(&rpzs->maint_lock); - LOCK(&rpzs->search_lock); + RWLOCK(&rpzs->search_lock, isc_rwlocktype_write); switch (rpz_type) { case DNS_RPZ_TYPE_QNAME: @@ -1906,7 +1907,7 @@ dns_rpz_add(dns_rpz_zones_t *rpzs, dns_rpz_num_t rpz_num, dns_name_t *src_name) break; } - UNLOCK(&rpzs->search_lock); + RWUNLOCK(&rpzs->search_lock, isc_rwlocktype_write); UNLOCK(&rpzs->maint_lock); return (result); } @@ -2096,7 +2097,7 @@ dns_rpz_delete(dns_rpz_zones_t *rpzs, dns_rpz_num_t rpz_num, rpz_type = type_from_name(rpz, src_name); LOCK(&rpzs->maint_lock); - LOCK(&rpzs->search_lock); + RWLOCK(&rpzs->search_lock, isc_rwlocktype_write); switch (rpz_type) { case DNS_RPZ_TYPE_QNAME: @@ -2112,7 +2113,7 @@ dns_rpz_delete(dns_rpz_zones_t *rpzs, dns_rpz_num_t rpz_num, break; } - UNLOCK(&rpzs->search_lock); + RWUNLOCK(&rpzs->search_lock, isc_rwlocktype_write); UNLOCK(&rpzs->maint_lock); } @@ -2197,13 +2198,13 @@ dns_rpz_find_ip(dns_rpz_zones_t *rpzs, dns_rpz_type_t rpz_type, return (DNS_RPZ_INVALID_NUM); make_addr_set(&tgt_set, zbits, rpz_type); - LOCK(&rpzs->search_lock); + RWLOCK(&rpzs->search_lock, isc_rwlocktype_read); result = search(rpzs, &tgt_ip, 128, &tgt_set, ISC_FALSE, &found); if (result == ISC_R_NOTFOUND) { /* * There are no eligible zones for this IP address. */ - UNLOCK(&rpzs->search_lock); + RWUNLOCK(&rpzs->search_lock, isc_rwlocktype_read); return (DNS_RPZ_INVALID_NUM); } @@ -2227,7 +2228,7 @@ dns_rpz_find_ip(dns_rpz_zones_t *rpzs, dns_rpz_type_t rpz_type, break; } result = ip2name(&found->ip, found->prefix, dns_rootname, ip_name); - UNLOCK(&rpzs->search_lock); + RWUNLOCK(&rpzs->search_lock, isc_rwlocktype_read); if (result != ISC_R_SUCCESS) { /* * bin/tests/system/rpz/tests.sh looks for "rpz.*failed". @@ -2260,7 +2261,7 @@ dns_rpz_find_name(dns_rpz_zones_t *rpzs, dns_rpz_type_t rpz_type, found_zbits = 0; - LOCK(&rpzs->search_lock); + RWLOCK(&rpzs->search_lock, isc_rwlocktype_read); nmnode = NULL; result = dns_rbt_findnode(rpzs->rbt, trig_name, NULL, &nmnode, NULL, @@ -2304,7 +2305,7 @@ dns_rpz_find_name(dns_rpz_zones_t *rpzs, dns_rpz_type_t rpz_type, break; } - UNLOCK(&rpzs->search_lock); + RWUNLOCK(&rpzs->search_lock, isc_rwlocktype_read); return (zbits & found_zbits); } diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 6c2b05c3b9..7cb3cfcb7a 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -5752,7 +5752,6 @@ dns_zone_setdb(dns_zone_t *zone, dns_db_t *db) { /* * Co-ordinates the starting of routine jobs. */ - void dns_zone_maintenance(dns_zone_t *zone) { const char me[] = "dns_zone_maintenance"; @@ -12296,8 +12295,9 @@ zone_settimer(dns_zone_t *zone, isc_time_t *now) { isc_time_t next; isc_result_t result; - ENTER; REQUIRE(DNS_ZONE_VALID(zone)); + ENTER; + if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING)) return; @@ -12361,12 +12361,12 @@ zone_settimer(dns_zone_t *zone, isc_time_t *now) { if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESH) && !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOMASTERS) && !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOREFRESH) && - !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADING)) { - INSIST(!isc_time_isepoch(&zone->refreshtime)); - if (isc_time_isepoch(&next) || - isc_time_compare(&zone->refreshtime, &next) < 0) - next = zone->refreshtime; - } + !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADING) && + !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADPENDING) && + !isc_time_isepoch(&zone->refreshtime) && + (isc_time_isepoch(&next) || + isc_time_compare(&zone->refreshtime, &next) < 0)) + next = zone->refreshtime; if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADED) && !isc_time_isepoch(&zone->expiretime)) { if (isc_time_isepoch(&next) || diff --git a/lib/dns/zt.c b/lib/dns/zt.c index 7a39d0d022..28e285542f 100644 --- a/lib/dns/zt.c +++ b/lib/dns/zt.c @@ -314,12 +314,15 @@ asyncload(dns_zone_t *zone, void *callback) { zt = dns_zone_getview(zone)->zonetable; INSIST(VALID_ZT(zt)); + INSIST(zt->references > 0); + zt->references++; + zt->loads_pending++; + result = dns_zone_asyncload(zone, *loaded, zt); - if (result == ISC_R_SUCCESS) { + if (result != ISC_R_SUCCESS) { + zt->references--; + zt->loads_pending--; INSIST(zt->references > 0); - zt->references++; - INSIST(zt->references != 0); - zt->loads_pending++; } return (ISC_R_SUCCESS); }