sctp: ABORT if receive queue is not empty while closing socket

On Wed, Jun 29, 2011 at 12:14:41PM -0400, Vladislav Yasevich wrote:
> Right.  The lack of ABORT from the receive of data is a bug.  I was trying to point out
> that instead of modified the sender of data to send the ABORT, you modify the receiver
> to send the ABORT when it is being closed while having data queued.

Is this what you had in mind?

Trigger user ABORT when a socket is closed which has skbs sitting on
the receive queue. If data was lost, there is no point in doing a
graceful shutdown. This is consistent with TCP behaviour.

This also resolves the situation when a receiver cannot reopen its rwnd
and the sender continues retransmission attempts indefinitely before
initiating the shutdown.

Signed-off-by: Thomas Graf <tgraf@infradead.org>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 06/30/2011 09:31 AM, Thomas Graf wrote:
> On Wed, Jun 29, 2011 at 12:14:41PM -0400, Vladislav Yasevich wrote:
>> Right.  The lack of ABORT from the receive of data is a bug.  I was trying to point out
>> that instead of modified the sender of data to send the ABORT, you modify the receiver
>> to send the ABORT when it is being closed while having data queued.
> 
> Is this what you had in mind?

Almost.  It could really be a simple true/false condition about recvqueue or inqueue
being non-empty.  If that's the case, trigger abort.

-vlad

