[Asterisk-code-review] pbx: Deadlock between contexts container and context merge l... (asterisk[11])

[Kevin Harwell]

Kevin Harwell has uploaded a new change for review.

  https://gerrit.asterisk.org/1855

Change subject: pbx: Deadlock between contexts container and context_merge locks
......................................................................

pbx: Deadlock between contexts container and context_merge locks

Recent changes (ASTERISK-25394 commit 2bd27d12223fe33b58c453965ed5c6ed3af7c4f5)
introduced the possibility of a deadlock. Due to the mentioned modifications
ast_change_hints now needs to keep both merge/delete and state callbacks from
occurring while it executes. Unfortunately, sometimes ast_change_hints can be
called with the contexts container locked. When this happens it's possible for
another thread to grab the context_merge_lock before the thread calling into
ast_change_hints does and then try to obtain the contexts container lock. This
of course causes a deadlock between the two threads. The thread calling into
ast_change_hints waits for the other thread to release context_merge_lock and
the other thread is waiting on that one to release the contexts container lock.

Unfortunately, there is not a great way to fix this problem. When hints change
the merge/delete cannot run, nor the state callbacks. Picking the least of
evils this patch adds yet another lock: state_callbacks_lock. This lock is held
during state callbacks and ast_change_hints. In order to keep the merge/delete
code from executing at the same time ast_change_hints locks the contexts
container as well. The context_merge_lock no longer needs to be held during
ast_change_hints, thus relieving the possible tension between threads.

ASTERISK-25640 #close
Reported by: Krzysztof Trempala

Change-Id: I7f67ce4b4c94b731c23932cc2ba9e0a4a720813d
---
M main/pbx.c
1 file changed, 26 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/55/1855/1

diff --git a/main/pbx.c b/main/pbx.c
index 0165c88..0b48f99 100644
--- a/main/pbx.c
+++ b/main/pbx.c
@@ -1450,6 +1450,11 @@
  */
 AST_MUTEX_DEFINE_STATIC(context_merge_lock);
 
+/*!
+ * \brief Lock used to protect state data during callbacks
+ */
+AST_MUTEX_DEFINE_STATIC(state_callbacks_lock);
+
 static AST_RWLIST_HEAD_STATIC(apps, ast_app);
 
 static AST_RWLIST_HEAD_STATIC(switches, ast_switch);
@@ -5317,6 +5322,7 @@
 	}
 
 	ast_mutex_lock(&context_merge_lock);/* Hold off ast_merge_contexts_and_delete */
+	ast_mutex_lock(&state_callbacks_lock);
 	i = ao2_iterator_init(hints, 0);
 	for (; (hint = ao2_iterator_next(&i)); ao2_ref(hint, -1)) {
 		struct ast_state_cb *state_cb;
@@ -5408,6 +5414,7 @@
 		ao2_iterator_destroy(&cb_iter);
 	}
 	ao2_iterator_destroy(&i);
+	ast_mutex_unlock(&state_callbacks_lock);
 	ast_mutex_unlock(&context_merge_lock);
 
 	res = 0;
@@ -5528,12 +5535,14 @@
 	strcpy(cmpdevice->hintdevice, sc->dev);
 
 	ast_mutex_lock(&context_merge_lock);/* Hold off ast_merge_contexts_and_delete */
+	ast_mutex_lock(&state_callbacks_lock);
 	dev_iter = ao2_t_callback(hintdevices,
 		OBJ_POINTER | OBJ_MULTIPLE,
 		hintdevice_cmp_multiple,
 		cmpdevice,
 		"find devices in container");
 	if (!dev_iter) {
+		ast_mutex_unlock(&state_callbacks_lock);
 		ast_mutex_unlock(&context_merge_lock);
 		ast_free(hint_app);
 		ast_free(sc);
@@ -5630,6 +5639,7 @@
 
 		ao2_cleanup(device_state_info);
 	}
+	ast_mutex_unlock(&state_callbacks_lock);
 	ast_mutex_unlock(&context_merge_lock);
 
 	ao2_iterator_destroy(dev_iter);
@@ -6013,7 +6023,14 @@
 		return -1;
 	}
 
-	ast_mutex_lock(&context_merge_lock); /* Hold off ast_merge_contexts_and_delete and state changes */
+	/* Lock state callbacks so hints can be manipulated */
+	ast_mutex_lock(&state_callbacks_lock);
+
+	/*
+	 * Hold off ast_merge_contexts_and_delete, but do it by holding the contexts lock.
+	 * If the context_merge_lock is held there is the potential for a deadlock.
+	 */
+	ast_rdlock_contexts();
 
 	ao2_lock(hints);/* Locked to hold off others while we move the hint around. */
 
@@ -6024,7 +6041,8 @@
 	hint = ao2_find(hints, oe, OBJ_UNLINK);
 	if (!hint) {
 		ao2_unlock(hints);
-		ast_mutex_unlock(&context_merge_lock);
+		ast_unlock_contexts();
+		ast_mutex_unlock(&state_callbacks_lock);
 		ast_free(hint_app);
 		return -1;
 	}
@@ -6065,9 +6083,10 @@
 
 	ao2_unlock(hints);
 
-	/* Locking for state callbacks is respected here and only the context_merge_lock lock is
-	 * held during the state callback invocation. This will stop the normal state callback
-	 * thread from being able to handle incoming state changes if they occur.
+	/* Locking for state callbacks is respected here and only the context_merge_lock and
+	 * state_callbacks_lock are held during the state callback invocation. This will stop
+	 * the normal state callback thread from being able to handle incoming state changes
+	 * if they occur.
 	 */
 
 	/* Determine if presence state has changed due to the change of the hint extension */
@@ -6150,7 +6169,8 @@
 
 	ao2_ref(hint, -1);
 
-	ast_mutex_unlock(&context_merge_lock);
+	ast_unlock_contexts();
+	ast_mutex_unlock(&state_callbacks_lock);
 
 	ast_free(hint_app);
 	ast_free(previous_message);

-- 
To view, visit https://gerrit.asterisk.org/1855
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I7f67ce4b4c94b731c23932cc2ba9e0a4a720813d
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 11
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>