channels: Avoid deadlocks when locking ao2_container *channels #311

intellasoft · 2023-09-09T19:10:38Z

One situation where deadlocks occur is when:
-> res_pjsip_sdp_rtp is checking for rtp timeouts -> Which locks
ao2_container channels, and then waits for a lock on a channel
that's currently running IMPORT
At the same time
-> Dialplan asks for IMPORT(${some_channel},...) -> Which grabs a
channel lock and then waits on ao2_container channels

So in this situation, res_pjsip_sdp_rtp is waiting on a channel lock
that will never release, because res_pjsip_sdp_rtp is currently
holding ao2_container channels, that the IMPORT needs after having
already locked the channel that res_pjsip_sdp_rtp is processing next.

Resolves: #307

cherry-pick-to: 19
cherry-pick-to: 20
cherry-pick-to: master

asterisk-org-access-app · 2023-09-09T19:12:56Z

REMINDER: If this PR applies to other branches, please add a comment with the appropriate "cherry-pick-to" headers as per the Create a Pull Request process.

If you don't want it cherry-picked, please add a comment with cherry-pick-to: none so we don't keep asking.

If, after adding "cherry-pick-to" comments, you change your mind, please edit the comment to DELETE the header lines and add cherry-pick-to: none.

The currently active branches are now 18, 20, 21 and master.

InterLinked1

Remove the cherry pick lines from the commit message and make a comment with those instead.

InterLinked1 · 2023-09-09T19:12:39Z

main/channel.c

