On 02/21/2017 06:13 AM, John Thompson wrote:
>
>
> On Tue, Feb 21, 2017 at 12:39 AM, Ying Xue <ying....@windriver.com
> <mailto: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
> <mailto:thompa....@gmail.com>>
> Reported-by: Jon Maloy <jon.ma...@ericsson.com
> <mailto:jon.ma...@ericsson.com>>
> Fixes: d094c4d5f5 ("tipc: add subscription refcount to avoid invalid
> delete")
> Signed-off-by: Ying Xue <ying....@windriver.com
> <mailto: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?
>
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
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
