packages/bind
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity
bind/backport-0027-Fix-locking-for-LMDB-0.9.26.patch
jiangheng ad37c37958 backport some patches from community 
(cherry picked from commit a9fd9ece9b9436b6103d084920c6897ef1adbae6)
2022-09-03 21:36:21 +08:00

314 lines
9.9 KiB
Diff
Raw Permalink Blame History

From 25818ac81fbc5665b101dead4b406a4dfe2d4486 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= <michal@isc.org>
Date: Fri, 10 Jul 2020 11:29:18 +0200
Subject: [PATCH] Fix locking for LMDB 0.9.26
When "rndc reconfig" is run, named first configures a fresh set of views
and then tears down the old views. Consider what happens for a single
view with LMDB enabled; "envA" is the pointer to the LMDB environment
used by the original/old version of the view, "envB" is the pointer to
the same LMDB environment used by the new version of that view:
1. mdb_env_open(envA) is called when the view is first created.
2. "rndc reconfig" is called.
3. mdb_env_open(envB) is called for the new instance of the view.
4. mdb_env_close(envA) is called for the old instance of the view.
This seems to have worked so far. However, an upstream change [1] in
LMDB which will be part of its 0.9.26 release prevents the above
sequence of calls from working as intended because the locktable mutexes
will now get destroyed by the mdb_env_close() call in step 4 above,
causing any subsequent mdb_txn_begin() calls to fail (because all of the
above steps are happening within a single named process).
Preventing the above scenario from happening would require either
redesigning the way we use LMDB in BIND, which is not something we can
easily backport, or redesigning the way BIND carries out its
reconfiguration process, which would be an even more severe change.
To work around the problem, set MDB_NOLOCK when calling mdb_env_open()
to stop LMDB from controlling concurrent access to the database and do
the necessary locking in named instead. Reuse the view->new_zone_lock
mutex for this purpose to prevent the need for modifying struct dns_view
(which would necessitate library API version bumps). Drop use of
MDB_NOTLS as it is made redundant by MDB_NOLOCK: MDB_NOTLS only affects
where LMDB reader locktable slots are stored while MDB_NOLOCK prevents
the reader locktable from being used altogether.
[1] https://git.openldap.org/openldap/openldap/-/commit/2fd44e325195ae81664eb5dc36e7d265927c5ebc
(cherry picked from commit 53120279b57e25b6462ef3ac4ef9c205a4e9192b)
Conflict: NA
Reference: https://gitlab.isc.org/isc-projects/bind9/-/commit/25818ac81fbc5665b101dead4b406a4dfe2d4486
---
.gitlab-ci.yml | 5 +---
bin/named/server.c | 61 +++++++++++++++++++++++++++++++++-----
lib/dns/include/dns/view.h | 7 +----
3 files changed, 55 insertions(+), 18 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index ea312c36de..bcea3c38e0 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -1099,8 +1099,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
@@ -1126,8 +1124,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
diff --git a/bin/named/server.c b/bin/named/server.c
index a2fa2c864c..5fd9fc1176 100644
--- a/bin/named/server.c
+++ b/bin/named/server.c
@@ -6797,6 +6797,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;
@@ -6811,6 +6813,8 @@ count_newzones(dns_view_t *view, ns_cfgctx_t *nzcfg, int *num_zonesp) {
if (result != ISC_R_SUCCESS)
*num_zonesp = 0;
+ UNLOCK(&view->new_zone_lock);
+
return (ISC_R_SUCCESS);
}
@@ -7116,6 +7120,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,
@@ -7228,8 +7234,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);
}
@@ -7256,6 +7265,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);
}
@@ -7277,6 +7289,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(ns_g_lctx,
@@ -7310,6 +7324,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(ns_g_addparser, &zoneconf);
}
@@ -11638,8 +11654,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);
@@ -11739,8 +11753,6 @@ nzd_save(MDB_txn **txnp, MDB_dbi dbi, dns_zone_t *zone,
}
*txnp = NULL;
- UNLOCK(&view->new_zone_lock);
-
if (text != NULL) {
isc_buffer_free(&text);
}
@@ -11748,6 +11760,11 @@ nzd_save(MDB_txn **txnp, MDB_dbi dbi, dns_zone_t *zone,
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;
@@ -11779,6 +11796,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;
@@ -11909,6 +11931,13 @@ nzd_env_reopen(dns_view_t *view) {
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;
@@ -11931,6 +11960,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;
@@ -11979,6 +12014,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.
@@ -12122,6 +12159,8 @@ migrate_nzf(dns_view_t *view) {
result = nzd_close(&txn, commit);
}
+ UNLOCK(&view->new_zone_lock);
+
if (text != NULL) {
isc_buffer_free(&text);
}
@@ -12325,6 +12364,7 @@ do_addzone(ns_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 */
@@ -12465,6 +12505,7 @@ do_addzone(ns_server_t *server, ns_cfgctx_t *cfg, dns_view_t *view,
#else /* HAVE_LMDB */
if (txn != NULL)
(void) nzd_close(&txn, false);
+ UNLOCK(&view->new_zone_lock);
#endif /* HAVE_LMDB */
if (zone != NULL)
@@ -12488,6 +12529,7 @@ do_modzone(ns_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 */
@@ -12667,6 +12709,7 @@ do_modzone(ns_server_t *server, ns_cfgctx_t *cfg, dns_view_t *view,
#else /* HAVE_LMDB */
if (txn != NULL)
(void) nzd_close(&txn, false);
+ UNLOCK(&view->new_zone_lock);
#endif /* HAVE_LMDB */
if (zone != NULL)
@@ -12816,6 +12859,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(ns_g_lctx, NS_LOGCATEGORY_GENERAL,
@@ -12834,6 +12878,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
result = delete_zoneconf(view, cfg->add_parser,
cfg->nzf_config,
@@ -12926,10 +12975,6 @@ rmzone(isc_task_t *task, isc_event_t *event) {
}
}
-#ifdef HAVE_LMDB
- if (txn != NULL)
- (void) nzd_close(&txn, false);
-#endif
if (raw != NULL)
dns_zone_detach(&raw);
dns_zone_detach(&zone);
diff --git a/lib/dns/include/dns/view.h b/lib/dns/include/dns/view.h
index c849dec154..09a9725de1 100644
--- a/lib/dns/include/dns/view.h
+++ b/lib/dns/include/dns/view.h
@@ -240,12 +240,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__ */
--
2.23.0
Reference in New Issue View Git Blame Copy Permalink