Merge branch '1976-fix-locking-for-lmdb-0.9.26' into 'main'

Fix locking for LMDB 0.9.26

Closes #1976

See merge request isc-projects/bind9!3758

.gitlab-ci.yml

@@ -1101,8 +1101,6 @@ clang:freebsd11.4:amd64:
   variables:
     CFLAGS: "${CFLAGS_COMMON}"
     USER: gitlab-runner
-    # Temporarily disable LMDB support [GL #1976]
-    EXTRA_CONFIGURE: "--without-lmdb"
   <<: *freebsd_amd64
   <<: *build_job
 
@@ -1128,8 +1126,7 @@ unit:clang:freebsd11.4:amd64:
 clang:freebsd12.1:amd64:
   variables:
     CFLAGS: "${CFLAGS_COMMON}"
-    # Temporarily disable LMDB support [GL #1976]
-    EXTRA_CONFIGURE: "--enable-dnstap --without-lmdb"
+    EXTRA_CONFIGURE: "--enable-dnstap"
     USER: gitlab-runner
   <<: *freebsd_amd64
   <<: *build_job

CHANGES

@@ -1,3 +1,5 @@
+5462.	[bug]		Move LMDB locking from LMDB itself to named. [GL #1976]
+
 5461.	[bug]		The header STALE attribute was not being updated with
 			the write lock being held leading to incorrect
 			statistics.  Convert the header attributes to use atomic

bin/named/server.c

@@ -7582,6 +7582,8 @@ count_newzones(dns_view_t *view, ns_cfgctx_t *nzcfg, int *num_zonesp) {
 		      "for view '%s'",
 		      view->new_zone_db, view->name);
 
+	LOCK(&view->new_zone_lock);
+
 	CHECK(nzd_count(view, &n));
 
 	*num_zonesp = n;
@@ -7596,6 +7598,8 @@ cleanup:
 		*num_zonesp = 0;
 	}
 
+	UNLOCK(&view->new_zone_lock);
+
 	return (ISC_R_SUCCESS);
 }
 