> 
> Trigger user ABORT when a socket is closed which has skbs sitting on
> the receive queue. If data was lost, there is no point in doing a
> graceful shutdown. This is consistent with TCP behaviour.
> 
> This also resolves the situation when a receiver cannot reopen its rwnd
> and the sender continues retransmission attempts indefinitely before
> initiating the shutdown.
> 
> Signed-off-by: Thomas Graf <tgraf@infradead.org>
> 
> diff --git a/include/net/sctp/ulpevent.h b/include/net/sctp/ulpevent.h
> index 99b027b..ca4693b 100644
> --- a/include/net/sctp/ulpevent.h
> +++ b/include/net/sctp/ulpevent.h
> @@ -80,7 +80,7 @@ static inline struct sctp_ulpevent *sctp_skb2event(struct sk_buff *skb)
>  
>  void sctp_ulpevent_free(struct sctp_ulpevent *);
>  int sctp_ulpevent_is_notification(const struct sctp_ulpevent *);
> -void sctp_queue_purge_ulpevents(struct sk_buff_head *list);
> +unsigned int sctp_queue_purge_ulpevents(struct sk_buff_head *list);
>  
>  struct sctp_ulpevent *sctp_ulpevent_make_assoc_change(
>  	const struct sctp_association *asoc,
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 6766913..958253a 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -1384,6 +1384,7 @@ SCTP_STATIC void sctp_close(struct sock *sk, long timeout)
>  	struct sctp_endpoint *ep;
>  	struct sctp_association *asoc;
>  	struct list_head *pos, *temp;
> +	unsigned int data_was_unread;
>  
>  	SCTP_DEBUG_PRINTK("sctp_close(sk: 0x%p, timeout:%ld)\n", sk, timeout);
>  
> @@ -1393,6 +1394,10 @@ SCTP_STATIC void sctp_close(struct sock *sk, long timeout)
>  
>  	ep = sctp_sk(sk)->ep;
>  
> +	/* Clean up any skbs sitting on the receive queue.  */
> +	data_was_unread = sctp_queue_purge_ulpevents(&sk->sk_receive_queue);
> +	data_was_unread += sctp_queue_purge_ulpevents(&sctp_sk(sk)->pd_lobby);
> +
>  	/* Walk all associations on an endpoint.  */
>  	list_for_each_safe(pos, temp, &ep->asocs) {
>  		asoc = list_entry(pos, struct sctp_association, asocs);
> @@ -1410,7 +1415,8 @@ SCTP_STATIC void sctp_close(struct sock *sk, long timeout)
>  			}
>  		}
>  
> -		if (sock_flag(sk, SOCK_LINGER) && !sk->sk_lingertime) {
> +		if (data_was_unread ||
> +		    (sock_flag(sk, SOCK_LINGER) && !sk->sk_lingertime)) {
>  			struct sctp_chunk *chunk;
>  
>  			chunk = sctp_make_abort_user(asoc, NULL, 0);
> @@ -1420,10 +1426,6 @@ SCTP_STATIC void sctp_close(struct sock *sk, long timeout)
>  			sctp_primitive_SHUTDOWN(asoc, NULL);
>  	}
>  
> -	/* Clean up any skbs sitting on the receive queue.  */
> -	sctp_queue_purge_ulpevents(&sk->sk_receive_queue);
> -	sctp_queue_purge_ulpevents(&sctp_sk(sk)->pd_lobby);
> -
>  	/* On a TCP-style socket, block for at most linger_time if set. */
>  	if (sctp_style(sk, TCP) && timeout)
>  		sctp_wait_for_close(sk, timeout);
> diff --git a/net/sctp/ulpevent.c b/net/sctp/ulpevent.c
> index e70e5fc..aab3184 100644
> --- a/net/sctp/ulpevent.c
> +++ b/net/sctp/ulpevent.c
> @@ -1081,9 +1081,19 @@ void sctp_ulpevent_free(struct sctp_ulpevent *event)
>  }
>  
>  /* Purge the skb lists holding ulpevents. */
> -void sctp_queue_purge_ulpevents(struct sk_buff_head *list)
> +unsigned int sctp_queue_purge_ulpevents(struct sk_buff_head *list)
>  {
>  	struct sk_buff *skb;
> -	while ((skb = skb_dequeue(list)) != NULL)
> +	unsigned int data_unread = 0;
> +
> +	while ((skb = skb_dequeue(list)) != NULL) {
> +		struct sctp_ulpevent *event = sctp_skb2event(skb);
> +
> +		if (!sctp_ulpevent_is_notification(event))
> +			data_unread += skb->len;
> +
>  		sctp_ulpevent_free(sctp_skb2event(skb));
> +	}
> +
> +	return data_unread;
>  }
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Jun 30, 2011 at 10:11:06AM -0400, Vladislav Yasevich wrote:
> On 06/30/2011 09:31 AM, Thomas Graf wrote:
> > On Wed, Jun 29, 2011 at 12:14:41PM -0400, Vladislav Yasevich wrote:
> >> Right.  The lack of ABORT from the receive of data is a bug.  I was trying to point out
> >> that instead of modified the sender of data to send the ABORT, you modify the receiver
> >> to send the ABORT when it is being closed while having data queued.
> > 
> > Is this what you had in mind?
> 
> Almost.  It could really be a simple true/false condition about recvqueue or inqueue
> being non-empty.  If that's the case, trigger abort.

What would be the advantage of that?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 06/30/2011 12:19 PM, Thomas Graf wrote:
> On Thu, Jun 30, 2011 at 10:11:06AM -0400, Vladislav Yasevich wrote:
>> On 06/30/2011 09:31 AM, Thomas Graf wrote:
>>> On Wed, Jun 29, 2011 at 12:14:41PM -0400, Vladislav Yasevich wrote:
>>>> Right.  The lack of ABORT from the receive of data is a bug.  I was trying to point out
>>>> that instead of modified the sender of data to send the ABORT, you modify the receiver
>>>> to send the ABORT when it is being closed while having data queued.
>>>
>>> Is this what you had in mind?
>>
>> Almost.  It could really be a simple true/false condition about recvqueue or inqueue
>> being non-empty.  If that's the case, trigger abort.
> 
> What would be the advantage of that?
> 

Wrt to true/false, it's simpler to test for non-empty then it is to go through and count
the data (but I perfectly ok with either way).  WRT to testing the inqueue, as you stated,
not everything may be in receive queue.

-vlad
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html