Re: [tipc-discussion] [net 5/5] tipc: remove unnecessary increasement of subscription refcount

[John T. <tho...@gm...>]

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 <yin...@wi...> wrote:

> On 02/21/2017 06:13 AM, John Thompson wrote:
> >
> >
> > On Tue, Feb 21, 2017 at 12:39 AM, Ying Xue <yin...@wi...
> > <mailto:yin...@wi...>> 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 <tho...@gm...
> >     <mailto:tho...@gm...>>
> >     Reported-by: Jon Maloy <jon...@er...
> >     <mailto:jon...@er...>>
> >     Fixes: d094c4d5f5 ("tipc: add subscription refcount to avoid invalid
> >     delete")
> >     Signed-off-by: Ying Xue <yin...@wi...
> >     <mailto:yin...@wi...>>
> >     ---
> >      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
> >
> >
>