[asterisk-commits] chan iax2: Prevent deadlock between hangup and sending lagrq... (asterisk[13])
SVN commits to the Asterisk project
asterisk-commits at lists.digium.com
Wed Jun 10 12:05:50 CDT 2015
Mark Michelson has submitted this change and it was merged.
Change subject: chan_iax2: Prevent deadlock between hangup and sending lagrq/ping
......................................................................
chan_iax2: Prevent deadlock between hangup and sending lagrq/ping
channels/chan_iax.c: Prevent the deadlock between iax2_hangup and send_lagrq/
send_ping. This deadlock happens because the scheduled task send_lagrq(or
send_ping) starts execution after the call hangup procedure starts but before
it deletes the tasks in the scheduler.
The solution is to delete scheduled lagrq (and ping) task asynchronously
(i.e. schedule AST_SCHED_DEL for these tasks); By this, AST_SCHED_DEL will
be called in a new context (doesn't have callno locked).
This commit also cleans up the procedure of sending LAGRQ and PING.
main/sched.c: Do not assert when deleting non existant entry from scheduler.
This assert seems to be the reason for a lot of awkward code to avoid it.
ASTERISK-24983 #close
Reported by: Y Ateya
Change-Id: I03bec1fc8faacb89630269e935fa667c6d6c080c
---
M channels/chan_iax2.c
M main/sched.c
2 files changed, 86 insertions(+), 53 deletions(-)
Approvals:
Mark Michelson: Looks good to me, approved; Verified
diff --git a/channels/chan_iax2.c b/channels/chan_iax2.c
index 6e8e9e4..efe1bd3 100644
--- a/channels/chan_iax2.c
+++ b/channels/chan_iax2.c
@@ -395,8 +395,6 @@
static struct io_context *io;
static struct ast_sched_context *sched;
-#define DONT_RESCHEDULE -2
-
static iax2_format iax2_capability = IAX_CAPABILITY_FULLBANDWIDTH;
static int iaxdebug = 0;
@@ -864,6 +862,8 @@
int frames_dropped;
/*! received frame count: (just for stats) */
int frames_received;
+ /*! Destroying this call initiated. */
+ int destroy_initiated;
/*! num bytes used for calltoken ie, even an empty ie should contain 2 */
unsigned char calltoken_ie_len;
/*! hold all signaling frames from the pbx thread until we have a destination callno */
@@ -1672,23 +1672,48 @@
return ast_sched_add(con, when, callback, data);
}
+/*
+ * \brief Acquire the iaxsl[callno] if call exists and not having ongoing hangup.
+ * \param callno Call number to lock.
+ * \return 0 If call disappeared or has ongoing hangup procedure. 1 If call found and mutex is locked.
+ */
+static int iax2_lock_callno_unless_destroyed(int callno)
+{
+ ast_mutex_lock(&iaxsl[callno]);
+
+ /* We acquired the lock; but the call was already destroyed (we came after full hang up procedures)
+ * or destroy initiated (in middle of hang up procedure. */
+ if (!iaxs[callno] || iaxs[callno]->destroy_initiated) {
+ ast_debug(3, "I wanted to lock callno %d, but it is dead or going to die.\n", callno);
+ ast_mutex_unlock(&iaxsl[callno]);
+ return 0;
+ }
+
+ /* Lock acquired, and callno is alive and kicking. */
+ return 1;
+}
+
static int send_ping(const void *data);
static void __send_ping(const void *data)
{
- int callno = (long) data;
+ int callno = PTR_TO_CALLNO(data);
- ast_mutex_lock(&iaxsl[callno]);
+ if (iax2_lock_callno_unless_destroyed(callno) == 0) {
+ ast_debug(3, "Hangup initiated on call %d, aborting __send_ping\n", callno);
+ return;
+ }
- if (iaxs[callno]) {
- if (iaxs[callno]->peercallno) {
- send_command(iaxs[callno], AST_FRAME_IAX, IAX_COMMAND_PING, 0, NULL, 0, -1);
- if (iaxs[callno]->pingid != DONT_RESCHEDULE) {
- iaxs[callno]->pingid = iax2_sched_add(sched, ping_time * 1000, send_ping, data);
- }
- }
- } else {
- ast_debug(1, "I was supposed to send a PING with callno %d, but no such call exists.\n", callno);
+ /* Mark pingid as invalid scheduler id. */
+ iaxs[callno]->pingid = -1;
+
+ /* callno is now locked. */
+ if (iaxs[callno]->peercallno) {
+ /* Send PING packet. */
+ send_command(iaxs[callno], AST_FRAME_IAX, IAX_COMMAND_PING, 0, NULL, 0, -1);
+
+ /* Schedule sending next ping. */
+ iaxs[callno]->pingid = iax2_sched_add(sched, ping_time * 1000, send_ping, data);
}
ast_mutex_unlock(&iaxsl[callno]);
@@ -1696,13 +1721,6 @@
static int send_ping(const void *data)
{
- int callno = (long) data;
- ast_mutex_lock(&iaxsl[callno]);
- if (iaxs[callno] && iaxs[callno]->pingid != DONT_RESCHEDULE) {
- iaxs[callno]->pingid = -1;
- }
- ast_mutex_unlock(&iaxsl[callno]);
-
#ifdef SCHED_MULTITHREADED
if (schedule_action(__send_ping, data))
#endif
@@ -1743,19 +1761,23 @@
static void __send_lagrq(const void *data)
{
- int callno = (long) data;
+ int callno = PTR_TO_CALLNO(data);
- ast_mutex_lock(&iaxsl[callno]);
+ if (iax2_lock_callno_unless_destroyed(callno) == 0) {
+ ast_debug(3, "Hangup initiated on call %d, aborting __send_lagrq\n", callno);
+ return;
+ }
- if (iaxs[callno]) {
- if (iaxs[callno]->peercallno) {
- send_command(iaxs[callno], AST_FRAME_IAX, IAX_COMMAND_LAGRQ, 0, NULL, 0, -1);
- if (iaxs[callno]->lagid != DONT_RESCHEDULE) {
- iaxs[callno]->lagid = iax2_sched_add(sched, lagrq_time * 1000, send_lagrq, data);
- }
- }
- } else {
- ast_debug(1, "I was supposed to send a LAGRQ with callno %d, but no such call exists.\n", callno);
+ /* Mark lagid as invalid scheduler id. */
+ iaxs[callno]->lagid = -1;
+
+ /* callno is now locked. */
+ if (iaxs[callno]->peercallno) {
+ /* Send LAGRQ packet. */
+ send_command(iaxs[callno], AST_FRAME_IAX, IAX_COMMAND_LAGRQ, 0, NULL, 0, -1);
+
+ /* Schedule sending next lagrq. */
+ iaxs[callno]->lagid = iax2_sched_add(sched, lagrq_time * 1000, send_lagrq, data);
}
ast_mutex_unlock(&iaxsl[callno]);
@@ -1763,13 +1785,6 @@
static int send_lagrq(const void *data)
{
- int callno = (long) data;
- ast_mutex_lock(&iaxsl[callno]);
- if (iaxs[callno] && iaxs[callno]->lagid != DONT_RESCHEDULE) {
- iaxs[callno]->lagid = -1;
- }
- ast_mutex_unlock(&iaxsl[callno]);
-
#ifdef SCHED_MULTITHREADED
if (schedule_action(__send_lagrq, data))
#endif
@@ -2053,6 +2068,16 @@
return res;
}
+/* Call AST_SCHED_DEL on a scheduled task if it is found in scheduler. */
+static int iax2_delete_from_sched(const void* data)
+{
+ int sched_id = (int)(long)data;
+
+ AST_SCHED_DEL(sched, sched_id);
+
+ return 0;
+}
+
/*!\note Assumes the lock on the pvt is already held, when
* iax2_destroy_helper() is called. */
static void iax2_destroy_helper(struct chan_iax2_pvt *pvt)
@@ -2069,11 +2094,27 @@
ast_clear_flag64(pvt, IAX_MAXAUTHREQ);
}
- /* No more pings or lagrq's */
- AST_SCHED_DEL_SPINLOCK(sched, pvt->pingid, &iaxsl[pvt->callno]);
- pvt->pingid = DONT_RESCHEDULE;
- AST_SCHED_DEL_SPINLOCK(sched, pvt->lagid, &iaxsl[pvt->callno]);
- pvt->lagid = DONT_RESCHEDULE;
+
+
+ /* Mark call destroy initiated flag. */
+ pvt->destroy_initiated = 1;
+
+ /*
+ * Schedule deleting the scheduled (but didn't run yet) PINGs or LAGRQs.
+ * Already running tasks will be terminated because of destroy_initiated.
+ *
+ * Don't call AST_SCHED_DEL from this thread for pingid and lagid because
+ * it leads to a deadlock between the scheduler thread callback locking
+ * the callno mutex and this thread which holds the callno mutex one or
+ * more times. It is better to have another thread delete the scheduled
+ * callbacks which doesn't lock the callno mutex.
+ */
+ iax2_sched_add(sched, 0, iax2_delete_from_sched, (void*)(long)pvt->pingid);
+ iax2_sched_add(sched, 0, iax2_delete_from_sched, (void*)(long)pvt->lagid);
+
+ pvt->pingid = -1;
+ pvt->lagid = -1;
+
AST_SCHED_DEL(sched, pvt->autoid);
AST_SCHED_DEL(sched, pvt->authid);
AST_SCHED_DEL(sched, pvt->initid);
diff --git a/main/sched.c b/main/sched.c
index 8fdbfed..4443497 100644
--- a/main/sched.c
+++ b/main/sched.c
@@ -513,16 +513,8 @@
if (!s && *last_id != id) {
ast_debug(1, "Attempted to delete nonexistent schedule entry %d!\n", id);
-#ifndef AST_DEVMODE
- ast_assert(s != NULL);
-#else
- {
- char buf[100];
-
- snprintf(buf, sizeof(buf), "s != NULL, id=%d", id);
- _ast_assert(0, buf, file, line, function);
- }
-#endif
+ /* Removing nonexistent schedule entry shouldn't trigger assert (it was enabled in DEV_MODE);
+ * because in many places entries is deleted without having valid id. */
*last_id = id;
return -1;
} else if (!s) {
--
To view, visit https://gerrit.asterisk.org/621
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I03bec1fc8faacb89630269e935fa667c6d6c080c
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
More information about the asterisk-commits
mailing list