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 dbe5d95337d50041236c2add8caa2ccbf0b81aab..5402f2422c9e2b59f6ebc2f7895efbe5e988dd7f 100644 (file)
--- 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 01b24301385ea3b9fc1962ded7e345f4d727d75e..54c88de550d991d6555b2457fdaaf609501335d9 100644 (file)
--- 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