@@ -7924,6 +7928,8 @@ typedef isc_result_t (*newzone_cfg_cb_t)(const cfg_obj_t *zconfig,
  * Immediately interrupt processing if an error is encountered while
  * transforming NZD data into a zone configuration object or if "callback"
  * returns an error.
+ *
+ * Caller must hold 'view->new_zone_lock'.
  */
 static isc_result_t
 for_all_newzone_cfgs(newzone_cfg_cb_t callback, cfg_obj_t *config,
@@ -8032,8 +8038,11 @@ configure_newzones(dns_view_t *view, cfg_obj_t *config, cfg_obj_t *vconfig,
 		return (ISC_R_SUCCESS);
 	}
 
+	LOCK(&view->new_zone_lock);
+
 	result = nzd_open(view, MDB_RDONLY, &txn, &dbi);
 	if (result != ISC_R_SUCCESS) {
+		UNLOCK(&view->new_zone_lock);
 		return (ISC_R_SUCCESS);
 	}
 
@@ -8059,6 +8068,9 @@ configure_newzones(dns_view_t *view, cfg_obj_t *config, cfg_obj_t *vconfig,
 	}
 
 	(void)nzd_close(&txn, false);
+
+	UNLOCK(&view->new_zone_lock);
+
 	return (result);
 }
 
@@ -8079,6 +8091,8 @@ get_newzone_config(dns_view_t *view, const char *zonename,
 
 	INSIST(zoneconfig != NULL && *zoneconfig == NULL);
 
+	LOCK(&view->new_zone_lock);
+
 	CHECK(nzd_open(view, MDB_RDONLY, &txn, &dbi));
 
 	isc_log_write(named_g_lctx, NAMED_LOGCATEGORY_GENERAL,
@@ -8112,6 +8126,8 @@ get_newzone_config(dns_view_t *view, const char *zonename,
 cleanup:
 	(void)nzd_close(&txn, false);
 
+	UNLOCK(&view->new_zone_lock);
+
 	if (zoneconf != NULL) {
 		cfg_obj_destroy(named_g_addparser, &zoneconf);
 	}
@@ -12569,8 +12585,6 @@ nzd_save(MDB_txn **txnp, MDB_dbi dbi, dns_zone_t *zone,
 
 	nzd_setkey(&key, dns_zone_getorigin(zone), namebuf, sizeof(namebuf));
 
-	LOCK(&view->new_zone_lock);
-
 	if (zconfig == NULL) {
 		/* We're deleting the zone from the database */
 		status = mdb_del(*txnp, dbi, &key, NULL);
@@ -12650,8 +12664,6 @@ cleanup:
 	}
 	*txnp = NULL;
 
-	UNLOCK(&view->new_zone_lock);
-
 	if (text != NULL) {
 		isc_buffer_free(&text);
 	}
@@ -12659,6 +12671,11 @@ cleanup:
 	return (result);
 }
 
+/*
+ * Check whether the new zone database for 'view' can be opened for writing.
+ *
+ * Caller must hold 'view->new_zone_lock'.
+ */
 static isc_result_t
 nzd_writable(dns_view_t *view) {
 	isc_result_t result = ISC_R_SUCCESS;
@@ -12688,6 +12705,11 @@ nzd_writable(dns_view_t *view) {
 	return (result);
 }
 
+/*
+ * Open the new zone database for 'view' and start a transaction for it.
+ *
+ * Caller must hold 'view->new_zone_lock'.
+ */
 static isc_result_t
 nzd_open(dns_view_t *view, unsigned int flags, MDB_txn **txnp, MDB_dbi *dbi) {
 	int status;
@@ -12815,6 +12837,13 @@ cleanup:
 	return (result);
 }
 
+/*
+ * If 'commit' is true, commit the new zone database transaction pointed to by
+ * 'txnp'; otherwise, abort that transaction.
+ *
+ * Caller must hold 'view->new_zone_lock' for the view that the transaction
+ * pointed to by 'txnp' was started for.
+ */
 static isc_result_t
 nzd_close(MDB_txn **txnp, bool commit) {
 	isc_result_t result = ISC_R_SUCCESS;
@@ -12837,6 +12866,12 @@ nzd_close(MDB_txn **txnp, bool commit) {
 	return (result);
 }
 
+/*
+ * Count the zones configured in the new zone database for 'view' and store the
+ * result in 'countp'.
+ *
+ * Caller must hold 'view->new_zone_lock'.
+ */
 static isc_result_t
 nzd_count(dns_view_t *view, int *countp) {
 	isc_result_t result;
@@ -12884,6 +12919,8 @@ migrate_nzf(dns_view_t *view) {
 	MDB_val key, data;
 	ns_dzarg_t dzarg;
 
+	LOCK(&view->new_zone_lock);
+
 	/*
 	 * If NZF file doesn't exist, or NZD DB exists and already
 	 * has data, return without attempting migration.
@@ -13019,6 +13056,8 @@ cleanup:
 		result = nzd_close(&txn, commit);
 	}
 
+	UNLOCK(&view->new_zone_lock);
+
 	if (text != NULL) {
 		isc_buffer_free(&text);
 	}
@@ -13228,6 +13267,7 @@ do_addzone(named_server_t *server, ns_cfgctx_t *cfg, dns_view_t *view,
 	MDB_dbi dbi;
 
 	UNUSED(zoneconf);
+	LOCK(&view->new_zone_lock);
 #endif /* HAVE_LMDB */
 
 	/* Zone shouldn't already exist */
@@ -13381,6 +13421,7 @@ cleanup:
 	if (txn != NULL) {
 		(void)nzd_close(&txn, false);
 	}
+	UNLOCK(&view->new_zone_lock);
 #endif /* HAVE_LMDB */
 
 	if (zone != NULL) {
@@ -13404,6 +13445,7 @@ do_modzone(named_server_t *server, ns_cfgctx_t *cfg, dns_view_t *view,
 #else  /* HAVE_LMDB */
 	MDB_txn *txn = NULL;
 	MDB_dbi dbi;
+	LOCK(&view->new_zone_lock);
 #endif /* HAVE_LMDB */
 
 	/* Zone must already exist */
@@ -13601,6 +13643,7 @@ cleanup:
 	if (txn != NULL) {
 		(void)nzd_close(&txn, false);
 	}
+	UNLOCK(&view->new_zone_lock);
 #endif /* HAVE_LMDB */
 
 	if (zone != NULL) {
@@ -13764,6 +13807,7 @@ rmzone(isc_task_t *task, isc_event_t *event) {
 	if (added && cfg != NULL) {
 #ifdef HAVE_LMDB
 		/* Make sure we can open the NZD database */
+		LOCK(&view->new_zone_lock);
 		result = nzd_open(view, 0, &txn, &dbi);
 		if (result != ISC_R_SUCCESS) {
 			isc_log_write(named_g_lctx, NAMED_LOGCATEGORY_GENERAL,
@@ -13781,6 +13825,11 @@ rmzone(isc_task_t *task, isc_event_t *event) {
 				      "delete zone configuration: %s",
 				      isc_result_totext(result));
 		}
+
+		if (txn != NULL) {
+			(void)nzd_close(&txn, false);
+		}
+		UNLOCK(&view->new_zone_lock);
 #else  /* ifdef HAVE_LMDB */
 		result = delete_zoneconf(view, cfg->add_parser, cfg->nzf_config,
 					 dns_zone_getorigin(zone),
@@ -13870,11 +13919,6 @@ rmzone(isc_task_t *task, isc_event_t *event) {
 		}
 	}
 
-#ifdef HAVE_LMDB
-	if (txn != NULL) {
-		(void)nzd_close(&txn, false);
-	}
-#endif /* ifdef HAVE_LMDB */
 	if (raw != NULL) {
 		dns_zone_detach(&raw);
 	}

lib/dns/include/dns/view.h

@@ -251,12 +251,7 @@ struct dns_view {
 
 #ifdef HAVE_LMDB
 #include <lmdb.h>
-/*
- * MDB_NOTLS is used to prevent problems after configuration is reloaded, due
- * to the way LMDB's use of thread-local storage (TLS) interacts with the BIND9
- * thread model.
- */
-#define DNS_LMDB_COMMON_FLAGS (MDB_CREATE | MDB_NOSUBDIR | MDB_NOTLS)
+#define DNS_LMDB_COMMON_FLAGS (MDB_CREATE | MDB_NOSUBDIR | MDB_NOLOCK)
 #ifndef __OpenBSD__
 #define DNS_LMDB_FLAGS (DNS_LMDB_COMMON_FLAGS)
 #else /* __OpenBSD__ */