netdev - Re: [PATCH v3 for-next 05/13] IB/cm: Reference count ib_cm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <55544523.3090003@mellanox.com>
Date:	Thu, 14 May 2015 09:48:03 +0300
From:	Haggai Eran <haggaie@...lanox.com>
To:	Jason Gunthorpe <jgunthorpe@...idianresearch.com>
CC:	Doug Ledford <dledford@...hat.com>, <linux-rdma@...r.kernel.org>,
	<netdev@...r.kernel.org>, Liran Liss <liranl@...lanox.com>,
	Guy Shapiro <guysh@...lanox.com>,
	Shachar Raindel <raindel@...lanox.com>,
	Yotam Kenneth <yotamke@...lanox.com>
Subject: Re: [PATCH v3 for-next 05/13] IB/cm: Reference count ib_cm_ids

On 13/05/2015 19:58, Jason Gunthorpe wrote:
> On Wed, May 13, 2015 at 01:20:22PM +0300, Haggai Eran wrote:
...
>>> Early exit
>>> from cm_destroy_id when the there are still people listening.
>>>
>>> That sounds like it keeps the basic rule of cm_destroy_id being
>>> properly paired with the alloc, and allows listen sharing without the
>>> confusion of what does multiple destroy mean.
>>
>> Won't you find it confusing if a call to ib_destroy_cm_id is successful
>> but the id actually continues to live?
> 
> No.. As the caller, I don't care what the cm layer is doing behind the scenes.
> 
> The lifetime if each cm_id is clearly defined:
> 
> cm_create_cm_id()
> cm_ref_id() / cm_deref_id()
> cm_destroy_id()
> 
> The fact the CM might share a listen (and only a listen) ID behind the
> scenes is not the caller's problem. That is an implementation choice,
> each caller stands alone and uses the API properly, assuming it is the
> only user of the returned cm_id.
I guess that's okay. Previously, the caller relied on context
information from the CM ID to hold its own context. Now it is no longer
allowed to do that, because the listener is now shared. So With this
change in place you could argue that the caller doesn't care if the CM
ID is actually shared or not.

> 
> The caller relies on cm_destroy_id being synchronous.
> 
>> I prefer the API to explicitly show that you are only decreasing the
>> reference count and that the id might not be destroyed immediately.
> 
> No, that is fuzzy thinking, and lead to this patch that tried to have
> both a ib_cm_id_put and cm_destroy_id being used at once. That is broken.
> 
> As discussed, we can't easily convert all call sites of cm_destroy_id
> to ib_cm_id_put because 1) We loose the error code and 2) The put_X
> idiom is async while cm_destory_id users expect sync.
> 
> So the best choice is to retain the cm_create_cm_id()/cm_destroy_id()
> pairing, and have cm_destroy_id 'do the right thing' when presented
> with a shared listen to destroy -> decrease the share count and free
> the underlying listen when no more shares are left.

Okay.

> That said: Are you sure this is going to work? Are all the listen
> specific cases of cm_destroy_id OK with not doing the
> wait_for_completion and cm_dqueue_work *for stuff related to that
> client* ?

As far as I can see, a listening ib_cm_id state's can be either IDLE or
LISTEN. In these states cm_destroy_id doesn't send anything to the wire.
As for the dequeue work, I expect the queue to remain intact if the CM
ID is still used. If there are work items in the queue that would have
belonged to the particular RDMA CM ID being destroyed, then the RDMA CM
ID module will have to reject them by itself.

Haggai
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html