Hi Partha, it's very luck that you have a chance to review the series before they are submitted to mainline.
Please take a look at my comments below. On 02/23/2017 08:10 PM, Parthasarathy Bhuvaragan wrote: > Hi Ying, > > > Thanks for the patches. I introduced these bugs and was sick for more > than 2 weeks, so you had to fix them. > > > I have few comments for the entire series. > > [tipc-discussion] [net 2/5] tipc: adjust the policy of holding > subscription kref > <https://sourceforge.net/p/tipc/mailman/message/35676908/>*<https://sourceforge.net/p/tipc/mailman/message/35676908/>* > > The above commit looks good and should fix the issue reported by John. > > > [tipc-discussion] [net 1/5] tipc: advance the time of deleting > subscription from subscriber->subscrp_list > <https://sourceforge.net/p/tipc/mailman/message/35676906/> > > In the above commit, we miss to delete sub->subscrp_list at subscription > timeout. We placed this common code in refcount cleanup to avoid code > duplication. > > > [tipc-discussion] [net 3/5] tipc: adjust policy that sub->timer holds > subscription kref > <https://sourceforge.net/p/tipc/mailman/message/35676904/>*<https://sourceforge.net/p/tipc/mailman/message/35676904/>* > > With this patch, we will never trigger a subscription cleanup at > tipc_subscrp_timeout() as the refcount will be one when we > exittipc_subscrp_timeout(). When I made the changes, in fact I was aware of the case, but I supposed that user should explicitly cancel subscription with TIPC_SUB_CANCEL to destroy subscription instance. However, I was unaware that new subscription events still happen e even if they are expired. I understood your proposal. But as you said, the approach looks very odd for us. However, I have another idea to a bit elegantly fix the issue while we don't need to make a big change for the whole series. Please wait for v2. Thanks, Ying > > Thus the subscription is still present in nametable and hence the > subscriber will receive new events for this subscription after receiving > the subscription timeout until the subscriber terminates. > > We need to update our design policy that at subscription timeout, we > remove all references to the subscription and it is no longer > accessible. This conflicts with the change proposed by the above commit. > > > If we still need to include this commit, we need to adapt > tipc_subscrp_timeout(), as: > > 1. tipc_nametbl_unsubscribe() > 2. tipc_subscrp_put() > 3. tipc_subscrp_put() > > static void tipc_subscrp_delete(struct tipc_subscription *sub) > { > u32 timeout = htohl(sub->evt.s.timeout, sub->swap); > > - if (timeout == TIPC_WAIT_FOREVER || del_timer(&sub->timer)) > + if (timeout != TIPC_WAIT_FOREVER && del_timer(&sub->timer)) { > tipc_subscrp_put(sub); > + tipc_subscrp_put(sub); > } else { > + tipc_subscrp_put(sub); > } > } > > But this makes the functions look odd as we adjust refcount twice in the > same function. To get the subscription timeout cleanup working > correctly, we need to sqash this commit together :*[tipc-discussion] > [net 4/5] tipc: advance the time of calling tipc_nametbl_unsubscribe > <https://sourceforge.net/p/tipc/mailman/message/35676905/>* > > *[tipc-discussion] [net 5/5] tipc: remove unnecessary increasement of > subscription refcount > <https://sourceforge.net/p/tipc/mailman/message/35676917/>* > > In this commit message, you need to edit the stacktraces as the scenario > you describe will not happen due to your previous patches. As we never > perform a tipc_subscrp_get() in tipc_subscrp_report_overlap(). > > > /Partha > ------------------------------------------------------------------------ > *From:* John Thompson <[email protected]> > *Sent:* Tuesday, February 21, 2017 10:15 PM > *To:* Ying Xue > *Cc:* Jon Maloy; Parthasarathy Bhuvaragan; > [email protected] > *Subject:* Re: [net 5/5] tipc: remove unnecessary increasement of > subscription refcount > > 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] > <mailto:[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]> > > <mailto:[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]> > > <mailto:[email protected] <mailto:[email protected]>>> > > Reported-by: Jon Maloy <[email protected] > <mailto:[email protected]> > > <mailto:[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]> > > <mailto:[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