@@ -712,12 +718,23 @@ static int does_id_conflict(const char *uniqueid)
 {
 	struct ast_channel *conflict;
 	size_t length = 0;
+	struct ast_channel_callback_data callback_data = {.search = uniqueid, .final_match_result = 0};


Can we put each element on its own line? Not sure if that's explicitly in the coding guidelines but that's easier to read.

InterLinked1 · 2023-09-09T19:13:53Z

main/channel.c


 	if (ast_strlen_zero(uniqueid)) {
 		return 0;
 	}

-	conflict = ast_channel_callback(ast_channel_by_uniqueid_cb, (char *) uniqueid, &length, OBJ_NOLOCK);
+  try_again:


I don't think goto is necessary here, this could be a do while or for (;;) with break

InterLinked1 · 2023-09-09T19:15:45Z

main/channel.c

+
+	if (callback_data.final_match_result == CMP_RETRY_NEEDED) {
+		/* We ran into a possible deadlock, try again */
+		ast_debug(3, "ast_channel_iterator_by_exten_new() Avoid deadlock trying to find channel by extension@context: %s@%s\n", exten, context);


ast_debug includes the function name, no need to repeat it.

This could all be simplified to:

`ast_debug(3, "Avoided deadlock trying to find channel by %@%s\n", exten, context);

InterLinked1 · 2023-09-09T19:16:11Z

main/channel.c


 	if (ast_strlen_zero(l_name)) {
 		/* We didn't have a name to search for so quit. */
 		return NULL;
 	}

-	chan = ast_channel_callback(ast_channel_by_name_cb, l_name, &name_len,
+  try_name_again:


Same thing here, you can use 2 loops here and goto isn't needed.

Are goto's that bad? It reduces nesting. The only thing that adding a loop does is introduce an extra level of nesting.

They're not inherently bad, this just doesn't seem like a necessary or appropriate use for them, since a loop works perfectly fine. I don't think there's a problem with adding a level of nesting, it contains the logic in its own scope.

InterLinked1 · 2023-09-09T19:16:19Z

main/channel.c

+
+	if (callback_data.final_match_result == CMP_RETRY_NEEDED) {
+		/* We ran into a possible deadlock, try again */
+		ast_debug(3, "ast_channel_get_by_name_prefix() Avoid deadlock trying to find channel by uniqueid: %s\n", name);


Same comment about log format

kobaz · 2023-09-09T19:21:31Z

cherry-pick-to: 19
cherry-pick-to: 20
cherry-pick-to: master

InterLinked1 · 2023-09-09T19:25:41Z

cherry-pick-to: 19
cherry-pick-to: 20
cherry-pick-to: master

PRs should be raised against master, not 18 (unlike Gerrit, it makes a difference now).
And it should be cherry picked to 18, 20, and 21, not 19.

kobaz · 2023-09-09T19:27:27Z

master is slightly different. It will not cleanly cherry-pick to branches from master. Is the process still go to to master first? Make separate PRs for each branch?

InterLinked1 · 2023-09-09T19:30:04Z

master is slightly different. It will not cleanly cherry-pick to branches from master. Is the process still go to to master first? Make separate PRs for each branch?

Ah, okay, then it's more complicated, then you might need to do multiple sets since GitHub can only cherry pick verbatim, might want to double check with George.

kobaz · 2023-09-10T17:03:24Z

master is slightly different. It will not cleanly cherry-pick to branches from master. Is the process still go to to master first? Make separate PRs for each branch?

Ah, okay, then it's more complicated, then you might need to do multiple sets since GitHub can only cherry pick verbatim, might want to double check with George.

master no longer has support for macros, so the checks in ast_channel_by_exten_cb() will not cherry-pick clean with the change as-is.

InterLinked1 · 2023-09-10T16:49:16Z

main/channel.c

+		/* We found a match or the search fully completed through all channels */
+		break;
+	}
+	while (1);


while should go on previous line
Though really I think for(;;) is preferred in Asterisk for infinite loops.

InterLinked1 · 2023-09-10T17:16:14Z

main/channel.c

+		/* We found a match or the search fully completed through all channels */
+		break;
+	}
+	while (1);


InterLinked1 · 2023-09-10T17:17:00Z

main/channel.c

+		/* We found a match or the search fully completed through all channels */
+		break;
+	}
+	while (1);


kobaz · 2023-09-21T14:49:24Z

Update: Been running this in the wild with mixed results.

Some days it's been completely solid with no deadlocks (better than before with 3-5 deadlocks per day or more).

Some days it actually almost immediately deadlocked after handling just a few calls.

jcolp · 2023-09-29T13:43:42Z

Due to the last comment I am transitioning this to a draft as it does not appear to be in a state for review as it does not resolve the underlying issue. If this is untrue and it should actually be reviewed, then it can be published.

kobaz · 2023-09-30T03:06:57Z

Fixed previous issue where it did not fully fix the problem. Small refactor to avoid breaking assumptions about value of 'arg' with respect to find channel callbacks.

It's now passing my tests regarding the original deadlock situation.

I would nominate this ready for review now.

InterLinked1 · 2023-10-03T12:32:27Z

include/asterisk/channel.h

+ *   Will be passed to cb_fn as its arg param
+ * \param data Arbitrary data to be passed to cb_fn as its data param
+ *
+ * \note This function SHOULD NOT be called unlessfully understanding the implications.


unlessfully?

Haha... yeah I noticed that after I sent the last push. Should be 'unless fully'

InterLinked1 · 2023-10-03T12:32:51Z

include/asterisk/channel.h

 *
+ * \param arg Must be a char *, Typically either a channel name or a channel uniqueid.


typically should not be capitalized

InterLinked1 · 2023-10-03T12:34:18Z

main/channel.c

+struct ast_channel *ast_channel_callback(ao2_callback_data_fn *cb_fn, void *arg,
+		void *data, int ao2_flags)
+{
+	ast_log(LOG_WARNING, "Using ast_channel_callback() directly may lead to deadlocks.  Please update your module.\n");


This is an existing function, right? Won't that start spewing warnings all over the place? If this really was a proper fix, then everything else should be updated at the same time too. But I would hold off on that for now until that is determined.

This is a public api function. All internal usages across the core and modules have been updated in this patch. People who may have written 3rd party modules that iterate through channels with ao2_channel_callback would need to be rewritten to use the safe version (which needs to be trylock aware) so there's no 'drop-in' replacement for ast_channel_callback.

3rd party modules would get this warning. I was thinking about it that there should be an option to turn the warning off, but I believe avoiding deadlocks is important enough it should be pretty loud (with the ability to mute).

InterLinked1 · 2023-10-03T12:34:46Z

main/channel.c

 static int does_id_conflict(const char *uniqueid)
 {
 	struct ast_channel *conflict;
+        char *l_uniqueid = (char *) uniqueid;


You're pointlessly casting to the same thing here

It's dropping the const qualifier so that you don't have to cast it for the callback.

InterLinked1 · 2023-10-03T12:35:04Z

main/channel.c

 	char *context = arg;
-	char *exten = data;
+	char *exten = (char *) callback_data->data_additional;


Redundant cast

Same... dropping the const qualifier.

InterLinked1 · 2023-10-03T12:35:41Z

main/channel.c


 	if (!(i = ast_calloc(1, sizeof(*i)))) {
 		return NULL;
 	}

-	i->active_iterator = (void *) ast_channel_callback(ast_channel_by_exten_cb,
-		l_context, l_exten, OBJ_MULTIPLE);
+	i->active_iterator = (void *) channel_find_by_exten(exten, context, OBJ_MULTIPLE);


You don't need to cast to void

This was the original code that had the cast... but yeah I agree.

InterLinked1 · 2023-10-03T12:35:49Z

main/channel.c

@@ -1388,9 +1487,10 @@ struct ast_channel_iterator *ast_channel_iterator_by_name_new(const char *name,
 		return NULL;
 	}

-	i->active_iterator = (void *) ast_channel_callback(ast_channel_by_name_cb,
-		l_name, &name_len,
+	i->active_iterator = (void *) ast_channel_callback_safe(ast_channel_by_name_cb,


So, I was thinking about this. Yes the void * cast is not required, but it does add code clarity. Normally ast_channel_callback returns a channel, but in this case with OBJ_MULTIPLE it returns an iterator containing the list of matched channels so (void *) is more applicable here for syntactic clarity.

Looking at this some more, the void * is required.

InterLinked1 · 2023-10-03T12:37:03Z

apps/app_directed_pickup.c

 	if (target) {
 		return target;
 	}

 	/* Now try a search for uniqueid. */
 	pickup_args.name = part;
 	pickup_args.len = 0;
-	return ast_channel_callback(find_by_uniqueid, NULL, &pickup_args, 0);
+
+	return ast_channel_callback_safe(find_by_uniqueid, "channel uniqueid, where pickup is possible", pickup_args.name, NULL, &pickup_args, 0);


The description is identical to line 229. Shouldn't they be different if the goal is to be unique?
Additionally I think automatically include the filename and line number is more robust than hardcoding descriptions. In which case, there should be a macro that injects these automatically.

One situation where deadlocks occur is when: -> res_pjsip_sdp_rtp is checking for rtp timeouts -> Which locks ao2_container channels, and then waits for a lock on a channel that's currently running IMPORT At the same time -> Dialplan asks for IMPORT(${some_channel},...) -> Which grabs a channel lock and then waits on ao2_container channels So in this situation, res_pjsip_sdp_rtp is waiting on a channel lock that will never release, because res_pjsip_sdp_rtp is currently holding ao2_container channels, that the IMPORT needs after having already locked the channel that res_pjsip_sdp_rtp is processing next. UserNote: Module developers should discontinue use of ast_channel_callback() and use ast_channel_callback_safe() instead. (This will avoid causing deadlocks) Resolves: asterisk#307

kobaz · 2023-10-08T15:27:44Z

Pushed a new one.. fixes channel reference leaks (introduced in the previous patch) when OBJ_MULTIPLE is used to return an iterator

kobaz · 2023-10-08T16:47:59Z

While trying to extract out components from our full platform into a standalone deadlock reproduction test, I've uncovered an additional deadlock related to ast_sip_push_task_wait() also related to IMPORT import_helper() (posted to #307). I realized this is not the same deadlock that this #311 pull is addressing. So I'll work on another test to trigger this specific one as well.

This is related, but probably should be split into a new issue since this other newly uncovered deadlock is not related to ao2_container *channels.

gtjoseph · 2024-01-23T16:42:08Z

I'm converting this back to a draft again since there are still outstanding changes requested and the cherry-pick lines still aren't correct.

Speaking of which, are you saying this PR will cherry-pick cleanly from 18 to master but not the other way around?

kobaz · 2024-01-23T16:43:35Z

Correct, I don't think it goes the other way, but I can do some tests.

asterisk-org-access-app bot requested a review from a team September 9, 2023 19:12

github-actions bot added testing-in-progress test-checks-failed and removed testing-in-progress labels Sep 9, 2023

InterLinked1 suggested changes Sep 9, 2023

View reviewed changes

intellasoft force-pushed the 18-issue-307 branch from 8fa1b68 to e200e31 Compare September 9, 2023 19:19

github-actions bot added testing-in-progress test-checks-failed and removed test-checks-failed testing-in-progress labels Sep 9, 2023

intellasoft force-pushed the 18-issue-307 branch from e200e31 to 52184d3 Compare September 10, 2023 16:47

github-actions bot added testing-in-progress test-checks-failed and removed test-checks-failed testing-in-progress labels Sep 10, 2023

InterLinked1 suggested changes Sep 10, 2023

View reviewed changes

jcolp marked this pull request as draft September 29, 2023 13:43

intellasoft force-pushed the 18-issue-307 branch from 52184d3 to 520b377 Compare September 30, 2023 03:04

github-actions bot added testing-in-progress and removed test-checks-failed labels Sep 30, 2023

github-actions bot added the test-checks-passed label Sep 30, 2023

github-actions bot added the test-gates-failed label Oct 2, 2023

InterLinked1 suggested changes Oct 3, 2023

View reviewed changes

intellasoft force-pushed the 18-issue-307 branch from 274087e to 59b0d33 Compare October 3, 2023 18:44

github-actions bot added testing-in-progress test-checks-passed test-gates-failed and removed test-checks-passed test-gates-failed testing-in-progress labels Oct 3, 2023

intellasoft force-pushed the 18-issue-307 branch from 59b0d33 to a000ddc Compare October 4, 2023 13:10

github-actions bot added testing-in-progress test-checks-passed test-gates-failed and removed test-checks-passed test-gates-failed testing-in-progress labels Oct 4, 2023

intellasoft force-pushed the 18-issue-307 branch from a000ddc to 3579cc5 Compare October 8, 2023 15:26

github-actions bot added testing-in-progress test-checks-passed test-gates-failed and removed test-checks-passed test-gates-failed testing-in-progress labels Oct 8, 2023

gtjoseph marked this pull request as draft January 23, 2024 16:42

gtjoseph assigned intellasoft Jan 23, 2024

		*
		* \param arg Must be a char *, Typically either a channel name or a channel uniqueid.

channels: Avoid deadlocks when locking ao2_container *channels #311

Are you sure you want to change the base?

channels: Avoid deadlocks when locking ao2_container *channels #311

Conversation

intellasoft commented Sep 9, 2023

asterisk-org-access-app bot commented Sep 9, 2023

InterLinked1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kobaz commented Sep 9, 2023

InterLinked1 commented Sep 9, 2023

kobaz commented Sep 9, 2023

InterLinked1 commented Sep 9, 2023 • edited

kobaz commented Sep 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kobaz commented Sep 21, 2023

jcolp commented Sep 29, 2023

kobaz commented Sep 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kobaz commented Oct 8, 2023

kobaz commented Oct 8, 2023

gtjoseph commented Jan 23, 2024

kobaz commented Jan 23, 2024

InterLinked1 commented Sep 9, 2023 •

edited