Sorry, I was mistaken. You are right that you have removed the sub->lock from tipc_subscrp_kref_release() and so there is no chance of a deadlock.
JT On Tue, Feb 21, 2017 at 11:22 PM, Ying Xue <[email protected]> wrote: > On 02/21/2017 06:13 AM, John Thompson wrote: > > > > > > On Tue, Feb 21, 2017 at 12:39 AM, Ying Xue <[email protected] > > <mailto:[email protected]>> 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 <[email protected] > > <mailto:[email protected]>> > > Reported-by: Jon Maloy <[email protected] > > <mailto:[email protected]>> > > Fixes: d094c4d5f5 ("tipc: add subscription refcount to avoid invalid > > delete") > > Signed-off-by: Ying Xue <[email protected] > > <mailto:[email protected]>> > > --- > > 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? > > > > I think there is no deadlock risk even if tipc_subscrp_put() will be > called twice in tipc_subscrp_delete() because > tipc_subscrp_kref_release() doesn't hold subscriber->lock after the change. > > Please take a look at tipc_subscrp_kref_release() code: > static void tipc_subscrp_kref_release(struct kref *kref) > { > struct tipc_subscription *sub = container_of(kref, > struct > tipc_subscription, > kref); > struct tipc_net *tn = net_generic(sub->net, tipc_net_id); > struct tipc_subscriber *subscriber = sub->subscriber; > > atomic_dec(&tn->subscription_count); > kfree(sub); > tipc_subscrb_put(subscriber); > } > > > > > > > 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 [email protected] https://lists.sourceforge.net/lists/listinfo/tipc-discussion
