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@xxxxxxxxx>
>>>> Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@xxxxxxxxx>
>>>> ---
>>>> 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
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
