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...
-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
