Re: [PATCH net-next 4/5] net: sctp: decouple cleaning socket data from endpoint

[Daniel Borkmann <dborkman@redhat.com>]

On 06/21/2013 03:12 AM, Vlad Yasevich wrote:
> On 06/20/2013 01:29 PM, Daniel Borkmann wrote:
>> On 06/20/2013 04:35 PM, Vlad Yasevich wrote:
>>> On 06/18/2013 04:55 AM, Daniel Borkmann wrote:
>>>> Rather instead of having the endpoint clean the garbage for the
>>>> socket, use a sk_destruct handler sctp_destruct_sock(), that does
>>>> the job for that when there are no more references on the socket.
>>>
>>> With this patch it is possible to run sctp_put_port while the socket
>>> is not locked.
>>>
>>> The flow goes something like this:
>>>
>>> sctp_close()
>>>    sk_bh_lock_sock();
>>>    sk_common_release()
>>>      sctp_destroy_sock()
>>>        endpoint_put()
>>>          endpoint_destroy() <-- This is where we used to do sctp_put_port
>>>    sk_bh_unlock_sock();
>>>    sock_put()
>>>      sk_free()
>>>        __sk_free()
>>>          sctp_destruct_sock()
>>>            sctp_put_port()
>>>
>>> I haven't found any race conditions yet, but that doesn't mean they
>>> don't exist.
>>>
>>> I think an easy solution would be to do sctp_put_port in sctp_unhash(),
>>> but I haven't traced all the paths.
>>
>> Hm, compared to the current (pre-patch) solution, sctp_put_port() does not
>> necessarily need to be called at sk_common_release() time if refs are still
>> on the endpoint, so that endpoint_destroy() is further deferred in time.
>> Thus,
>> if we would do the sctp_put_port() in sctp_unhash(), we could free it at an
>> earlier time than with endpoint_destroy(). This does not necessarily
>> need to
>> be a bad or wrong way, but with the current approach it's done at an later
>> point in time afaik.
>
> You are right. Doing it in sctp_unhash() could be possibly too early.
>
>> If it's only about the locking, what if we just hold
>> that socket lock around sctp_put_port() in the current patch?
>>
>> But besides that, if at such a late point in time someone still has
>> access to
>> that socket member (right before we do the kfree(sk)), we would be
>> pretty much
>> screwed. :-) Despite having the socket lock or not, the port hashtable
>> has it's
>> own protection from what I see.
>
> Yes it does and I've been looking to see if this is sufficient enough
> for our purposes.  It looks like our saving grace is the fact that
> we set the sk_state to CLOSED sctp_endpoint_free().  Otherwise, we'd
> have a race between sctp_endpoint_destroy() and conflict detection
> in sctp_get_port_local.  This seems a bit fragile and we are making
> it a bit so more with this patch.
>
> I think it would be better to see if we can remove the socket from
> the port table a bit earlier if possbile.

Ok, let me come back to this on Monday.