[Asterisk-code-review] sig_pri: Fix deadlock caused by sig_pri_queue_hangup (asterisk[13])

[George Joseph]

George Joseph has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/13505 )


Change subject: sig_pri:  Fix deadlock caused by sig_pri_queue_hangup
......................................................................

sig_pri:  Fix deadlock caused by sig_pri_queue_hangup

The change to add setting hangupsource to sig_pri_queue_hangup()
made in https://gerrit.asterisk.org/c/asterisk/+/12857 casued
deadlocks when a hangup request was received from the core at the
same time a hanguprequest was received from the remote end via the
D channel.  This was caused by using the incorrect locking model of
which there are 2.  One for core-initiated requests and one for D
channel initiated requests.  sig_pri_queue_hangup() is only ever
called by the d-channel thread but the core model was being used.

ast_set_hangupsource() was being called after the channel was being
unlocked and even though it does relock the channel itself (twice),
the gaps in the locks allowed the core-initiated hangup to get in.
However, the locking model for core-initiated and
dchannel-initiated requests is inverted with respect to channels
(core gets channel first, dchannel gets channel last).  Combined
with the fact that both request types also try to get the pri's
lock and the pri's private channel structure lock and the fact
that sig_pri_queue_hangup() was actually unlocking the pri's private
channel structure lock, then relocking it again after calling
ast_set_hangupsource(), this results in massive deadlocks that can
escalate to the entire DAHDI subsystem via the global DAHDI
interface lock.

The fix is actually easy.  We no longer unlock the pri's private
channel structure before calling ast_set_hangupsource() and relock
it afterwards, and we also now hold the channel lock until after
ast_set_hangupsource() has finished.

ASTERISK-28605
Reported by: Dirk Wendland

Change-Id: Id74aaa5d4e3746063dbe9deed188eb65193cb9c9
---
M channels/sig_pri.c
1 file changed, 1 insertion(+), 3 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/05/13505/1

diff --git a/channels/sig_pri.c b/channels/sig_pri.c
index 90dc051..52fef84 100644
--- a/channels/sig_pri.c
+++ b/channels/sig_pri.c
@@ -1402,13 +1402,11 @@
 	if (owner) {
 		ao2_ref(owner, +1);
 		ast_queue_hangup(owner);
-		ast_channel_unlock(owner);
 
 		/* Tell the CDR this DAHDI channel hung up */
-		sig_pri_unlock_private(pri->pvts[chanpos]);
 		ast_set_hangupsource(owner, ast_channel_name(owner), 0);
-		sig_pri_lock_private(pri->pvts[chanpos]);
 
+		ast_channel_unlock(owner);
 		ao2_ref(owner, -1);
 	}
 }

-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/13505
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Change-Id: Id74aaa5d4e3746063dbe9deed188eb65193cb9c9
Gerrit-Change-Number: 13505
Gerrit-PatchSet: 1
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20191230/1d10ac23/attachment.html>