Hi Ying,
Sorry I sent that reply before signing it off.
The rest of the changes look like they are doing the right things but this
change looks like it has the potential to cause a deadlock.
Regards,
John
On Tue, Feb 21, 2017 at 11:13 AM, John Thompson <thompa....@gmail.com>
wrote:
>
>
> On Tue, Feb 21, 2017 at 12:39 AM, Ying Xue <ying....@windriver.com> wrote:
>
>> As the policy of holding subscription in subscriber time has been
>> adjusted, it's unnecessary to hold subscription refcount before
>> tipc_subscrp_delete() is called. As a consequence, subscriber->lock
>> can be safely removed to avoid the following two deadlock scenarios:
>>
>> CPU1: CPU2:
>> ---------- ----------------
>> tipc_nametbl_publish
>> spin_lock_bh(&tn->nametbl_lock)
>> tipc_nametbl_insert_publ
>> tipc_nameseq_insert_publ
>> tipc_subscrp_report_overlap
>> tipc_subscrp_get
>> tipc_subscrp_send_event
>> tipc_close_conn
>> tipc_subscrb_release_cb
>> tipc_subscrb_delete
>> tipc_subscrp_put
>> tipc_subscrp_put
>> tipc_subscrp_kref_release
>> tipc_nametbl_unsubscribe
>> spin_lock_bh(&tn->nametbl_lock)
>> <<grab nametbl_lock again>>
>>
>> CPU1: CPU2:
>> ---------- ----------------
>> tipc_nametbl_stop
>> spin_lock_bh(&tn->nametbl_lock)
>> tipc_purge_publications
>> tipc_nameseq_remove_publ
>> tipc_subscrp_report_overlap
>> tipc_subscrp_get
>> tipc_subscrp_send_event
>> tipc_close_conn
>> tipc_subscrb_release_cb
>> tipc_subscrb_delete
>> tipc_subscrp_put
>> tipc_subscrp_put
>> tipc_subscrp_kref_release
>> tipc_nametbl_unsubscribe
>> spin_lock_bh(&tn->nametbl_lock)
>> <<grab nametbl_lock again>>
>>
>> Reported-by: John Thompson <thompa....@gmail.com>
>> Reported-by: Jon Maloy <jon.ma...@ericsson.com>
>> Fixes: d094c4d5f5 ("tipc: add subscription refcount to avoid invalid
>> delete")
>> Signed-off-by: Ying Xue <ying....@windriver.com>
>> ---
>> net/tipc/subscr.c | 6 ------
>> 1 file changed, 6 deletions(-)
>>
>> diff --git a/net/tipc/subscr.c b/net/tipc/subscr.c
>> index 6624232..ea67cce 100644
>> --- a/net/tipc/subscr.c
>> +++ b/net/tipc/subscr.c
>> @@ -168,9 +168,7 @@ static void tipc_subscrp_kref_release(struct kref
>> *kref)
>> struct tipc_net *tn = net_generic(sub->net, tipc_net_id);
>> struct tipc_subscriber *subscriber = sub->subscriber;
>>
>> - spin_lock_bh(&subscriber->lock);
>> atomic_dec(&tn->subscription_count);
>> - spin_unlock_bh(&subscriber->lock);
>> kfree(sub);
>> tipc_subscrb_put(subscriber);
>> }
>> @@ -202,11 +200,7 @@ static void tipc_subscrb_subscrp_delete(struct
>> tipc_subscriber *subscriber,
>> list_del(&sub->subscrp_list);
>>
>> tipc_nametbl_unsubscribe(sub);
>> - tipc_subscrp_get(sub);
>> - spin_unlock_bh(&subscriber->lock);
>>
>
> By removing the unlocking at this point couldn't you end up with a
> deadlock on subscriber->lock
> due to tipc_subscrp_delete() putting the subscrp twice (which could end up
> with kref == 0) and
> as a result calling tipc_subscrp_kref_release() which gets
> subscriber->lock?
>
>
>
>> tipc_subscrp_delete(sub);
>> - tipc_subscrp_put(sub);
>> - spin_lock_bh(&subscriber->lock);
>>
>> if (s)
>> break;
>> --
>> 2.7.4
>>
>>
>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
