From: Diego Fronza <diego@isc.org>
Date: Fri, 16 Apr 2021 15:21:31 +0000 (-0300)
Subject: Fix deadlock between rndc addzone/delzone/modzone
X-Git-Tag: v9.16.16~24^2~2
X-Git-Url: http://git.ipfire.org/gitweb/?a=commitdiff_plain;h=942b83d3926e6b50f75c0ad73e8870db3af76500;p=thirdparty%2Fbind9.git

Fix deadlock between rndc addzone/delzone/modzone

It follows a description of the steps that were leading to the deadlock:

1. `do_addzone` calls `isc_task_beginexclusive`.

2. `isc_task_beginexclusive` waits for (N_WORKERS - 1) halted tasks,
   this blocks waiting for those (no. workers -1) workers to halt.
...
isc_task_beginexclusive(isc_task_t *task0) {
    ...
	while (manager->halted + 1 < manager->workers) {
		wake_all_queues(manager);
		WAIT(&manager->halt_cond, &manager->halt_lock);
	}
```

3. It is possible that in `task.c / dispatch()` a worker is running a
   task event, if that event blocks it will not allow this worker to
   halt.

4. `do_addzone` acquires `LOCK(&view->new_zone_lock);`,

5. `rmzone` event is called from some worker's `dispatch()`, `rmzone`
   blocks waiting for the same lock.

6. `do_addzone` calls `isc_task_beginexclusive`.

7. Deadlock triggered, since:
	- `rmzone` is wating for the lock.
	- `isc_task_beginexclusive` is waiting for (no. workers - 1) to
	   be halted
	- since `rmzone` event is blocked it won't allow the worker to halt.

To fix this, we updated do_addzone code to call isc_task_beginexclusive
before the lock is acquired, we postpone locking to the nearest required
place, same for isc_task_beginexclusive.

The same could happen with rndc modzone, so that was addressed as well.
---

diff --git a/bin/named/server.c b/bin/named/server.c
index 343dc6acc89..e2b44c4b173 100644
--- a/bin/named/server.c
+++ b/bin/named/server.c
@@ -13342,13 +13342,13 @@ do_addzone(named_server_t *server, ns_cfgctx_t *cfg, dns_view_t *view,
 #ifndef HAVE_LMDB
 	FILE *fp = NULL;
 	bool cleanup_config = false;
-#else  /* HAVE_LMDB */
+#else /* HAVE_LMDB */
 	MDB_txn *txn = NULL;
 	MDB_dbi dbi;
+	bool locked = false;
 
 	UNUSED(zoneconf);
-	LOCK(&view->new_zone_lock);
-#endif /* HAVE_LMDB */
+#endif
 
 	/* Zone shouldn't already exist */
 	if (redirect) {
@@ -13368,12 +13368,16 @@ do_addzone(named_server_t *server, ns_cfgctx_t *cfg, dns_view_t *view,
 		goto cleanup;
 	}
 
+	result = isc_task_beginexclusive(server->task);
+	RUNTIME_CHECK(result == ISC_R_SUCCESS);
+
 #ifndef HAVE_LMDB
 	/*
 	 * Make sure we can open the configuration save file
 	 */
 	result = isc_stdio_open(view->new_zone_file, "a", &fp);
 	if (result != ISC_R_SUCCESS) {
+		isc_task_endexclusive(server->task);
 		TCHECK(putstr(text, "unable to create '"));
 		TCHECK(putstr(text, view->new_zone_file));
 		TCHECK(putstr(text, "': "));
@@ -13384,9 +13388,12 @@ do_addzone(named_server_t *server, ns_cfgctx_t *cfg, dns_view_t *view,
 	(void)isc_stdio_close(fp);
 	fp = NULL;
 #else  /* HAVE_LMDB */
+	LOCK(&view->new_zone_lock);
+	locked = true;
 	/* Make sure we can open the NZD database */
 	result = nzd_writable(view);
 	if (result != ISC_R_SUCCESS) {
+		isc_task_endexclusive(server->task);
 		TCHECK(putstr(text, "unable to open NZD database for '"));
 		TCHECK(putstr(text, view->new_zone_db));
 		TCHECK(putstr(text, "'"));
@@ -13395,9 +13402,6 @@ do_addzone(named_server_t *server, ns_cfgctx_t *cfg, dns_view_t *view,
 	}
 #endif /* HAVE_LMDB */
 
-	result = isc_task_beginexclusive(server->task);
-	RUNTIME_CHECK(result == ISC_R_SUCCESS);
-
 	/* Mark view unfrozen and configure zone */
 	dns_view_thaw(view);
 	result = configure_zone(cfg->config, zoneobj, cfg->vconfig,
@@ -13501,7 +13505,9 @@ cleanup:
 	if (txn != NULL) {
 		(void)nzd_close(&txn, false);
 	}
-	UNLOCK(&view->new_zone_lock);
+	if (locked) {
+		UNLOCK(&view->new_zone_lock);
+	}
 #endif /* HAVE_LMDB */
 
 	if (zone != NULL) {
@@ -13525,7 +13531,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);
+	bool locked = false;
 #endif /* HAVE_LMDB */
 
 	/* Zone must already exist */
@@ -13571,6 +13577,8 @@ do_modzone(named_server_t *server, ns_cfgctx_t *cfg, dns_view_t *view,
 	(void)isc_stdio_close(fp);
 	fp = NULL;
 #else  /* HAVE_LMDB */
+	LOCK(&view->new_zone_lock);
+	locked = true;
 	/* Make sure we can open the NZD database */
 	result = nzd_writable(view);
 	if (result != ISC_R_SUCCESS) {
@@ -13723,7 +13731,9 @@ cleanup:
 	if (txn != NULL) {
 		(void)nzd_close(&txn, false);
 	}
-	UNLOCK(&view->new_zone_lock);
+	if (locked) {
+		UNLOCK(&view->new_zone_lock);
+	}
 #endif /* HAVE_LMDB */
 
 	if (zone != NULL) {