bind/backport-0009-Remove-taskmgr-excl_lock-fix-the-locking-for-taskmgr.patch

From 5be356760dd6855944234980f2fc0f13130267fb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= <ondrej@isc.org>
Date: Wed, 5 Jan 2022 13:06:37 +0100
Subject: [PATCH] Remove taskmgr->excl_lock, fix the locking for
 taskmgr->exiting

While doing code review, it was found that the taskmgr->exiting is set
under taskmgr->lock, but accessed under taskmgr->excl_lock in the
isc_task_beginexclusive().

Additionally, before the change that moved running the tasks to the
netmgr, the task_ready() subrouting of isc_task_detach() would lock
mgr->lock, requiring the mgr->excl to be protected mgr->excl_lock
to prevent deadlock in the code.  After !4918 has been merged, this is
no longer true, and we can remove taskmgr->excl_lock and use
taskmgr->lock in its stead.

Solve both issues by removing the taskmgr->excl_lock and exclusively use
taskmgr->lock to protect both taskmgr->excl and taskmgr->exiting which
now doesn't need to be atomic_bool, because it's always accessed from
within the locked section.

(cherry picked from commit e705f213cac8a79e1fa8c20ce20f2e7a28daf3f9)
Conflict: NA
Reference: https://gitlab.isc.org/isc-projects/bind9/-/commit/5be356760dd6855944234980f2fc0f13130267fb
---
 lib/isc/task.c | 54 ++++++++++++++++----------------------------------
 1 file changed, 17 insertions(+), 37 deletions(-)

diff --git a/lib/isc/task.c b/lib/isc/task.c
index 3e894f8dab..caf2c06c47 100644
--- a/lib/isc/task.c
+++ b/lib/isc/task.c
@@ -140,14 +140,7 @@ struct isc_taskmgr {
 	LIST(isc_task_t) tasks;
 	atomic_uint_fast32_t mode;
 	atomic_bool exclusive_req;
-	atomic_bool exiting;
-
-	/*
-	 * Multiple threads can read/write 'excl' at the same time, so we need
-	 * to protect the access.  We can't use 'lock' since isc_task_detach()
-	 * will try to acquire it.
-	 */
-	isc_mutex_t excl_lock;
+	bool exiting;
 	isc_task_t *excl;
 };
 
