Re: [PATCH net] sctp: sctp should release assoc when sctp_make_abort_user return NULL in sctp_close

[Vlad Yasevich <vyasevich@gmail.com>]

On 12/17/2015 02:33 PM, Vlad Yasevich wrote:
> On 12/17/2015 02:01 PM, Marcelo Ricardo Leitner wrote:
>> Em 17-12-2015 16:29, Vlad Yasevich escreveu:
>>> On 12/17/2015 09:30 AM, Xin Long wrote:
>>>> In sctp_close, sctp_make_abort_user may return NULL because of memory
>>>> allocation failure. If this happens, it will bypass any state change
>>>> and never free the assoc. The assoc has no chance to be freed and it
>>>> will be kept in memory with the state it had even after the socket is
>>>> closed by sctp_close().
>>>>
>>>> So if sctp_make_abort_user fails to allocate memory, we should just
>>>> free the asoc, as there isn't much else that we can do.
>>>>
>>>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>>>> Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
>>>> ---
>>>>   net/sctp/socket.c | 6 +++++-
>>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>>> index 9b6cc6d..267b8f8 100644
>>>> --- a/net/sctp/socket.c
>>>> +++ b/net/sctp/socket.c
>>>> @@ -1513,8 +1513,12 @@ static void sctp_close(struct sock *sk, long timeout)
>>>>               struct sctp_chunk *chunk;
>>>>
>>>>               chunk = sctp_make_abort_user(asoc, NULL, 0);
>>>> -            if (chunk)
>>>> +            if (chunk) {
>>>>                   sctp_primitive_ABORT(net, asoc, chunk);
>>>> +            } else {
>>>> +                sctp_unhash_established(asoc);
>>>> +                sctp_association_free(asoc);
>>>> +            }
>>>
>>> I don't think you can do that for an association that has not been closed.
>>>
>>> I think a cleaner approach might be to update abort primitive handlers
>>> to handle a NULL chunk value and unconditionally call the primitive.
>>>
>>> This guarantees that any timers or waitqueues that might be active are
>>> stopped correctly.
>>
>> sctp_association_free() is the one who does that job, even that way. All in between the
>> primitive call and then the call to sctp_association_free() is just status changes and
>> packet xmit, which doing this way we cut out when we are in memory pressure. pkt xmit or
>> ULP events are likely going to fail too anyway.
>>
>> sctp_sf_do_9_1_prm_abort() -> SCTP_CMD_ASSOC_FAILED ->
>>   sctp_cmd_assoc_failed -> ULP events, send abort, and SCTP_CMD_DELETE_TCB ->
>>     sctp_cmd_delete_tcb ->
>>       sctp_unhash_established(asoc);
>>       sctp_association_free(asoc);
>> and returns.
>>
>> There is a check on sctp_cmd_delete_tcb() that avoids calling that on temp assocs on
>> listening sockets, but that condition is false due to the check on sk_shutdown so it will
>> call those two functions anyway.
> 
> The condition I am a bit concerned about is one thread waiting in sctp_wait_for_sndbuf
> while another does an abort.
> 
> I think this is OK though.  I need to look a bit more...

I think the only time this ends up biting us is if SO_SNDTIMEO was used and we ran out
of send buffer.  It looks to me like schedule_timeout() will wait until timer expired and
depending on the timer value, you could wait quite a while.

With this path, since you don't transition state, the asoc->wait wait queue is never
notified and it could be hanging around for quite a while.

-vlad	

> 
> -vlad
> 
> 
>>
>>   Marcelo
>>
>