[asterisk-dev] [Code Review] Deadlock fixes for chan_local.c

[Russell Bryant]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/631/#review1921
-----------------------------------------------------------

Ship it!


- Russell


On 2010-04-28 15:12:55, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/631/
> -----------------------------------------------------------
> 
> (Updated 2010-04-28 15:12:55)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> ISSUE_1:
> 
> In the local_hangup() 3 locks must be held at the same time... pvt, pvt->chan, and pvt->owner.  Proper deadlock avoidance is done when the channel to hangup is the outbound chan_local channel, but when it is not the outbound channel we have an issue... We attempt to do deadlock avoidance only on the tech pvt, when both the tech pvt and the pvt->owner are locked coming into that loop.  By never giving up the pvt->owner channel deadlock avoidance is not entirely possible.
> 
> PROBLEM: get pvt->chan lock
> LOCKS HELD: pvt, pvt->owner
> LOCKS WE WANT: pvt, pvt->owner, pvt->chan
> 
> CURRENT LOGIC: We attempt to lock pvt->chan without ever giving up the pvt->owner lock during deadlock avoidance.
> 
> while (trylock(pvt->chan))
>     unlock(pvt)
>     sleep(1)
>     lock(pvt)
> 
> NEW LOGIC:  Clear all locks during deadlock avoidance, adding back the locks in the proper locking order.
> 
> while (trylock(pvt->chan))
>     unlock(pvt)
>     unlock(pvt->owner)
>     sleep(1)
>     lock(pvt->owner)
>     lock(pvt)
> 
> ISSUE_2:
> 
> ast_prod() is used in ast_activate_generator() to queue a frame on the channel and make the channel's read function get called.  This function is used in ast_activate_generator() while the channel is locked, which mean's the channel will have a lock both from the generator code and the frame_queue code by the time it gets to chan_local.c's local_queue_frame code... local_queue_frame contains some of the same crazy deadlock avoidance that local_hangup requires, and this recursive lock prevents that deadlock avoidance from happening correctly.
>  
> 
> 
> This addresses bug 17185.
>     https://issues.asterisk.org/view.php?id=17185
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/channels/chan_local.c 259663 
>   /branches/1.4/main/channel.c 259663 
> 
> Diff: https://reviewboard.asterisk.org/r/631/diff
> 
> 
> Testing
> -------
> 
> tested on reporter's system.  deadlock issue did not occur anymore.
> 
> 
> Thanks,
> 
> David
> 
>