@@ -254,13 +247,11 @@ isc_task_create_bound(isc_taskmgr_t *manager, unsigned int quantum,
 	INIT_LINK(task, link);
 	task->magic = TASK_MAGIC;
 
-	exiting = false;
 	LOCK(&manager->lock);
-	if (!atomic_load_relaxed(&manager->exiting)) {
+	exiting = manager->exiting;
+	if (!exiting) {
 		APPEND(manager->tasks, task, link);
 		atomic_fetch_add(&manager->tasks_count, 1);
-	} else {
-		exiting = true;
 	}
 	UNLOCK(&manager->lock);
 
@@ -956,7 +947,6 @@ manager_free(isc_taskmgr_t *manager) {
 	isc_nm_detach(&manager->netmgr);
 
 	isc_mutex_destroy(&manager->lock);
-	isc_mutex_destroy(&manager->excl_lock);
 	manager->magic = 0;
 	isc_mem_putanddetach(&manager->mctx, manager, sizeof(*manager));
 }
@@ -1000,7 +990,6 @@ isc__taskmgr_create(isc_mem_t *mctx, unsigned int default_quantum, isc_nm_t *nm,
 	*manager = (isc_taskmgr_t){ .magic = TASK_MANAGER_MAGIC };
 
 	isc_mutex_init(&manager->lock);
-	isc_mutex_init(&manager->excl_lock);
 
 	if (default_quantum == 0) {
 		default_quantum = DEFAULT_DEFAULT_QUANTUM;
@@ -1012,7 +1001,6 @@ isc__taskmgr_create(isc_mem_t *mctx, unsigned int default_quantum, isc_nm_t *nm,
 	}
 
 	INIT_LIST(manager->tasks);
-	atomic_init(&manager->exiting, false);
 	atomic_init(&manager->mode, isc_taskmgrmode_normal);
 	atomic_init(&manager->exclusive_req, false);
 	atomic_init(&manager->tasks_count, 0);
@@ -1041,15 +1029,6 @@ isc__taskmgr_shutdown(isc_taskmgr_t *manager) {
 	 * that the startup thread is sleeping on.
 	 */
 
-	/*
-	 * Detach the exclusive task before acquiring the manager lock
-	 */
-	LOCK(&manager->excl_lock);
-	if (manager->excl != NULL) {
-		isc_task_detach((isc_task_t **)&manager->excl);
-	}
-	UNLOCK(&manager->excl_lock);
-
 	/*
 	 * Unlike elsewhere, we're going to hold this lock a long time.
 	 * We need to do so, because otherwise the list of tasks could
@@ -1058,14 +1037,16 @@ isc__taskmgr_shutdown(isc_taskmgr_t *manager) {
 	 * This is also the only function where we will hold both the
 	 * task manager lock and a task lock at the same time.
 	 */
-
 	LOCK(&manager->lock);
+	if (manager->excl != NULL) {
+		isc_task_detach((isc_task_t **)&manager->excl);
+	}
 
 	/*
 	 * Make sure we only get called once.
 	 */
-	INSIST(atomic_compare_exchange_strong(&manager->exiting,
-					      &(bool){ false }, true));
+	INSIST(manager->exiting == false);
+	manager->exiting = true;
 
 	/*
 	 * Post shutdown event(s) to every task (if they haven't already been
@@ -1120,12 +1101,12 @@ isc_taskmgr_setexcltask(isc_taskmgr_t *mgr, isc_task_t *task) {
 	REQUIRE(task->threadid == 0);
 	UNLOCK(&task->lock);
 
-	LOCK(&mgr->excl_lock);
+	LOCK(&mgr->lock);
 	if (mgr->excl != NULL) {
 		isc_task_detach(&mgr->excl);
 	}
 	isc_task_attach(task, &mgr->excl);
-	UNLOCK(&mgr->excl_lock);
+	UNLOCK(&mgr->lock);
 }
 
 isc_result_t
@@ -1135,16 +1116,16 @@ isc_taskmgr_excltask(isc_taskmgr_t *mgr, isc_task_t **taskp) {
 	REQUIRE(VALID_MANAGER(mgr));
 	REQUIRE(taskp != NULL && *taskp == NULL);
 
-	LOCK(&mgr->excl_lock);
+	LOCK(&mgr->lock);
 	if (mgr->excl != NULL) {
 		isc_task_attach(mgr->excl, taskp);
 		result = ISC_R_SUCCESS;
-	} else if (atomic_load_relaxed(&mgr->exiting)) {
+	} else if (mgr->exiting) {
 		result = ISC_R_SHUTTINGDOWN;
 	} else {
 		result = ISC_R_NOTFOUND;
 	}
-	UNLOCK(&mgr->excl_lock);
+	UNLOCK(&mgr->lock);
 
 	return (result);
 }
@@ -1159,11 +1140,10 @@ isc_task_beginexclusive(isc_task_t *task) {
 
 	REQUIRE(task->state == task_state_running);
 
-	LOCK(&manager->excl_lock);
-	REQUIRE(task == task->manager->excl ||
-		(atomic_load_relaxed(&task->manager->exiting) &&
-		 task->manager->excl == NULL));
-	UNLOCK(&manager->excl_lock);
+	LOCK(&manager->lock);
+	REQUIRE(task == manager->excl ||
+		(manager->exiting && manager->excl == NULL));
+	UNLOCK(&manager->lock);
 
 	if (!atomic_compare_exchange_strong(&manager->exclusive_req,
 					    &(bool){ false }, true))
-- 
2.23.0