From: Alessio Podda Date: Mon, 13 Jul 2026 15:40:06 +0000 (+0200) Subject: Split rpz maint_lock into two X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=1f962f299e91e15a431fab02043ffdcb387caf46;p=thirdparty%2Fbind9.git Split rpz maint_lock into two Before this commit, maint_lock jointly protected both the "summary structure" dns_rpz_zones_t and the individual zones. In particular, locking maint_lock would block timer callbacks and shutdown requests, which is undesirable. Now that the update state has been moved to the baton, there is no longer a reason to have a lock shared among all zones. This commit splits the old rpzs->maint_lock into two. First, rpz->update_lock protects callbacks and timers for individual RPZ zones. Second, rpzs->data_lock protects joint operations on the QP and CIDR trees. The latter replaces the old maint_lock. --- diff --git a/lib/dns/include/dns/rpz.h b/lib/dns/include/dns/rpz.h index dbe5d95337d..5402f2422c9 100644 --- a/lib/dns/include/dns/rpz.h +++ b/lib/dns/include/dns/rpz.h @@ -132,6 +132,9 @@ struct dns_rpz_zone { unsigned int magic; isc_loop_t *loop; + /* Protect this zone's database, timer, and update state. */ + isc_mutex_t update_lock; + dns_rpz_num_t num; /* ordinal in list of policy zones */ dns_name_t origin; /* Policy zone name */ dns_name_t client_ip; /* DNS_RPZ_CLIENT_IP_ZONE.origin. */ @@ -158,8 +161,8 @@ struct dns_rpz_zone { bool updaterunning; /* there is an update running */ dns_db_t *db; /* zones database */ dns_dbversion_t *dbversion; /* version we will be updating to */ - bool addsoa; /* add soa to the additional section */ - isc_timer_t *updatetimer; + bool addsoa; /* add soa to the additional section */ + isc_timer_t *updatetimer; }; /* @@ -254,17 +257,15 @@ struct dns_rpz_zones { */ dns_rpz_triggers_t total_triggers; - /* - * One lock for short term read-only search that guarantees the - * consistency of the pointers. - * A second lock for maintenance that guarantees no other thread - * is adding or deleting nodes. - */ + /* Protect query readers against changes to the CIDR tree. */ isc_rwlock_t search_lock; - isc_mutex_t maint_lock; + + /* Serialize summary QP and CIDR updates and their derived state. */ + isc_mutex_t data_lock; bool first_time; - bool shuttingdown; + /* Publish shutdown without waiting for an update or data lock. */ + atomic_bool shuttingdown; dns_rpz_cidr_node_t *cidr; dns_qpmulti_t *table; diff --git a/lib/dns/rpz.c b/lib/dns/rpz.c index 01b24301385..54c88de550d 100644 --- a/lib/dns/rpz.c +++ b/lib/dns/rpz.c @@ -183,8 +183,8 @@ struct nmdata { }; typedef struct rpz_update { - dns_rpz_zone_t *rpz; - dns_db_t *db; + dns_rpz_zone_t *rpz; + dns_db_t *db; dns_dbversion_t *dbversion; } rpz_update_t; @@ -456,7 +456,7 @@ set_sum_pair(dns_rpz_cidr_node_t *cnode) { } while (cnode != NULL); } -/* Caller must hold rpzs->maint_lock */ +/* Caller must hold rpzs->data_lock. */ static void fix_qname_skip_recurse(dns_rpz_zones_t *rpzs) { dns_rpz_zbits_t mask; @@ -1464,7 +1464,8 @@ dns_rpz_new_zones(dns_view_t *view, dns_rpz_zones_t **rpzsp, bool first_time) { }; isc_rwlock_init(&rpzs->search_lock); - isc_mutex_init(&rpzs->maint_lock); + isc_mutex_init(&rpzs->data_lock); + atomic_init(&rpzs->shuttingdown, false); isc_refcount_init(&rpzs->references, 1); dns_qpmulti_create(mctx, &qpmethods, view, &rpzs->table); @@ -1494,6 +1495,7 @@ dns_rpz_new_zone(dns_rpz_zones_t *rpzs, dns_rpz_zone_t **rpzp) { .magic = DNS_RPZ_ZONE_MAGIC, .rpzs = rpzs, }; + isc_mutex_init(&rpz->update_lock); /* * This will never be used, but costs us nothing and @@ -1530,9 +1532,9 @@ dns_rpz_dbupdate_callback(dns_db_t *db, void *fn_arg) { REQUIRE(DNS_DB_VALID(db)); REQUIRE(DNS_RPZ_ZONE_VALID(rpz)); - LOCK(&rpz->rpzs->maint_lock); + LOCK(&rpz->update_lock); - if (rpz->rpzs->shuttingdown) { + if (atomic_load(&rpz->rpzs->shuttingdown)) { result = ISC_R_SHUTTINGDOWN; goto unlock; } @@ -1574,7 +1576,7 @@ dns_rpz_dbupdate_callback(dns_db_t *db, void *fn_arg) { } unlock: - UNLOCK(&rpz->rpzs->maint_lock); + UNLOCK(&rpz->update_lock); return result; } @@ -1584,7 +1586,7 @@ dns_rpz_dbupdate_unregister(dns_db_t *db, dns_rpz_zone_t *rpz) { REQUIRE(DNS_DB_VALID(db)); REQUIRE(DNS_RPZ_ZONE_VALID(rpz)); - LOCK(&rpz->rpzs->maint_lock); + LOCK(&rpz->update_lock); dns_db_updatenotify_unregister(db, dns_rpz_dbupdate_callback, rpz); if (rpz->processed) { rpz->processed = false; @@ -1596,7 +1598,7 @@ dns_rpz_dbupdate_unregister(dns_db_t *db, dns_rpz_zone_t *rpz) { INSIST(atomic_fetch_sub_acq_rel(&rpz->rpzs->zones_registered, 1) > 0); } - UNLOCK(&rpz->rpzs->maint_lock); + UNLOCK(&rpz->update_lock); } void @@ -1604,13 +1606,13 @@ dns_rpz_dbupdate_register(dns_db_t *db, dns_rpz_zone_t *rpz) { REQUIRE(DNS_DB_VALID(db)); REQUIRE(DNS_RPZ_ZONE_VALID(rpz)); - LOCK(&rpz->rpzs->maint_lock); + LOCK(&rpz->update_lock); if (!rpz->dbregistered) { rpz->dbregistered = true; atomic_fetch_add_acq_rel(&rpz->rpzs->zones_registered, 1); } dns_db_updatenotify_register(db, dns_rpz_dbupdate_callback, rpz); - UNLOCK(&rpz->rpzs->maint_lock); + UNLOCK(&rpz->update_lock); } static void @@ -1665,12 +1667,12 @@ update_rpz_done_cb(void *data, isc_result_t result) { REQUIRE(DNS_RPZ_ZONE_VALID(rpz)); - LOCK(&rpz->rpzs->maint_lock); + LOCK(&rpz->update_lock); rpz->updaterunning = false; dns_name_format(&rpz->origin, dname, DNS_NAME_FORMATSIZE); - if (rpz->updatepending && !rpz->rpzs->shuttingdown) { + if (rpz->updatepending && !atomic_load(&rpz->rpzs->shuttingdown)) { /* Restart the timer */ dns__rpz_timer_start(rpz); } @@ -1683,7 +1685,7 @@ update_rpz_done_cb(void *data, isc_result_t result) { atomic_fetch_add_acq_rel(&rpz->rpzs->zones_processed, 1); } - UNLOCK(&rpz->rpzs->maint_lock); + UNLOCK(&rpz->update_lock); isc_log_write(DNS_LOGCATEGORY_GENERAL, DNS_LOGMODULE_RPZ, ISC_LOG_INFO, "rpz: %s: reload done: %s", dname, @@ -1776,8 +1778,8 @@ cleanup: } static isc_result_t -update_nodes(dns_rpz_zone_t *rpz, dns_db_t *db, - dns_dbversion_t *dbversion, isc_ht_t *newnodes) { +update_nodes(dns_rpz_zone_t *rpz, dns_db_t *db, dns_dbversion_t *dbversion, + isc_ht_t *newnodes) { isc_result_t result; dns_dbiterator_t *updbit = NULL; dns_name_t *name = NULL; @@ -1807,7 +1809,7 @@ update_nodes(dns_rpz_zone_t *rpz, dns_db_t *db, goto cleanup; } - LOCK(&rpz->rpzs->maint_lock); + LOCK(&rpz->rpzs->data_lock); slow_mode = rpz->rpzs->p.slow_mode; dns_qp_t *qp = NULL; @@ -1818,7 +1820,7 @@ update_nodes(dns_rpz_zone_t *rpz, dns_db_t *db, dns_rdatasetiter_t *rdsiter = NULL; dns_dbnode_t *node = NULL; - if (rpz->rpzs->shuttingdown) { + if (atomic_load(&rpz->rpzs->shuttingdown)) { result = ISC_R_SHUTTINGDOWN; goto done; } @@ -1917,7 +1919,7 @@ update_nodes(dns_rpz_zone_t *rpz, dns_db_t *db, done: dns_qp_compact(qp, DNS_QPGC_MAYBE); dns_qpmulti_commit(rpz->rpzs->table, &qp); - UNLOCK(&rpz->rpzs->maint_lock); + UNLOCK(&rpz->rpzs->data_lock); cleanup: dns_dbiterator_destroy(&updbit); @@ -1935,7 +1937,7 @@ cleanup_nodes(dns_rpz_zone_t *rpz) { name = dns_fixedname_initname(&fixname); - LOCK(&rpz->rpzs->maint_lock); + LOCK(&rpz->rpzs->data_lock); dns_qpmulti_write(rpz->rpzs->table, &qp); isc_ht_iter_create(rpz->nodes, &iter); @@ -1947,8 +1949,8 @@ cleanup_nodes(dns_rpz_zone_t *rpz) { unsigned char *key = NULL; size_t keysize; - if (rpz->rpzs->shuttingdown) { - result = ISC_R_SHUTTINGDOWN; + result = dns__rpz_shuttingdown(rpz->rpzs); + if (result != ISC_R_SUCCESS) { break; } @@ -1969,20 +1971,14 @@ cleanup_nodes(dns_rpz_zone_t *rpz) { isc_ht_iter_destroy(&iter); - UNLOCK(&rpz->rpzs->maint_lock); + UNLOCK(&rpz->rpzs->data_lock); return result; } static isc_result_t dns__rpz_shuttingdown(dns_rpz_zones_t *rpzs) { - bool shuttingdown = false; - - LOCK(&rpzs->maint_lock); - shuttingdown = rpzs->shuttingdown; - UNLOCK(&rpzs->maint_lock); - - if (shuttingdown) { + if (atomic_load(&rpzs->shuttingdown)) { return ISC_R_SHUTTINGDOWN; } @@ -2022,13 +2018,13 @@ dns__rpz_timer_cb(void *arg) { rpz_update_t *update = NULL; REQUIRE(DNS_RPZ_ZONE_VALID(rpz)); - REQUIRE(DNS_DB_VALID(rpz->db)); - LOCK(&rpz->rpzs->maint_lock); + LOCK(&rpz->update_lock); - if (rpz->rpzs->shuttingdown) { + if (atomic_load(&rpz->rpzs->shuttingdown)) { goto unlock; } + REQUIRE(DNS_DB_VALID(rpz->db)); rpz->updatepending = false; rpz->updaterunning = true; @@ -2055,7 +2051,7 @@ dns__rpz_timer_cb(void *arg) { rpz->lastupdated = isc_time_now(); unlock: - UNLOCK(&rpz->rpzs->maint_lock); + UNLOCK(&rpz->update_lock); } /* @@ -2093,7 +2089,7 @@ cidr_free(dns_rpz_zones_t *rpzs) { static void dns__rpz_shutdown(dns_rpz_zone_t *rpz) { - /* maint_lock must be locked */ + /* update_lock must be locked. */ if (rpz->updatetimer != NULL) { /* Don't wait for timer to trigger for shutdown */ INSIST(rpz->loop != NULL); @@ -2152,13 +2148,14 @@ dns_rpz_zone_destroy(dns_rpz_zone_t **rpzp) { INSIST(!rpz->updaterunning); isc_ht_destroy(&rpz->nodes); + isc_mutex_destroy(&rpz->update_lock); isc_mem_put(rpzs->mctx, rpz, sizeof(*rpz)); } static void dns__rpz_zones_destroy(dns_rpz_zones_t *rpzs) { - REQUIRE(rpzs->shuttingdown); + REQUIRE(atomic_load(&rpzs->shuttingdown)); for (dns_rpz_num_t rpz_num = 0; rpz_num < DNS_RPZ_MAX_ZONES; ++rpz_num) { @@ -2174,7 +2171,7 @@ dns__rpz_zones_destroy(dns_rpz_zones_t *rpzs) { dns_qpmulti_destroy(&rpzs->table); } - isc_mutex_destroy(&rpzs->maint_lock); + isc_mutex_destroy(&rpzs->data_lock); isc_rwlock_destroy(&rpzs->search_lock); isc_mem_putanddetach(&rpzs->mctx, rpzs, sizeof(*rpzs)); } @@ -2186,23 +2183,22 @@ dns_rpz_zones_shutdown(dns_rpz_zones_t *rpzs) { * Forget the last of the view's rpz machinery when shutting down. */ - LOCK(&rpzs->maint_lock); - if (rpzs->shuttingdown) { - UNLOCK(&rpzs->maint_lock); + if (!atomic_compare_exchange_strong(&rpzs->shuttingdown, + &(bool){ false }, true)) + { return; } - rpzs->shuttingdown = true; - for (dns_rpz_num_t rpz_num = 0; rpz_num < DNS_RPZ_MAX_ZONES; ++rpz_num) { if (rpzs->zones[rpz_num] == NULL) { continue; } + LOCK(&rpzs->zones[rpz_num]->update_lock); dns__rpz_shutdown(rpzs->zones[rpz_num]); + UNLOCK(&rpzs->zones[rpz_num]->update_lock); } - UNLOCK(&rpzs->maint_lock); } #ifdef DNS_RPZ_TRACE