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 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);
>  	}
>