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

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

[Xin Long]

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);
+			}
 		} else
 			sctp_primitive_SHUTDOWN(net, asoc, NULL);
 	}
-- 
2.1.0

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

[Vlad Yasevich]

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.

-vlad


>  		} else
>  			sctp_primitive_SHUTDOWN(net, asoc, NULL);
>  	}
> 

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

[Marcelo Ricardo Leitner]

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.

   Marcelo

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

[Vlad Yasevich]

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

-vlad


> 
>   Marcelo
> 

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

[Vlad Yasevich]

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

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

[Marcelo Ricardo Leitner]

On Fri, Dec 18, 2015 at 09:08:46AM -0500, Vlad Yasevich wrote:
> On 12/17/2015 02:33 PM, Vlad Yasevich wrote:
> > On 12/17/2015 02:01 PM, Marcelo Ricardo Leitner wrote:
...
> >> 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.

Yes, agreed. For blocking sockets, it could hang waiting until the
application finally closes. Thanks Vlad.

  Marcelo

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

[Xin Long]

On Sat, Dec 19, 2015 at 12:23 AM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> On Fri, Dec 18, 2015 at 09:08:46AM -0500, Vlad Yasevich wrote:
>> On 12/17/2015 02:33 PM, Vlad Yasevich wrote:
>> > On 12/17/2015 02:01 PM, Marcelo Ricardo Leitner wrote:
> ...
>> >> 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.

do you think it makes sense if we have this condition judgment there ?
        if (waitqueue_active(&asoc->wait))
            wake_up_interruptible(&asoc->wait);


>
> Yes, agreed. For blocking sockets, it could hang waiting until the
> application finally closes. Thanks Vlad.
>
>   Marcelo
>

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

[Marcelo Ricardo Leitner]

Em 21-12-2015 07:56, Xin Long escreveu:
> On Sat, Dec 19, 2015 at 12:23 AM, Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
>> On Fri, Dec 18, 2015 at 09:08:46AM -0500, Vlad Yasevich wrote:
>>> On 12/17/2015 02:33 PM, Vlad Yasevich wrote:
>>>> On 12/17/2015 02:01 PM, Marcelo Ricardo Leitner wrote:
>> ...
>>>>> 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.
>
> do you think it makes sense if we have this condition judgment there ?
>          if (waitqueue_active(&asoc->wait))
>              wake_up_interruptible(&asoc->wait);

No, because later if there is something else like this that we need to 
handle on this situation, we will have to update both places and we may 
forget to update one of them. It's better to just skip the packet 
transmission/CMD_REPLY if chunk is NULL and let rest execute, as Vlad 
suggested.

It will also be better for troubleshooting, as it may generate debug 
msgs about the state transition.

   Marcelo