[PATCH] sctp: Enforce maximum retransmissions during shutdown

[PATCH] sctp: Enforce maximum retransmissions during shutdown

[Thomas Graf]

When initiating a graceful shutdown while having data chunks
on the retransmission queue with a peer which is in zero
window mode the shutdown is never completed because the
retransmission error count is reset periodically by the
following two rules:

 - Do not timeout association while doing zero window probe.
 - Reset overall error count when a heartbeat request has
   been acknowledged.

The graceful shutdown will wait for all outstanding TSN to be
acknowledged before sending the SHUTDOWN request. This never
happens due to the peer's zero window not acknowledging the
continuously retransmitted data chunks. Although the error
counter is incremented for each failed retransmission done
via the T3-rtx timer, the receiving of the SACK sent in return
to the retransmission, announcing the zero window, clears the
error count again immediately.

Also heartbeat requests continue to be sent periodically. The
peer acknowledges these requests causing the error counter to
be reset as well.

This patch changes behaviour to only reset the overall error
counter for the above rules while not in shutdown. This means
that if already queued data can't be transmitted in max_retrans
attempts we ABORT because a graceful shutdown is obviously not
possible.

The issue can be easily reproduced by establishing a sctp
association over the loopback device, constantly queueing data
at the sender while not reading any at the receiver.  Wait for
the window to reach zero, then initiate a shutdown by killing
both processes simultaneously. The association will never be
freed and the chunks on the retransmission queue will be
retransmitted indefinitely.

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

diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
index 1c88c89..14a5295 100644
--- a/net/sctp/outqueue.c
+++ b/net/sctp/outqueue.c
@@ -1629,10 +1629,15 @@ static void sctp_check_transmitted(struct sctp_outq *q,
 			 * A sender is doing zero window probing when the
 			 * receiver's advertised window is zero, and there is
 			 * only one data chunk in flight to the receiver.
+			 *
+			 * Allow the association to timeout if SHUTDOWN is
+			 * pending. We have no interest in keeping the
+			 * association around forever.
 			 */
 			if (!q->asoc->peer.rwnd &&
 			    !list_empty(&tlist) &&
-			    (sack_ctsn+2 == q->asoc->next_tsn)) {
+			    (sack_ctsn+2 == q->asoc->next_tsn) &&
+			    !(q->asoc->state >= SCTP_STATE_SHUTDOWN_PENDING)) {
 				SCTP_DEBUG_PRINTK("%s: SACK received for zero "
 						  "window probe: %u\n",
 						  __func__, sack_ctsn);
diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index 534c2e5..fa92f4d6 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c
@@ -670,10 +670,21 @@ static void sctp_cmd_transport_on(sctp_cmd_seq_t *cmds,
 	/* 8.3 Upon the receipt of the HEARTBEAT ACK, the sender of the
 	 * HEARTBEAT should clear the error counter of the destination
 	 * transport address to which the HEARTBEAT was sent.
-	 * The association's overall error count is also cleared.
 	 */
 	t->error_count = 0;
-	t->asoc->overall_error_count = 0;
+
+	/*
+	 * Although RFC2960 and RFC4460 specify that the overall error
+	 * count must be cleared when a HEARTBEAT ACK is received this
+	 * behaviour may prevent the maximum retransmission count from
+	 * being reached while in SHUTDOWN. If the peer keeps its window
+	 * closed not acknowledging any outstanding TSN we may rely on
+	 * reaching the max_retrans limit via the T3-rtx timer to close
+	 * the association which will never happen if the error count is
+	 * reset every heartbeat interval.
+	 */
+	if (!(t->asoc->state >= SCTP_STATE_SHUTDOWN_PENDING))
+		t->asoc->overall_error_count = 0;
 
 	/* Clear the hb_sent flag to signal that we had a good
 	 * acknowledgement.

Re: [PATCH] sctp: Enforce maximum retransmissions during shutdown

[Vladislav Yasevich]

On 06/29/2011 09:57 AM, Thomas Graf wrote:
> When initiating a graceful shutdown while having data chunks
> on the retransmission queue with a peer which is in zero
> window mode the shutdown is never completed because the
> retransmission error count is reset periodically by the
> following two rules:
> 
>  - Do not timeout association while doing zero window probe.
>  - Reset overall error count when a heartbeat request has
>    been acknowledged.
> 
> The graceful shutdown will wait for all outstanding TSN to be
> acknowledged before sending the SHUTDOWN request. This never
> happens due to the peer's zero window not acknowledging the
> continuously retransmitted data chunks. Although the error
> counter is incremented for each failed retransmission done
> via the T3-rtx timer, the receiving of the SACK sent in return
> to the retransmission, announcing the zero window, clears the
> error count again immediately.
> 
> Also heartbeat requests continue to be sent periodically. The
> peer acknowledges these requests causing the error counter to
> be reset as well.
> 
> This patch changes behaviour to only reset the overall error
> counter for the above rules while not in shutdown. This means
> that if already queued data can't be transmitted in max_retrans
> attempts we ABORT because a graceful shutdown is obviously not
> possible.
> 
> The issue can be easily reproduced by establishing a sctp
> association over the loopback device, constantly queueing data
> at the sender while not reading any at the receiver.  Wait for
> the window to reach zero, then initiate a shutdown by killing
> both processes simultaneously. The association will never be
> freed and the chunks on the retransmission queue will be
> retransmitted indefinitely.
> 

Hi Tomas

I think in this particular case, the receiver has to terminate, not the sender.
Look at how tcp_close() handles this.

As long as receiver is available, the sender should continue to try
sending data.

Once the receiver is no longer available (killed), if it had queued data, it should
trigger an ABORT since it lost data.  That ABORT will stop the sender as well.

-vlad

> Signed-off-by: Thomas Graf <tgraf@infradead.org>
> 
> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
> index 1c88c89..14a5295 100644
> --- a/net/sctp/outqueue.c
> +++ b/net/sctp/outqueue.c
> @@ -1629,10 +1629,15 @@ static void sctp_check_transmitted(struct sctp_outq *q,
>  			 * A sender is doing zero window probing when the
>  			 * receiver's advertised window is zero, and there is
>  			 * only one data chunk in flight to the receiver.
> +			 *
> +			 * Allow the association to timeout if SHUTDOWN is
> +			 * pending. We have no interest in keeping the
> +			 * association around forever.
>  			 */
>  			if (!q->asoc->peer.rwnd &&
>  			    !list_empty(&tlist) &&
> -			    (sack_ctsn+2 == q->asoc->next_tsn)) {
> +			    (sack_ctsn+2 == q->asoc->next_tsn) &&
> +			    !(q->asoc->state >= SCTP_STATE_SHUTDOWN_PENDING)) {
>  				SCTP_DEBUG_PRINTK("%s: SACK received for zero "
>  						  "window probe: %u\n",
>  						  __func__, sack_ctsn);
> diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> index 534c2e5..fa92f4d6 100644
> --- a/net/sctp/sm_sideeffect.c
> +++ b/net/sctp/sm_sideeffect.c
> @@ -670,10 +670,21 @@ static void sctp_cmd_transport_on(sctp_cmd_seq_t *cmds,
>  	/* 8.3 Upon the receipt of the HEARTBEAT ACK, the sender of the
>  	 * HEARTBEAT should clear the error counter of the destination
>  	 * transport address to which the HEARTBEAT was sent.
> -	 * The association's overall error count is also cleared.
>  	 */
>  	t->error_count = 0;
> -	t->asoc->overall_error_count = 0;
> +
> +	/*
> +	 * Although RFC2960 and RFC4460 specify that the overall error
> +	 * count must be cleared when a HEARTBEAT ACK is received this
> +	 * behaviour may prevent the maximum retransmission count from
> +	 * being reached while in SHUTDOWN. If the peer keeps its window
> +	 * closed not acknowledging any outstanding TSN we may rely on
> +	 * reaching the max_retrans limit via the T3-rtx timer to close
> +	 * the association which will never happen if the error count is
> +	 * reset every heartbeat interval.
> +	 */
> +	if (!(t->asoc->state >= SCTP_STATE_SHUTDOWN_PENDING))
> +		t->asoc->overall_error_count = 0;
>  
>  	/* Clear the hb_sent flag to signal that we had a good
>  	 * acknowledgement.
> 

Re: [PATCH] sctp: Enforce maximum retransmissions during shutdown

[Thomas Graf]

On Wed, Jun 29, 2011 at 10:20:01AM -0400, Vladislav Yasevich wrote:
> I think in this particular case, the receiver has to terminate, not the sender.
> Look at how tcp_close() handles this.
> 
> As long as receiver is available, the sender should continue to try
> sending data.

The receiver does not know that the sender wishes to shutdown the
association. No shutdown request has been sent yet.

I don't think we should be relying on the behaviour of the sender for
the receiver to be able to ever free its ressources. We will be
retransmitting data and keeping an association alive _forever_ for no
purpose.

If there is no reliable way of _ever_ doing a graceful shutdown then
the only alternative is to just ABORT in the first place.

The difference in TCP is that we can close the connection half-way,
something we can't do in sctp.

Re: [PATCH] sctp: Enforce maximum retransmissions during shutdown

[Vladislav Yasevich]

On 06/29/2011 10:36 AM, Thomas Graf wrote:
> On Wed, Jun 29, 2011 at 10:20:01AM -0400, Vladislav Yasevich wrote:
>> I think in this particular case, the receiver has to terminate, not the sender.
>> Look at how tcp_close() handles this.
>>
>> As long as receiver is available, the sender should continue to try
>> sending data.
> 
> The receiver does not know that the sender wishes to shutdown the
> association. No shutdown request has been sent yet.
> 
> I don't think we should be relying on the behaviour of the sender for
> the receiver to be able to ever free its ressources. We will be
> retransmitting data and keeping an association alive _forever_ for no
> purpose.
> 
> If there is no reliable way of _ever_ doing a graceful shutdown then
> the only alternative is to just ABORT in the first place.
> 
> The difference in TCP is that we can close the connection half-way,
> something we can't do in sctp.
> 

But what you are proposing violates the protocol.  Zero-window probes do
not count against max retransmissions, even when you are in shutdown pending
state.

You'll come out of this one of 2 ways:
  1) receiver wakes up and processes data.  This will allow for graceful termination.
  2) receiver dies.  Since receive window is full, we have data queued, and this will
     trigger an ABORT to be sent to the sender.

What you patch is doing is taking a perfectly valid scenario and putting a time limit
on it in violation of the spec.

-vlad

Re: [PATCH] sctp: Enforce maximum retransmissions during shutdown

[Thomas Graf]

On Wed, Jun 29, 2011 at 10:58:41AM -0400, Vladislav Yasevich wrote:
> But what you are proposing violates the protocol.  Zero-window probes do
> not count against max retransmissions, even when you are in shutdown pending
> state.
> 
> You'll come out of this one of 2 ways:
>   1) receiver wakes up and processes data.  This will allow for graceful termination.
>   2) receiver dies.  Since receive window is full, we have data queued, and this will
>      trigger an ABORT to be sent to the sender.

If by die you mean kill the process then this is exactly what I do to trigger
the issue. I simulatenously kill both processes. Not sure what you mean by
trigger an ABORT but I don't see that happen. I also don't see the rwnd reopen
after the socket is closed on the receiver side but that's a separate issue.

> What you patch is doing is taking a perfectly valid scenario and putting a time limit
> on it in violation of the spec.

We also violate the spec by not doing so. The spec says that the number of
SHUTDOWN retransmissions has to be limited by Max.Retrans which we also
can't enforce because of the above.

The scenario is closed sockets on both sides, endpoints on both sides gone
already and retransmissions + heartbeat requests forever.

Any alternative suggestion how to fix this?

Re: [PATCH] sctp: Enforce maximum retransmissions during shutdown

[Vladislav Yasevich]

On 06/29/2011 11:48 AM, Thomas Graf wrote:
> On Wed, Jun 29, 2011 at 10:58:41AM -0400, Vladislav Yasevich wrote:
>> But what you are proposing violates the protocol.  Zero-window probes do
>> not count against max retransmissions, even when you are in shutdown pending
>> state.
>>
>> You'll come out of this one of 2 ways:
>>   1) receiver wakes up and processes data.  This will allow for graceful termination.
>>   2) receiver dies.  Since receive window is full, we have data queued, and this will
>>      trigger an ABORT to be sent to the sender.
> 
> If by die you mean kill the process then this is exactly what I do to trigger
> the issue. I simulatenously kill both processes. Not sure what you mean by
> trigger an ABORT but I don't see that happen. I also don't see the rwnd reopen
> after the socket is closed on the receiver side but that's a separate issue.

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.

> 
>> What you patch is doing is taking a perfectly valid scenario and putting a time limit
>> on it in violation of the spec.
> 
> We also violate the spec by not doing so. The spec says that the number of
> SHUTDOWN retransmissions has to be limited by Max.Retrans which we also
> can't enforce because of the above.

But we don't even get to sending the SHUTDOWN, so from the wire protocol, we
do not violated it.  We have bad behavior in that when both sender and receiver
are dead, the association is hung.

> 
> The scenario is closed sockets on both sides, endpoints on both sides gone
> already and retransmissions + heartbeat requests forever.
> 
> Any alternative suggestion how to fix this?
> 

The receiver, on sctp_close() checks to see if it has queued data and if it does, the
ABORT is triggered.  This is the same behavior as tcp_close().

-vlad

Re: [PATCH] sctp: Enforce maximum retransmissions during shutdown

[Thomas Graf]

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.

Agreed. This makes a good procedure if there is data is on
sk_receive_queue and gets us in line with TCP although I don't see this
in the spec at all :-)

> But we don't even get to sending the SHUTDOWN, so from the wire protocol, we
> do not violated it.  We have bad behavior in that when both sender and receiver
> are dead, the association is hung.

So how do we get out if ...

1) there is nothing queued on sk_receive_queue but the window still
   remains 0 forver?
   
2) the receiver is an older Linux without the above fix or another stack
   that does not ABORT?

I agree that using ABORT on the receiver is the ideal way whenver
possible but we still need to fix this if the receiver does not do so.

What sideeffects are you worried about resulting from my proposal?

Re: [PATCH] sctp: Enforce maximum retransmissions during shutdown

[Vladislav Yasevich]

On 06/30/2011 04:49 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.
> 
> Agreed. This makes a good procedure if there is data is on
> sk_receive_queue and gets us in line with TCP although I don't see this
> in the spec at all :-)
> 
>> But we don't even get to sending the SHUTDOWN, so from the wire protocol, we
>> do not violated it.  We have bad behavior in that when both sender and receiver
>> are dead, the association is hung.
> 
> So how do we get out if ...
> 
> 1) there is nothing queued on sk_receive_queue but the window still
>    remains 0 forver?

sk_receive_queue isn't the only queue you have to check.  You'll need to check
the reassembly and ordering queues, as partial or out of order things might stuck
there.  That's would be an extremely rare condition since if we ever get here, the
first thing we do is reneg on those TSN and open the window to get the missing chunk
in and push complete packet up to sk_receive_queue.

>    
> 2) the receiver is an older Linux without the above fix or another stack
>    that does not ABORT?

crap....

How about this.  If we in SHUTDOWN_PENDING state, let the errors accumulate upto
max_retrans.  After that, start SHUTDOWN_GUARD timer to let the association live a
bit longer just on the off-chance the receive comes back.  When SHUTDOWN_GUARD
expires it will abort the association.

When we are in this state, SACK processing will have to reset SHUTDOWN_GUARD when
the SACK is actually acknowledging something.

> 
> I agree that using ABORT on the receiver is the ideal way whenver
> possible but we still need to fix this if the receiver does not do so.
> 
> What sideeffects are you worried about resulting from my proposal?
> 

There is a potential that the sender may abort prematurely.  The issue is that
the sender has no way of knowing if the remote process somehow terminated and
will never consume data, or if it is just extremely busy with something else and
will come back.  Since this is a reliable protocol, we given the receive the benefit
of the doubt and try our hardest to get the data across.

My suggestion above is still a bit of a hack that one could argue still violates the
protocol, but the time period tries to remove as much doubt from the sender as possible
the the receiver is really out-to-lunch.

-vlad

Re: [PATCH] sctp: Enforce maximum retransmissions during shutdown

[Thomas Graf]

On Thu, Jun 30, 2011 at 10:08:40AM -0400, Vladislav Yasevich wrote:
> How about this.  If we in SHUTDOWN_PENDING state, let the errors accumulate upto
> max_retrans.  After that, start SHUTDOWN_GUARD timer to let the association live a
> bit longer just on the off-chance the receive comes back.  When SHUTDOWN_GUARD
> expires it will abort the association.
> 
> When we are in this state, SACK processing will have to reset SHUTDOWN_GUARD when
> the SACK is actually acknowledging something.

Good idea. I'll update my patch.

> > 
> > What sideeffects are you worried about resulting from my proposal?
> > 
> 
> There is a potential that the sender may abort prematurely.  The issue is that
> the sender has no way of knowing if the remote process somehow terminated and
> will never consume data, or if it is just extremely busy with something else and
> will come back.  Since this is a reliable protocol, we given the receive the benefit
> of the doubt and try our hardest to get the data across.

Understood although we are talking 10 * RTO here without an actual SACK.

> My suggestion above is still a bit of a hack that one could argue still violates the
> protocol, but the time period tries to remove as much doubt from the sender as possible
> the the receiver is really out-to-lunch.

Assuming that by 'shutdown sequence' the spec is only referring to the
SHUTDOWN / SHUTDOWN ACK exchange it would still violate the protocol
but I don't see how to avoid having association hang around forever without
violating the spec. This really looks like a hole in the spec to me.

[PATCHv2] sctp: Enforce retransmission limit during shutdown

[Thomas Graf]

When initiating a graceful shutdown while having data chunks
on the retransmission queue with a peer which is in zero
window mode the shutdown is never completed because the
retransmission error count is reset periodically by the
following two rules:

 - Do not timeout association while doing zero window probe.
 - Reset overall error count when a heartbeat request has
   been acknowledged.

The graceful shutdown will wait for all outstanding TSN to
be acknowledged before sending the SHUTDOWN request. This
never happens due to the peer's zero window not acknowledging
the continuously retransmitted data chunks. Although the
error counter is incremented for each failed retransmission,
the receiving of the SACK announcing the zero window clears
the error count again immediately. Also heartbeat requests
continue to be sent periodically. The peer acknowledges these
requests causing the error counter to be reset as well.

This patch changes behaviour to only reset the overall error
counter for the above rules while not in shutdown. After
reaching the maximum number of retransmission attempts, the
T5 shutdown guard timer is scheduled to give the receiver
some additional time to recover. The timer is stopped as soon
as the receiver acknowledges any data.

The issue can be easily reproduced by establishing a sctp
association over the loopback device, constantly queueing
data at the sender while not reading any at the receiver.
Wait for the window to reach zero, then initiate a shutdown
by killing both processes simultaneously. The association
will never be freed and the chunks on the retransmission
queue will be retransmitted indefinitely.

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

diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
index 1c88c89..0ae911f 100644
--- a/net/sctp/outqueue.c
+++ b/net/sctp/outqueue.c
@@ -1582,6 +1582,9 @@ static void sctp_check_transmitted(struct sctp_outq *q,
 #endif /* SCTP_DEBUG */
 	if (transport) {
 		if (bytes_acked) {
+			struct sctp_association *asoc = transport->asoc;
+			struct timer_list *t;
+
 			/* We may have counted DATA that was migrated
 			 * to this transport due to DEL-IP operation.
 			 * Subtract those bytes, since the were never
@@ -1600,6 +1603,17 @@ static void sctp_check_transmitted(struct sctp_outq *q,
 			transport->error_count = 0;
 			transport->asoc->overall_error_count = 0;
 
+			/*
+			 * While in SHUTDOWN PENDING, we may have started
+			 * the T5 shutdown guard timer after reaching the
+			 * retransmission limit. Stop that timer as soon
+			 * as the receiver acknowledged any data.
+			 */
+			t = &asoc->timers[SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD];
+			if (asoc->state == SCTP_STATE_SHUTDOWN_PENDING &&
+			    timer_pending(t) && del_timer(t))
+				sctp_association_put(asoc);
+
 			/* Mark the destination transport address as
 			 * active if it is not so marked.
 			 */
@@ -1629,10 +1643,15 @@ static void sctp_check_transmitted(struct sctp_outq *q,
 			 * A sender is doing zero window probing when the
 			 * receiver's advertised window is zero, and there is
 			 * only one data chunk in flight to the receiver.
+			 *
+			 * Allow the association to timeout if SHUTDOWN is
+			 * pending in case the receiver stays in zero window
+			 * mode forever.
 			 */
 			if (!q->asoc->peer.rwnd &&
 			    !list_empty(&tlist) &&
-			    (sack_ctsn+2 == q->asoc->next_tsn)) {
+			    (sack_ctsn+2 == q->asoc->next_tsn) &&
+			    !(q->asoc->state >= SCTP_STATE_SHUTDOWN_PENDING)) {
 				SCTP_DEBUG_PRINTK("%s: SACK received for zero "
 						  "window probe: %u\n",
 						  __func__, sack_ctsn);
diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index 534c2e5..fa92f4d6 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c
@@ -670,10 +670,21 @@ static void sctp_cmd_transport_on(sctp_cmd_seq_t *cmds,
 	/* 8.3 Upon the receipt of the HEARTBEAT ACK, the sender of the
 	 * HEARTBEAT should clear the error counter of the destination
 	 * transport address to which the HEARTBEAT was sent.
-	 * The association's overall error count is also cleared.
 	 */
 	t->error_count = 0;
-	t->asoc->overall_error_count = 0;
+
+	/*
+	 * Although RFC2960 and RFC4460 specify that the overall error
+	 * count must be cleared when a HEARTBEAT ACK is received this
+	 * behaviour may prevent the maximum retransmission count from
+	 * being reached while in SHUTDOWN. If the peer keeps its window
+	 * closed not acknowledging any outstanding TSN we may rely on
+	 * reaching the max_retrans limit via the T3-rtx timer to close
+	 * the association which will never happen if the error count is
+	 * reset every heartbeat interval.
+	 */
+	if (!(t->asoc->state >= SCTP_STATE_SHUTDOWN_PENDING))
+		t->asoc->overall_error_count = 0;
 
 	/* Clear the hb_sent flag to signal that we had a good
 	 * acknowledgement.
diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index a297283..e6a0c35 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -5154,7 +5154,7 @@ sctp_disposition_t sctp_sf_do_9_2_start_shutdown(
 	 * The sender of the SHUTDOWN MAY also start an overall guard timer
 	 * 'T5-shutdown-guard' to bound the overall time for shutdown sequence.
 	 */
-	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
+	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 			SCTP_TO(SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD));
 
 	if (asoc->autoclose)
@@ -5299,14 +5299,28 @@ sctp_disposition_t sctp_sf_do_6_3_3_rtx(const struct sctp_endpoint *ep,
 	SCTP_INC_STATS(SCTP_MIB_T3_RTX_EXPIREDS);
 
 	if (asoc->overall_error_count >= asoc->max_retrans) {
-		sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR,
-				SCTP_ERROR(ETIMEDOUT));
-		/* CMD_ASSOC_FAILED calls CMD_DELETE_TCB. */
-		sctp_add_cmd_sf(commands, SCTP_CMD_ASSOC_FAILED,
-				SCTP_PERR(SCTP_ERROR_NO_ERROR));
-		SCTP_INC_STATS(SCTP_MIB_ABORTEDS);
-		SCTP_DEC_STATS(SCTP_MIB_CURRESTAB);
-		return SCTP_DISPOSITION_DELETE_TCB;
+		if (asoc->state == SCTP_STATE_SHUTDOWN_PENDING) {
+			/*
+			 * We are here likely because the receiver had its rwnd
+			 * closed for a while and we have not been able to
+			 * transmit the locally queued data within the maximum
+			 * retransmission attempts limit.  Start the T5
+			 * shutdown guard timer to give the receiver one last
+			 * chance and some additional time to recover before
+			 * aborting.
+			 */
+			sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
+				SCTP_TO(SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD));
+		} else {
+			sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR,
+					SCTP_ERROR(ETIMEDOUT));
+			/* CMD_ASSOC_FAILED calls CMD_DELETE_TCB. */
+			sctp_add_cmd_sf(commands, SCTP_CMD_ASSOC_FAILED,
+					SCTP_PERR(SCTP_ERROR_NO_ERROR));
+			SCTP_INC_STATS(SCTP_MIB_ABORTEDS);
+			SCTP_DEC_STATS(SCTP_MIB_CURRESTAB);
+			return SCTP_DISPOSITION_DELETE_TCB;
+		}
 	}
 
 	/* E1) For the destination address for which the timer
diff --git a/net/sctp/sm_statetable.c b/net/sctp/sm_statetable.c
index 0338dc6..7c211a7 100644
--- a/net/sctp/sm_statetable.c
+++ b/net/sctp/sm_statetable.c
@@ -827,7 +827,7 @@ static const sctp_sm_table_entry_t other_event_table[SCTP_NUM_OTHER_TYPES][SCTP_
 	/* SCTP_STATE_ESTABLISHED */ \
 	TYPE_SCTP_FUNC(sctp_sf_timer_ignore), \
 	/* SCTP_STATE_SHUTDOWN_PENDING */ \
-	TYPE_SCTP_FUNC(sctp_sf_timer_ignore), \
+	TYPE_SCTP_FUNC(sctp_sf_t5_timer_expire), \
 	/* SCTP_STATE_SHUTDOWN_SENT */ \
 	TYPE_SCTP_FUNC(sctp_sf_t5_timer_expire), \
 	/* SCTP_STATE_SHUTDOWN_RECEIVED */ \

Re: [PATCHv2] sctp: Enforce retransmission limit during shutdown

[David Miller]

Vlad, SCTP folks, please review this patch.

Re: [PATCHv2] sctp: Enforce retransmission limit during shutdown

[Neil Horman]

On Mon, Jul 04, 2011 at 09:50:19AM -0400, Thomas Graf wrote:
> When initiating a graceful shutdown while having data chunks
> on the retransmission queue with a peer which is in zero
> window mode the shutdown is never completed because the
> retransmission error count is reset periodically by the
> following two rules:
> 
>  - Do not timeout association while doing zero window probe.
>  - Reset overall error count when a heartbeat request has
>    been acknowledged.
> 
> The graceful shutdown will wait for all outstanding TSN to
> be acknowledged before sending the SHUTDOWN request. This
> never happens due to the peer's zero window not acknowledging
> the continuously retransmitted data chunks. Although the
> error counter is incremented for each failed retransmission,
> the receiving of the SACK announcing the zero window clears
> the error count again immediately. Also heartbeat requests
> continue to be sent periodically. The peer acknowledges these
> requests causing the error counter to be reset as well.
> 
> This patch changes behaviour to only reset the overall error
> counter for the above rules while not in shutdown. After
> reaching the maximum number of retransmission attempts, the
> T5 shutdown guard timer is scheduled to give the receiver
> some additional time to recover. The timer is stopped as soon
> as the receiver acknowledges any data.
> 
> The issue can be easily reproduced by establishing a sctp
> association over the loopback device, constantly queueing
> data at the sender while not reading any at the receiver.
> Wait for the window to reach zero, then initiate a shutdown
> by killing both processes simultaneously. The association
> will never be freed and the chunks on the retransmission
> queue will be retransmitted indefinitely.
> 
> Signed-off-by: Thomas Graf <tgraf@infradead.org>
<snip>
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -5154,7 +5154,7 @@ sctp_disposition_t sctp_sf_do_9_2_start_shutdown(
>  	 * The sender of the SHUTDOWN MAY also start an overall guard timer
>  	 * 'T5-shutdown-guard' to bound the overall time for shutdown sequence.
>  	 */
> -	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
> +	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
>  			SCTP_TO(SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD));
>  
How come you're modifying this chunk to use TIMER_RESTART rather than
TIMER_START? start shutdown is where the t5 timer is actually started, isn't it?


The rest, I think looks ok to me.
Neil

Re: [PATCHv2] sctp: Enforce retransmission limit during shutdown

[Thomas Graf]

On Wed, 2011-07-06 at 08:15 -0400, Neil Horman wrote: 
> On Mon, Jul 04, 2011 at 09:50:19AM -0400, Thomas Graf wrote:

> > --- a/net/sctp/sm_statefuns.c
> > +++ b/net/sctp/sm_statefuns.c
> > @@ -5154,7 +5154,7 @@ sctp_disposition_t sctp_sf_do_9_2_start_shutdown(
> >  	 * The sender of the SHUTDOWN MAY also start an overall guard timer
> >  	 * 'T5-shutdown-guard' to bound the overall time for shutdown sequence.
> >  	 */
> > -	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
> > +	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
> >  			SCTP_TO(SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD));
> >  
> How come you're modifying this chunk to use TIMER_RESTART rather than
> TIMER_START? start shutdown is where the t5 timer is actually started, isn't it?

Since we also start the timer in SHUTDOWN_PENDING now if we hit
the retransmission limit the timer may be running already and
needs to be restarted (at least in theory).

In reality the timer should be stopped though, we can only go
from SHUTDOWN_PENDING into SHUTDOWN by actually SACKing bytes which
will delete the timer. This may change though and I did not want
this to bite us later on.

Re: [PATCHv2] sctp: Enforce retransmission limit during shutdown

[Neil Horman]

On Wed, Jul 06, 2011 at 03:16:24PM +0200, Thomas Graf wrote:
> On Wed, 2011-07-06 at 08:15 -0400, Neil Horman wrote: 
> > On Mon, Jul 04, 2011 at 09:50:19AM -0400, Thomas Graf wrote:
> 
> > > --- a/net/sctp/sm_statefuns.c
> > > +++ b/net/sctp/sm_statefuns.c
> > > @@ -5154,7 +5154,7 @@ sctp_disposition_t sctp_sf_do_9_2_start_shutdown(
> > >  	 * The sender of the SHUTDOWN MAY also start an overall guard timer
> > >  	 * 'T5-shutdown-guard' to bound the overall time for shutdown sequence.
> > >  	 */
> > > -	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
> > > +	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
> > >  			SCTP_TO(SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD));
> > >  
> > How come you're modifying this chunk to use TIMER_RESTART rather than
> > TIMER_START? start shutdown is where the t5 timer is actually started, isn't it?
> 
> Since we also start the timer in SHUTDOWN_PENDING now if we hit
> the retransmission limit the timer may be running already and
> needs to be restarted (at least in theory).
> 
> In reality the timer should be stopped though, we can only go
> from SHUTDOWN_PENDING into SHUTDOWN by actually SACKing bytes which
> will delete the timer. This may change though and I did not want
> this to bite us later on.
> 
> 
> 
Ok, makes sense
Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCHv2] sctp: Enforce retransmission limit during shutdown

[Vladislav Yasevich]

Hi Tomas

Some minor nits and one substantial issue.  See below.

On a related note, were you going to re-submit the receiver patch as well?

On 07/04/2011 09:50 AM, Thomas Graf wrote:
> 
> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
> index 1c88c89..0ae911f 100644
> --- a/net/sctp/outqueue.c
> +++ b/net/sctp/outqueue.c
> @@ -1582,6 +1582,9 @@ static void sctp_check_transmitted(struct sctp_outq *q,
>  #endif /* SCTP_DEBUG */
>  	if (transport) {
>  		if (bytes_acked) {
> +			struct sctp_association *asoc = transport->asoc;
> +			struct timer_list *t;
> +
>  			/* We may have counted DATA that was migrated
>  			 * to this transport due to DEL-IP operation.
>  			 * Subtract those bytes, since the were never
> @@ -1600,6 +1603,17 @@ static void sctp_check_transmitted(struct sctp_outq *q,
>  			transport->error_count = 0;
>  			transport->asoc->overall_error_count = 0;
>  
> +			/*
> +			 * While in SHUTDOWN PENDING, we may have started
> +			 * the T5 shutdown guard timer after reaching the
> +			 * retransmission limit. Stop that timer as soon
> +			 * as the receiver acknowledged any data.
> +			 */
> +			t = &asoc->timers[SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD];
> +			if (asoc->state == SCTP_STATE_SHUTDOWN_PENDING &&
> +			    timer_pending(t) && del_timer(t))
> +				sctp_association_put(asoc);
> +

I believe 'state' and 'timers' are in different cache lines, so might be able to optimize it
a little by checking the state prior to referencing timers array.

>  			/* Mark the destination transport address as
>  			 * active if it is not so marked.
>  			 */
> @@ -1629,10 +1643,15 @@ static void sctp_check_transmitted(struct sctp_outq *q,
>  			 * A sender is doing zero window probing when the
>  			 * receiver's advertised window is zero, and there is
>  			 * only one data chunk in flight to the receiver.
> +			 *
> +			 * Allow the association to timeout if SHUTDOWN is
> +			 * pending in case the receiver stays in zero window
> +			 * mode forever.
>  			 */
>  			if (!q->asoc->peer.rwnd &&
>  			    !list_empty(&tlist) &&
> -			    (sack_ctsn+2 == q->asoc->next_tsn)) {
> +			    (sack_ctsn+2 == q->asoc->next_tsn) &&
> +			    !(q->asoc->state >= SCTP_STATE_SHUTDOWN_PENDING)) {

Would a test for (q->asoc->state != SCTP_STATE_SHUTDOWN_PENDING) be clearer?  We only
care about the PENDING state here.

>  				SCTP_DEBUG_PRINTK("%s: SACK received for zero "
>  						  "window probe: %u\n",
>  						  __func__, sack_ctsn);
> diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> index 534c2e5..fa92f4d6 100644
> --- a/net/sctp/sm_sideeffect.c
> +++ b/net/sctp/sm_sideeffect.c
> @@ -670,10 +670,21 @@ static void sctp_cmd_transport_on(sctp_cmd_seq_t *cmds,
>  	/* 8.3 Upon the receipt of the HEARTBEAT ACK, the sender of the
>  	 * HEARTBEAT should clear the error counter of the destination
>  	 * transport address to which the HEARTBEAT was sent.
> -	 * The association's overall error count is also cleared.
>  	 */
>  	t->error_count = 0;
> -	t->asoc->overall_error_count = 0;
> +
> +	/*
> +	 * Although RFC2960 and RFC4460 specify that the overall error
> +	 * count must be cleared when a HEARTBEAT ACK is received this
> +	 * behaviour may prevent the maximum retransmission count from
> +	 * being reached while in SHUTDOWN. If the peer keeps its window
> +	 * closed not acknowledging any outstanding TSN we may rely on
> +	 * reaching the max_retrans limit via the T3-rtx timer to close
> +	 * the association which will never happen if the error count is
> +	 * reset every heartbeat interval.
> +	 */
> +	if (!(t->asoc->state >= SCTP_STATE_SHUTDOWN_PENDING))
> +		t->asoc->overall_error_count = 0;

Same here.  We only care about the PENDING state. Also, please fix the comment to reflect
the code.

>  
>  	/* Clear the hb_sent flag to signal that we had a good
>  	 * acknowledgement.
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index a297283..e6a0c35 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -5154,7 +5154,7 @@ sctp_disposition_t sctp_sf_do_9_2_start_shutdown(
>  	 * The sender of the SHUTDOWN MAY also start an overall guard timer
>  	 * 'T5-shutdown-guard' to bound the overall time for shutdown sequence.
>  	 */
> -	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
> +	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
>  			SCTP_TO(SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD));
>  
>  	if (asoc->autoclose)
> @@ -5299,14 +5299,28 @@ sctp_disposition_t sctp_sf_do_6_3_3_rtx(const struct sctp_endpoint *ep,
>  	SCTP_INC_STATS(SCTP_MIB_T3_RTX_EXPIREDS);
>  
>  	if (asoc->overall_error_count >= asoc->max_retrans) {
> -		sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR,
> -				SCTP_ERROR(ETIMEDOUT));
> -		/* CMD_ASSOC_FAILED calls CMD_DELETE_TCB. */
> -		sctp_add_cmd_sf(commands, SCTP_CMD_ASSOC_FAILED,
> -				SCTP_PERR(SCTP_ERROR_NO_ERROR));
> -		SCTP_INC_STATS(SCTP_MIB_ABORTEDS);
> -		SCTP_DEC_STATS(SCTP_MIB_CURRESTAB);
> -		return SCTP_DISPOSITION_DELETE_TCB;
> +		if (asoc->state == SCTP_STATE_SHUTDOWN_PENDING) {
> +			/*
> +			 * We are here likely because the receiver had its rwnd
> +			 * closed for a while and we have not been able to
> +			 * transmit the locally queued data within the maximum
> +			 * retransmission attempts limit.  Start the T5
> +			 * shutdown guard timer to give the receiver one last
> +			 * chance and some additional time to recover before
> +			 * aborting.
> +			 */
> +			sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
> +				SCTP_TO(SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD));

This is bug.  You don't want to restart the timer every time you hit a T3-timeout.  Remember, since you fall
through here, you do another retransmission and schedule another timeout.  So next time the timeout happens,
you'll restart the SHUTDOWN_GUARD, which is not what you want.

We want to start it once if it isn't pending, and leave it running without restart if it is already pending.

-vlad

> +		} else {
> +			sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR,
> +					SCTP_ERROR(ETIMEDOUT));
> +			/* CMD_ASSOC_FAILED calls CMD_DELETE_TCB. */
> +			sctp_add_cmd_sf(commands, SCTP_CMD_ASSOC_FAILED,
> +					SCTP_PERR(SCTP_ERROR_NO_ERROR));
> +			SCTP_INC_STATS(SCTP_MIB_ABORTEDS);
> +			SCTP_DEC_STATS(SCTP_MIB_CURRESTAB);
> +			return SCTP_DISPOSITION_DELETE_TCB;
> +		}
>  	}
>  
>  	/* E1) For the destination address for which the timer
> diff --git a/net/sctp/sm_statetable.c b/net/sctp/sm_statetable.c
> index 0338dc6..7c211a7 100644
> --- a/net/sctp/sm_statetable.c
> +++ b/net/sctp/sm_statetable.c
> @@ -827,7 +827,7 @@ static const sctp_sm_table_entry_t other_event_table[SCTP_NUM_OTHER_TYPES][SCTP_
>  	/* SCTP_STATE_ESTABLISHED */ \
>  	TYPE_SCTP_FUNC(sctp_sf_timer_ignore), \
>  	/* SCTP_STATE_SHUTDOWN_PENDING */ \
> -	TYPE_SCTP_FUNC(sctp_sf_timer_ignore), \
> +	TYPE_SCTP_FUNC(sctp_sf_t5_timer_expire), \
>  	/* SCTP_STATE_SHUTDOWN_SENT */ \
>  	TYPE_SCTP_FUNC(sctp_sf_t5_timer_expire), \
>  	/* SCTP_STATE_SHUTDOWN_RECEIVED */ \
> 

Re: [PATCHv2] sctp: Enforce retransmission limit during shutdown

[Thomas Graf]

On Wed, Jul 06, 2011 at 09:42:42AM -0400, Vladislav Yasevich wrote:
> On a related note, were you going to re-submit the receiver patch as well?

Yes

> On 07/04/2011 09:50 AM, Thomas Graf wrote:
> > +			 * retransmission limit. Stop that timer as soon
> > +			 * as the receiver acknowledged any data.
> > +			 */
> > +			t = &asoc->timers[SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD];
> > +			if (asoc->state == SCTP_STATE_SHUTDOWN_PENDING &&
> > +			    timer_pending(t) && del_timer(t))
> > +				sctp_association_put(asoc);
> > +
> 
> I believe 'state' and 'timers' are in different cache lines, so might be able to optimize it
> a little by checking the state prior to referencing timers array.

gcc should do that but I'm fine with changing it.

> > +			 *
> > +			 * Allow the association to timeout if SHUTDOWN is
> > +			 * pending in case the receiver stays in zero window
> > +			 * mode forever.
> >  			 */
> >  			if (!q->asoc->peer.rwnd &&
> >  			    !list_empty(&tlist) &&
> > -			    (sack_ctsn+2 == q->asoc->next_tsn)) {
> > +			    (sack_ctsn+2 == q->asoc->next_tsn) &&
> > +			    !(q->asoc->state >= SCTP_STATE_SHUTDOWN_PENDING)) {
> 
> Would a test for (q->asoc->state != SCTP_STATE_SHUTDOWN_PENDING) be clearer?  We only
> care about the PENDING state here.

I think SHUTDOWN_RECEIVED should also be included. We continue to transmit and
process SACKs after receiving a SHUTDOWN.

> > +	 * Although RFC2960 and RFC4460 specify that the overall error
> > +	 * count must be cleared when a HEARTBEAT ACK is received this
> > +	 * behaviour may prevent the maximum retransmission count from
> > +	 * being reached while in SHUTDOWN. If the peer keeps its window
> > +	 * closed not acknowledging any outstanding TSN we may rely on
> > +	 * reaching the max_retrans limit via the T3-rtx timer to close
> > +	 * the association which will never happen if the error count is
> > +	 * reset every heartbeat interval.
> > +	 */
> > +	if (!(t->asoc->state >= SCTP_STATE_SHUTDOWN_PENDING))
> > +		t->asoc->overall_error_count = 0;
> 
> Same here.  We only care about the PENDING state. Also, please fix the comment to reflect
> the code.

Agreed.

> > +		if (asoc->state == SCTP_STATE_SHUTDOWN_PENDING) {
> > +			/*
> > +			 * We are here likely because the receiver had its rwnd
> > +			 * closed for a while and we have not been able to
> > +			 * transmit the locally queued data within the maximum
> > +			 * retransmission attempts limit.  Start the T5
> > +			 * shutdown guard timer to give the receiver one last
> > +			 * chance and some additional time to recover before
> > +			 * aborting.
> > +			 */
> > +			sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
> > +				SCTP_TO(SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD));
> 
> This is bug.  You don't want to restart the timer every time you hit a T3-timeout.  Remember, since you fall
> through here, you do another retransmission and schedule another timeout.  So next time the timeout happens,
> you'll restart the SHUTDOWN_GUARD, which is not what you want.
> 
> We want to start it once if it isn't pending, and leave it running without restart if it is already pending.

Doh, absolutely. The timer_pending() check got lost between testing and submission.

Re: [PATCHv2] sctp: Enforce retransmission limit during shutdown

[Vladislav Yasevich]

On 07/06/2011 10:18 AM, Thomas Graf wrote:
> On Wed, Jul 06, 2011 at 09:42:42AM -0400, Vladislav Yasevich wrote:
>> On a related note, were you going to re-submit the receiver patch as well?
> 
> Yes
> 
>> On 07/04/2011 09:50 AM, Thomas Graf wrote:
>>> +			 * retransmission limit. Stop that timer as soon
>>> +			 * as the receiver acknowledged any data.
>>> +			 */
>>> +			t = &asoc->timers[SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD];
>>> +			if (asoc->state == SCTP_STATE_SHUTDOWN_PENDING &&
>>> +			    timer_pending(t) && del_timer(t))
>>> +				sctp_association_put(asoc);
>>> +
>>
>> I believe 'state' and 'timers' are in different cache lines, so might be able to optimize it
>> a little by checking the state prior to referencing timers array.
> 
> gcc should do that but I'm fine with changing it.
> 
>>> +			 *
>>> +			 * Allow the association to timeout if SHUTDOWN is
>>> +			 * pending in case the receiver stays in zero window
>>> +			 * mode forever.
>>>  			 */
>>>  			if (!q->asoc->peer.rwnd &&
>>>  			    !list_empty(&tlist) &&
>>> -			    (sack_ctsn+2 == q->asoc->next_tsn)) {
>>> +			    (sack_ctsn+2 == q->asoc->next_tsn) &&
>>> +			    !(q->asoc->state >= SCTP_STATE_SHUTDOWN_PENDING)) {
>>
>> Would a test for (q->asoc->state != SCTP_STATE_SHUTDOWN_PENDING) be clearer?  We only
>> care about the PENDING state here.
> 
> I think SHUTDOWN_RECEIVED should also be included. We continue to transmit and
> process SACKs after receiving a SHUTDOWN.

I am not sure about SHUTDOWN_RECEIVED.  If we received shutdown, then we are not in
a 0 window situation.  Additionally, the sender of the SHUTDOWN started the GUARD timer
and will abort after it expires.  So there is no special handling on our part.

-vlad

> 
>>> +	 * Although RFC2960 and RFC4460 specify that the overall error
>>> +	 * count must be cleared when a HEARTBEAT ACK is received this
>>> +	 * behaviour may prevent the maximum retransmission count from
>>> +	 * being reached while in SHUTDOWN. If the peer keeps its window
>>> +	 * closed not acknowledging any outstanding TSN we may rely on
>>> +	 * reaching the max_retrans limit via the T3-rtx timer to close
>>> +	 * the association which will never happen if the error count is
>>> +	 * reset every heartbeat interval.
>>> +	 */
>>> +	if (!(t->asoc->state >= SCTP_STATE_SHUTDOWN_PENDING))
>>> +		t->asoc->overall_error_count = 0;
>>
>> Same here.  We only care about the PENDING state. Also, please fix the comment to reflect
>> the code.
> 
> Agreed.
> 
>>> +		if (asoc->state == SCTP_STATE_SHUTDOWN_PENDING) {
>>> +			/*
>>> +			 * We are here likely because the receiver had its rwnd
>>> +			 * closed for a while and we have not been able to
>>> +			 * transmit the locally queued data within the maximum
>>> +			 * retransmission attempts limit.  Start the T5
>>> +			 * shutdown guard timer to give the receiver one last
>>> +			 * chance and some additional time to recover before
>>> +			 * aborting.
>>> +			 */
>>> +			sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
>>> +				SCTP_TO(SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD));
>>
>> This is bug.  You don't want to restart the timer every time you hit a T3-timeout.  Remember, since you fall
>> through here, you do another retransmission and schedule another timeout.  So next time the timeout happens,
>> you'll restart the SHUTDOWN_GUARD, which is not what you want.
>>
>> We want to start it once if it isn't pending, and leave it running without restart if it is already pending.
> 
> Doh, absolutely. The timer_pending() check got lost between testing and submission.
> 

Re: [PATCHv2] sctp: Enforce retransmission limit during shutdown

[Thomas Graf]

On Wed, Jul 06, 2011 at 10:31:56AM -0400, Vladislav Yasevich wrote:
> >>> +			 *
> >>> +			 * Allow the association to timeout if SHUTDOWN is
> >>> +			 * pending in case the receiver stays in zero window
> >>> +			 * mode forever.
> >>>  			 */
> >>>  			if (!q->asoc->peer.rwnd &&
> >>>  			    !list_empty(&tlist) &&
> >>> -			    (sack_ctsn+2 == q->asoc->next_tsn)) {
> >>> +			    (sack_ctsn+2 == q->asoc->next_tsn) &&
> >>> +			    !(q->asoc->state >= SCTP_STATE_SHUTDOWN_PENDING)) {
> >>
> >> Would a test for (q->asoc->state != SCTP_STATE_SHUTDOWN_PENDING) be clearer?  We only
> >> care about the PENDING state here.
> > 
> > I think SHUTDOWN_RECEIVED should also be included. We continue to transmit and
> > process SACKs after receiving a SHUTDOWN.
> 
> I am not sure about SHUTDOWN_RECEIVED.  If we received shutdown, then we are not in
> a 0 window situation.  Additionally, the sender of the SHUTDOWN started the GUARD timer
> and will abort after it expires.  So there is no special handling on our part.

Why can't we be in a 0 window situation? A well behaving sctp peer may not,
but we're on the Internet, everyone behaves at their worst :-)

Seriously, this would make for a simple dos. Establish a stream, don't ack any
data to make sure there is something on the retransmission queue of the peer.
Immediately shutdown the stream and ack any retransmission attempt with
a_rwnd=0 to keep the association around forever.

Starting the T5 SHUTDOWN GUARD timer is specified as MAY and not MUST so even in
a well behaving world we could not really rely on it.

Alternatively the peer could just be buggy as well.

Re: [PATCHv2] sctp: Enforce retransmission limit during shutdown

[Vladislav Yasevich]

On 07/06/2011 11:49 AM, Thomas Graf wrote:
> On Wed, Jul 06, 2011 at 10:31:56AM -0400, Vladislav Yasevich wrote:
>>>>> +			 *
>>>>> +			 * Allow the association to timeout if SHUTDOWN is
>>>>> +			 * pending in case the receiver stays in zero window
>>>>> +			 * mode forever.
>>>>>  			 */
>>>>>  			if (!q->asoc->peer.rwnd &&
>>>>>  			    !list_empty(&tlist) &&
>>>>> -			    (sack_ctsn+2 == q->asoc->next_tsn)) {
>>>>> +			    (sack_ctsn+2 == q->asoc->next_tsn) &&
>>>>> +			    !(q->asoc->state >= SCTP_STATE_SHUTDOWN_PENDING)) {
>>>>
>>>> Would a test for (q->asoc->state != SCTP_STATE_SHUTDOWN_PENDING) be clearer?  We only
>>>> care about the PENDING state here.
>>>
>>> I think SHUTDOWN_RECEIVED should also be included. We continue to transmit and
>>> process SACKs after receiving a SHUTDOWN.
>>
>> I am not sure about SHUTDOWN_RECEIVED.  If we received shutdown, then we are not in
>> a 0 window situation.  Additionally, the sender of the SHUTDOWN started the GUARD timer
>> and will abort after it expires.  So there is no special handling on our part.
> 
> Why can't we be in a 0 window situation? A well behaving sctp peer may not,
> but we're on the Internet, everyone behaves at their worst :-)
> 
> Seriously, this would make for a simple dos. Establish a stream, don't ack any
> data to make sure there is something on the retransmission queue of the peer.
> Immediately shutdown the stream and ack any retransmission attempt with
> a_rwnd=0 to keep the association around forever.
> 
> Starting the T5 SHUTDOWN GUARD timer is specified as MAY and not MUST so even in
> a well behaving world we could not really rely on it.
> 
> Alternatively the peer could just be buggy as well.
> 

You are right.  Without a receiver patch, a linux receiver would stay in 0-window condition
while sending a SHUTDOWN with a_rwnd of 0.

How about instead of checking for "Not greater then or equals", we instead simply test for
"less then"?

-vlad

Re: [PATCHv2] sctp: Enforce retransmission limit during shutdown

[Thomas Graf]

On Wed, Jul 06, 2011 at 12:23:50PM -0400, Vladislav Yasevich wrote:
> You are right.  Without a receiver patch, a linux receiver would stay in 0-window condition
> while sending a SHUTDOWN with a_rwnd of 0.
> 
> How about instead of checking for "Not greater then or equals", we instead simply test for
> "less then"?

Agreed

Will repost the patch with your suggestions included and look into the
receiver patch as well.

[PATCHv3] sctp: Enforce retransmission limit during shutdown

[Thomas Graf]

When initiating a graceful shutdown while having data chunks
on the retransmission queue with a peer which is in zero
window mode the shutdown is never completed because the
retransmission error count is reset periodically by the
following two rules:

 - Do not timeout association while doing zero window probe.
 - Reset overall error count when a heartbeat request has
   been acknowledged.

The graceful shutdown will wait for all outstanding TSN to
be acknowledged before sending the SHUTDOWN request. This
never happens due to the peer's zero window not acknowledging
the continuously retransmitted data chunks. Although the
error counter is incremented for each failed retransmission,
the receiving of the SACK announcing the zero window clears
the error count again immediately. Also heartbeat requests
continue to be sent periodically. The peer acknowledges these
requests causing the error counter to be reset as well.

This patch changes behaviour to only reset the overall error
counter for the above rules while not in shutdown. After
reaching the maximum number of retransmission attempts, the
T5 shutdown guard timer is scheduled to give the receiver
some additional time to recover. The timer is stopped as soon
as the receiver acknowledges any data.

The issue can be easily reproduced by establishing a sctp
association over the loopback device, constantly queueing
data at the sender while not reading any at the receiver.
Wait for the window to reach zero, then initiate a shutdown
by killing both processes simultaneously. The association
will never be freed and the chunks on the retransmission
queue will be retransmitted indefinitely.

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

diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h
index dd6847e..6506458 100644
--- a/include/net/sctp/command.h
+++ b/include/net/sctp/command.h
@@ -63,6 +63,7 @@ typedef enum {
 	SCTP_CMD_ECN_ECNE,	/* Do delayed ECNE processing. */
 	SCTP_CMD_ECN_CWR,	/* Do delayed CWR processing.  */
 	SCTP_CMD_TIMER_START,	/* Start a timer.  */
+	SCTP_CMD_TIMER_START_ONCE, /* Start a timer once */
 	SCTP_CMD_TIMER_RESTART,	/* Restart a timer. */
 	SCTP_CMD_TIMER_STOP,	/* Stop a timer. */
 	SCTP_CMD_INIT_CHOOSE_TRANSPORT, /* Choose transport for an INIT. */
diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
index 1c88c89..d036821 100644
--- a/net/sctp/outqueue.c
+++ b/net/sctp/outqueue.c
@@ -1582,6 +1582,8 @@ static void sctp_check_transmitted(struct sctp_outq *q,
 #endif /* SCTP_DEBUG */
 	if (transport) {
 		if (bytes_acked) {
+			struct sctp_association *asoc = transport->asoc;
+
 			/* We may have counted DATA that was migrated
 			 * to this transport due to DEL-IP operation.
 			 * Subtract those bytes, since the were never
@@ -1600,6 +1602,17 @@ static void sctp_check_transmitted(struct sctp_outq *q,
 			transport->error_count = 0;
 			transport->asoc->overall_error_count = 0;
 
+			/*
+			 * While in SHUTDOWN PENDING, we may have started
+			 * the T5 shutdown guard timer after reaching the
+			 * retransmission limit. Stop that timer as soon
+			 * as the receiver acknowledged any data.
+			 */
+			if (asoc->state == SCTP_STATE_SHUTDOWN_PENDING &&
+			    del_timer(&asoc->timers
+				[SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD]))
+					sctp_association_put(asoc);
+
 			/* Mark the destination transport address as
 			 * active if it is not so marked.
 			 */
@@ -1629,10 +1642,15 @@ static void sctp_check_transmitted(struct sctp_outq *q,
 			 * A sender is doing zero window probing when the
 			 * receiver's advertised window is zero, and there is
 			 * only one data chunk in flight to the receiver.
+			 *
+			 * Allow the association to timeout while in SHUTDOWN
+			 * PENDING or SHUTDOWN RECEIVED in case the receiver
+			 * stays in zero window mode forever.
 			 */
 			if (!q->asoc->peer.rwnd &&
 			    !list_empty(&tlist) &&
-			    (sack_ctsn+2 == q->asoc->next_tsn)) {
+			    (sack_ctsn+2 == q->asoc->next_tsn) &&
+			    q->asoc->state < SCTP_STATE_SHUTDOWN_PENDING) {
 				SCTP_DEBUG_PRINTK("%s: SACK received for zero "
 						  "window probe: %u\n",
 						  __func__, sack_ctsn);
diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index 534c2e5..6e0f882 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c
@@ -670,10 +670,19 @@ static void sctp_cmd_transport_on(sctp_cmd_seq_t *cmds,
 	/* 8.3 Upon the receipt of the HEARTBEAT ACK, the sender of the
 	 * HEARTBEAT should clear the error counter of the destination
 	 * transport address to which the HEARTBEAT was sent.
-	 * The association's overall error count is also cleared.
 	 */
 	t->error_count = 0;
-	t->asoc->overall_error_count = 0;
+
+	/*
+	 * Although RFC4960 specifies that the overall error count must
+	 * be cleared when a HEARTBEAT ACK is received, we make an
+	 * exception while in SHUTDOWN PENDING. If the peer keeps its
+	 * window shut forever, we may never be able to transmit our
+	 * outstanding data and rely on the retransmission limit be reached
+	 * to shutdown the association.
+	 */
+	if (t->asoc->state != SCTP_STATE_SHUTDOWN_PENDING)
+		t->asoc->overall_error_count = 0;
 
 	/* Clear the hb_sent flag to signal that we had a good
 	 * acknowledgement.
@@ -1437,6 +1446,13 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 			sctp_cmd_setup_t2(commands, asoc, cmd->obj.ptr);
 			break;
 
+		case SCTP_CMD_TIMER_START_ONCE:
+			timer = &asoc->timers[cmd->obj.to];
+
+			if (timer_pending(timer))
+				break;
+			/* fall through */
+
 		case SCTP_CMD_TIMER_START:
 			timer = &asoc->timers[cmd->obj.to];
 			timeout = asoc->timeouts[cmd->obj.to];
diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index a297283..2461171 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -5154,7 +5154,7 @@ sctp_disposition_t sctp_sf_do_9_2_start_shutdown(
 	 * The sender of the SHUTDOWN MAY also start an overall guard timer
 	 * 'T5-shutdown-guard' to bound the overall time for shutdown sequence.
 	 */
-	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
+	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
 			SCTP_TO(SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD));
 
 	if (asoc->autoclose)
@@ -5299,14 +5299,28 @@ sctp_disposition_t sctp_sf_do_6_3_3_rtx(const struct sctp_endpoint *ep,
 	SCTP_INC_STATS(SCTP_MIB_T3_RTX_EXPIREDS);
 
 	if (asoc->overall_error_count >= asoc->max_retrans) {
-		sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR,
-				SCTP_ERROR(ETIMEDOUT));
-		/* CMD_ASSOC_FAILED calls CMD_DELETE_TCB. */
-		sctp_add_cmd_sf(commands, SCTP_CMD_ASSOC_FAILED,
-				SCTP_PERR(SCTP_ERROR_NO_ERROR));
-		SCTP_INC_STATS(SCTP_MIB_ABORTEDS);
-		SCTP_DEC_STATS(SCTP_MIB_CURRESTAB);
-		return SCTP_DISPOSITION_DELETE_TCB;
+		if (asoc->state == SCTP_STATE_SHUTDOWN_PENDING) {
+			/*
+			 * We are here likely because the receiver had its rwnd
+			 * closed for a while and we have not been able to
+			 * transmit the locally queued data within the maximum
+			 * retransmission attempts limit.  Start the T5
+			 * shutdown guard timer to give the receiver one last
+			 * chance and some additional time to recover before
+			 * aborting.
+			 */
+			sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START_ONCE,
+				SCTP_TO(SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD));
+		} else {
+			sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR,
+					SCTP_ERROR(ETIMEDOUT));
+			/* CMD_ASSOC_FAILED calls CMD_DELETE_TCB. */
+			sctp_add_cmd_sf(commands, SCTP_CMD_ASSOC_FAILED,
+					SCTP_PERR(SCTP_ERROR_NO_ERROR));
+			SCTP_INC_STATS(SCTP_MIB_ABORTEDS);
+			SCTP_DEC_STATS(SCTP_MIB_CURRESTAB);
+			return SCTP_DISPOSITION_DELETE_TCB;
+		}
 	}
 
 	/* E1) For the destination address for which the timer
diff --git a/net/sctp/sm_statetable.c b/net/sctp/sm_statetable.c
index 0338dc6..7c211a7 100644
--- a/net/sctp/sm_statetable.c
+++ b/net/sctp/sm_statetable.c
@@ -827,7 +827,7 @@ static const sctp_sm_table_entry_t other_event_table[SCTP_NUM_OTHER_TYPES][SCTP_
 	/* SCTP_STATE_ESTABLISHED */ \
 	TYPE_SCTP_FUNC(sctp_sf_timer_ignore), \
 	/* SCTP_STATE_SHUTDOWN_PENDING */ \
-	TYPE_SCTP_FUNC(sctp_sf_timer_ignore), \
+	TYPE_SCTP_FUNC(sctp_sf_t5_timer_expire), \
 	/* SCTP_STATE_SHUTDOWN_SENT */ \
 	TYPE_SCTP_FUNC(sctp_sf_t5_timer_expire), \
 	/* SCTP_STATE_SHUTDOWN_RECEIVED */ \

Re: [PATCHv3] sctp: Enforce retransmission limit during shutdown

[Vladislav Yasevich]

On 07/07/2011 06:28 AM, Thomas Graf wrote:
> When initiating a graceful shutdown while having data chunks
> on the retransmission queue with a peer which is in zero
> window mode the shutdown is never completed because the
> retransmission error count is reset periodically by the
> following two rules:
> 
>  - Do not timeout association while doing zero window probe.
>  - Reset overall error count when a heartbeat request has
>    been acknowledged.
> 
> The graceful shutdown will wait for all outstanding TSN to
> be acknowledged before sending the SHUTDOWN request. This
> never happens due to the peer's zero window not acknowledging
> the continuously retransmitted data chunks. Although the
> error counter is incremented for each failed retransmission,
> the receiving of the SACK announcing the zero window clears
> the error count again immediately. Also heartbeat requests
> continue to be sent periodically. The peer acknowledges these
> requests causing the error counter to be reset as well.
> 
> This patch changes behaviour to only reset the overall error
> counter for the above rules while not in shutdown. After
> reaching the maximum number of retransmission attempts, the
> T5 shutdown guard timer is scheduled to give the receiver
> some additional time to recover. The timer is stopped as soon
> as the receiver acknowledges any data.
> 
> The issue can be easily reproduced by establishing a sctp
> association over the loopback device, constantly queueing
> data at the sender while not reading any at the receiver.
> Wait for the window to reach zero, then initiate a shutdown
> by killing both processes simultaneously. The association
> will never be freed and the chunks on the retransmission
> queue will be retransmitted indefinitely.
> 
> Signed-off-by: Thomas Graf <tgraf@infradead.org>

Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>

Thanks
-vlad

> 
> diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h
> index dd6847e..6506458 100644
> --- a/include/net/sctp/command.h
> +++ b/include/net/sctp/command.h
> @@ -63,6 +63,7 @@ typedef enum {
>  	SCTP_CMD_ECN_ECNE,	/* Do delayed ECNE processing. */
>  	SCTP_CMD_ECN_CWR,	/* Do delayed CWR processing.  */
>  	SCTP_CMD_TIMER_START,	/* Start a timer.  */
> +	SCTP_CMD_TIMER_START_ONCE, /* Start a timer once */
>  	SCTP_CMD_TIMER_RESTART,	/* Restart a timer. */
>  	SCTP_CMD_TIMER_STOP,	/* Stop a timer. */
>  	SCTP_CMD_INIT_CHOOSE_TRANSPORT, /* Choose transport for an INIT. */
> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
> index 1c88c89..d036821 100644
> --- a/net/sctp/outqueue.c
> +++ b/net/sctp/outqueue.c
> @@ -1582,6 +1582,8 @@ static void sctp_check_transmitted(struct sctp_outq *q,
>  #endif /* SCTP_DEBUG */
>  	if (transport) {
>  		if (bytes_acked) {
> +			struct sctp_association *asoc = transport->asoc;
> +
>  			/* We may have counted DATA that was migrated
>  			 * to this transport due to DEL-IP operation.
>  			 * Subtract those bytes, since the were never
> @@ -1600,6 +1602,17 @@ static void sctp_check_transmitted(struct sctp_outq *q,
>  			transport->error_count = 0;
>  			transport->asoc->overall_error_count = 0;
>  
> +			/*
> +			 * While in SHUTDOWN PENDING, we may have started
> +			 * the T5 shutdown guard timer after reaching the
> +			 * retransmission limit. Stop that timer as soon
> +			 * as the receiver acknowledged any data.
> +			 */
> +			if (asoc->state == SCTP_STATE_SHUTDOWN_PENDING &&
> +			    del_timer(&asoc->timers
> +				[SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD]))
> +					sctp_association_put(asoc);
> +
>  			/* Mark the destination transport address as
>  			 * active if it is not so marked.
>  			 */
> @@ -1629,10 +1642,15 @@ static void sctp_check_transmitted(struct sctp_outq *q,
>  			 * A sender is doing zero window probing when the
>  			 * receiver's advertised window is zero, and there is
>  			 * only one data chunk in flight to the receiver.
> +			 *
> +			 * Allow the association to timeout while in SHUTDOWN
> +			 * PENDING or SHUTDOWN RECEIVED in case the receiver
> +			 * stays in zero window mode forever.
>  			 */
>  			if (!q->asoc->peer.rwnd &&
>  			    !list_empty(&tlist) &&
> -			    (sack_ctsn+2 == q->asoc->next_tsn)) {
> +			    (sack_ctsn+2 == q->asoc->next_tsn) &&
> +			    q->asoc->state < SCTP_STATE_SHUTDOWN_PENDING) {
>  				SCTP_DEBUG_PRINTK("%s: SACK received for zero "
>  						  "window probe: %u\n",
>  						  __func__, sack_ctsn);
> diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> index 534c2e5..6e0f882 100644
> --- a/net/sctp/sm_sideeffect.c
> +++ b/net/sctp/sm_sideeffect.c
> @@ -670,10 +670,19 @@ static void sctp_cmd_transport_on(sctp_cmd_seq_t *cmds,
>  	/* 8.3 Upon the receipt of the HEARTBEAT ACK, the sender of the
>  	 * HEARTBEAT should clear the error counter of the destination
>  	 * transport address to which the HEARTBEAT was sent.
> -	 * The association's overall error count is also cleared.
>  	 */
>  	t->error_count = 0;
> -	t->asoc->overall_error_count = 0;
> +
> +	/*
> +	 * Although RFC4960 specifies that the overall error count must
> +	 * be cleared when a HEARTBEAT ACK is received, we make an
> +	 * exception while in SHUTDOWN PENDING. If the peer keeps its
> +	 * window shut forever, we may never be able to transmit our
> +	 * outstanding data and rely on the retransmission limit be reached
> +	 * to shutdown the association.
> +	 */
> +	if (t->asoc->state != SCTP_STATE_SHUTDOWN_PENDING)
> +		t->asoc->overall_error_count = 0;
>  
>  	/* Clear the hb_sent flag to signal that we had a good
>  	 * acknowledgement.
> @@ -1437,6 +1446,13 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
>  			sctp_cmd_setup_t2(commands, asoc, cmd->obj.ptr);
>  			break;
>  
> +		case SCTP_CMD_TIMER_START_ONCE:
> +			timer = &asoc->timers[cmd->obj.to];
> +
> +			if (timer_pending(timer))
> +				break;
> +			/* fall through */
> +
>  		case SCTP_CMD_TIMER_START:
>  			timer = &asoc->timers[cmd->obj.to];
>  			timeout = asoc->timeouts[cmd->obj.to];
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index a297283..2461171 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -5154,7 +5154,7 @@ sctp_disposition_t sctp_sf_do_9_2_start_shutdown(
>  	 * The sender of the SHUTDOWN MAY also start an overall guard timer
>  	 * 'T5-shutdown-guard' to bound the overall time for shutdown sequence.
>  	 */
> -	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
> +	sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
>  			SCTP_TO(SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD));
>  
>  	if (asoc->autoclose)
> @@ -5299,14 +5299,28 @@ sctp_disposition_t sctp_sf_do_6_3_3_rtx(const struct sctp_endpoint *ep,
>  	SCTP_INC_STATS(SCTP_MIB_T3_RTX_EXPIREDS);
>  
>  	if (asoc->overall_error_count >= asoc->max_retrans) {
> -		sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR,
> -				SCTP_ERROR(ETIMEDOUT));
> -		/* CMD_ASSOC_FAILED calls CMD_DELETE_TCB. */
> -		sctp_add_cmd_sf(commands, SCTP_CMD_ASSOC_FAILED,
> -				SCTP_PERR(SCTP_ERROR_NO_ERROR));
> -		SCTP_INC_STATS(SCTP_MIB_ABORTEDS);
> -		SCTP_DEC_STATS(SCTP_MIB_CURRESTAB);
> -		return SCTP_DISPOSITION_DELETE_TCB;
> +		if (asoc->state == SCTP_STATE_SHUTDOWN_PENDING) {
> +			/*
> +			 * We are here likely because the receiver had its rwnd
> +			 * closed for a while and we have not been able to
> +			 * transmit the locally queued data within the maximum
> +			 * retransmission attempts limit.  Start the T5
> +			 * shutdown guard timer to give the receiver one last
> +			 * chance and some additional time to recover before
> +			 * aborting.
> +			 */
> +			sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START_ONCE,
> +				SCTP_TO(SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD));
> +		} else {
> +			sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR,
> +					SCTP_ERROR(ETIMEDOUT));
> +			/* CMD_ASSOC_FAILED calls CMD_DELETE_TCB. */
> +			sctp_add_cmd_sf(commands, SCTP_CMD_ASSOC_FAILED,
> +					SCTP_PERR(SCTP_ERROR_NO_ERROR));
> +			SCTP_INC_STATS(SCTP_MIB_ABORTEDS);
> +			SCTP_DEC_STATS(SCTP_MIB_CURRESTAB);
> +			return SCTP_DISPOSITION_DELETE_TCB;
> +		}
>  	}
>  
>  	/* E1) For the destination address for which the timer
> diff --git a/net/sctp/sm_statetable.c b/net/sctp/sm_statetable.c
> index 0338dc6..7c211a7 100644
> --- a/net/sctp/sm_statetable.c
> +++ b/net/sctp/sm_statetable.c
> @@ -827,7 +827,7 @@ static const sctp_sm_table_entry_t other_event_table[SCTP_NUM_OTHER_TYPES][SCTP_
>  	/* SCTP_STATE_ESTABLISHED */ \
>  	TYPE_SCTP_FUNC(sctp_sf_timer_ignore), \
>  	/* SCTP_STATE_SHUTDOWN_PENDING */ \
> -	TYPE_SCTP_FUNC(sctp_sf_timer_ignore), \
> +	TYPE_SCTP_FUNC(sctp_sf_t5_timer_expire), \
>  	/* SCTP_STATE_SHUTDOWN_SENT */ \
>  	TYPE_SCTP_FUNC(sctp_sf_t5_timer_expire), \
>  	/* SCTP_STATE_SHUTDOWN_RECEIVED */ \
> 

Re: [PATCHv3] sctp: Enforce retransmission limit during shutdown

[David Miller]

From: Vladislav Yasevich <vladislav.yasevich@hp.com>
Date: Thu, 07 Jul 2011 09:36:26 -0400

> On 07/07/2011 06:28 AM, Thomas Graf wrote:
>> When initiating a graceful shutdown while having data chunks
>> on the retransmission queue with a peer which is in zero
>> window mode the shutdown is never completed because the
>> retransmission error count is reset periodically by the
>> following two rules:
 ...
>> Signed-off-by: Thomas Graf <tgraf@infradead.org>
> 
> Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>

Applied.

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

[Thomas Graf]

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>

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

Re: [PATCH] sctp: ABORT if receive queue is not empty while closing socket

[Vladislav Yasevich]

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

Re: [PATCH] sctp: ABORT if receive queue is not empty while closing socket

[Thomas Graf]

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?

Re: [PATCH] sctp: ABORT if receive queue is not empty while closing socket

[Vladislav Yasevich]

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

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

[Thomas Graf]

Trigger user ABORT if application closes a socket which has data
queued on the socket receive queue as this would imply data being
lost which defeats the point of a graceful shutdown.

This behavior is already practiced in TCP.

We do not check the input queue because that would mean to parse
all chunks on it to look for unacknowledged data which seems too
much of an effort. Control chunks or duplicated chunks may also
be in the input queue and should not be stopping a graceful
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 08c6238..17cb3fc 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..8a84017 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)
-		sctp_ulpevent_free(sctp_skb2event(skb));
+	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(event);
+	}
+
+	return data_unread;
 }

Re: [PATCHv2] sctp: ABORT if receive queue is not empty while closing socket

[Vladislav Yasevich]

On 07/08/2011 06:57 AM, Thomas Graf wrote:
> Trigger user ABORT if application closes a socket which has data
> queued on the socket receive queue as this would imply data being
> lost which defeats the point of a graceful shutdown.
> 
> This behavior is already practiced in TCP.
> 
> We do not check the input queue because that would mean to parse
> all chunks on it to look for unacknowledged data which seems too
> much of an effort. Control chunks or duplicated chunks may also
> be in the input queue and should not be stopping a graceful
> shutdown.

I think you need to check the ulpq as well.

It is possible to have a condition where you only have data
in the ulpq (imagine a few lost out of order packets or a few lost
fragments from very large messages).  In those circumstances, either fragmentation
or ordering queues may consume all of the window (especially if the buffer was
set small) and you would never trigger the abort.

Since there would never be any notifications in the ulpq (just data), you can
simply use skb_queue_empty() call.

-vlad

> 
> 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 08c6238..17cb3fc 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..8a84017 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)
> -		sctp_ulpevent_free(sctp_skb2event(skb));
> +	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(event);
> +	}
> +
> +	return data_unread;
>  }
> 

Re: [PATCHv2] sctp: ABORT if receive queue is not empty while closing socket

[Thomas Graf]

On Fri, Jul 08, 2011 at 09:49:52AM -0400, Vladislav Yasevich wrote:
> On 07/08/2011 06:57 AM, Thomas Graf wrote:
> > Trigger user ABORT if application closes a socket which has data
> > queued on the socket receive queue as this would imply data being
> > lost which defeats the point of a graceful shutdown.
> > 
> > This behavior is already practiced in TCP.
> > 
> > We do not check the input queue because that would mean to parse
> > all chunks on it to look for unacknowledged data which seems too
> > much of an effort. Control chunks or duplicated chunks may also
> > be in the input queue and should not be stopping a graceful
> > shutdown.
> 
> I think you need to check the ulpq as well.
> 
> It is possible to have a condition where you only have data
> in the ulpq (imagine a few lost out of order packets or a few lost
> fragments from very large messages).  In those circumstances, either fragmentation
> or ordering queues may consume all of the window (especially if the buffer was
> set small) and you would never trigger the abort.

Good point. Updating the patch.

[PATCHv3] sctp: ABORT if receive, reassmbly, or reodering queue is not empty while closing socket

[Thomas Graf]

Trigger user ABORT if application closes a socket which has data
queued on the socket receive queue or chunks waiting on the
reassembly or ordering queue as this would imply data being lost
which defeats the point of a graceful shutdown.

This behavior is already practiced in TCP.

We do not check the input queue because that would mean to parse
all chunks on it to look for unacknowledged data which seems too
much of an effort. Control chunks or duplicated chunks may also
be in the input queue and should not be stopping a graceful
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 08c6238..d3ccf79 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,9 @@ SCTP_STATIC void sctp_close(struct sock *sk, long timeout)
 			}
 		}
 
-		if (sock_flag(sk, SOCK_LINGER) && !sk->sk_lingertime) {
+		if (data_was_unread || !skb_queue_empty(&asoc->ulpq.lobby) ||
+		    !skb_queue_empty(&asoc->ulpq.reasm) ||
+		    (sock_flag(sk, SOCK_LINGER) && !sk->sk_lingertime)) {
 			struct sctp_chunk *chunk;
 
 			chunk = sctp_make_abort_user(asoc, NULL, 0);
@@ -1420,10 +1427,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..8a84017 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)
-		sctp_ulpevent_free(sctp_skb2event(skb));
+	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(event);
+	}
+
+	return data_unread;
 }

Re: [PATCHv3] sctp: ABORT if receive, reassmbly, or reodering queue is not empty while closing socket

[David Miller]

From: Thomas Graf <tgraf@infradead.org>
Date: Fri, 8 Jul 2011 10:37:46 -0400

> Trigger user ABORT if application closes a socket which has data
> queued on the socket receive queue or chunks waiting on the
> reassembly or ordering queue as this would imply data being lost
> which defeats the point of a graceful shutdown.
> 
> This behavior is already practiced in TCP.
> 
> We do not check the input queue because that would mean to parse
> all chunks on it to look for unacknowledged data which seems too
> much of an effort. Control chunks or duplicated chunks may also
> be in the input queue and should not be stopping a graceful
> shutdown.
> 
> Signed-off-by: Thomas Graf <tgraf@infradead.org>

Vlad please ACK/NACK this updated patch.

Thanks.

Re: [PATCHv3] sctp: ABORT if receive, reassmbly, or reodering queue is not empty while closing socket

[Vladislav Yasevich]

On 07/08/2011 10:37 AM, Thomas Graf wrote:
> Trigger user ABORT if application closes a socket which has data
> queued on the socket receive queue or chunks waiting on the
> reassembly or ordering queue as this would imply data being lost
> which defeats the point of a graceful shutdown.
> 
> This behavior is already practiced in TCP.
> 
> We do not check the input queue because that would mean to parse
> all chunks on it to look for unacknowledged data which seems too
> much of an effort. Control chunks or duplicated chunks may also
> be in the input queue and should not be stopping a graceful
> shutdown.
> 
> Signed-off-by: Thomas Graf <tgraf@infradead.org>

Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>

-vlad

> 
> 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 08c6238..d3ccf79 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,9 @@ SCTP_STATIC void sctp_close(struct sock *sk, long timeout)
>  			}
>  		}
>  
> -		if (sock_flag(sk, SOCK_LINGER) && !sk->sk_lingertime) {
> +		if (data_was_unread || !skb_queue_empty(&asoc->ulpq.lobby) ||
> +		    !skb_queue_empty(&asoc->ulpq.reasm) ||
> +		    (sock_flag(sk, SOCK_LINGER) && !sk->sk_lingertime)) {
>  			struct sctp_chunk *chunk;
>  
>  			chunk = sctp_make_abort_user(asoc, NULL, 0);
> @@ -1420,10 +1427,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..8a84017 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)
> -		sctp_ulpevent_free(sctp_skb2event(skb));
> +	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(event);
> +	}
> +
> +	return data_unread;
>  }
> 

Re: [PATCHv3] sctp: ABORT if receive, reassmbly, or reodering queue is not empty while closing socket

[David Miller]

From: Vladislav Yasevich <vladislav.yasevich@hp.com>
Date: Fri, 08 Jul 2011 12:43:59 -0400

> On 07/08/2011 10:37 AM, Thomas Graf wrote:
>> Trigger user ABORT if application closes a socket which has data
>> queued on the socket receive queue or chunks waiting on the
>> reassembly or ordering queue as this would imply data being lost
>> which defeats the point of a graceful shutdown.
>> 
>> This behavior is already practiced in TCP.
>> 
>> We do not check the input queue because that would mean to parse
>> all chunks on it to look for unacknowledged data which seems too
>> much of an effort. Control chunks or duplicated chunks may also
>> be in the input queue and should not be stopping a graceful
>> shutdown.
>> 
>> Signed-off-by: Thomas Graf <tgraf@infradead.org>
> 
> Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>

Applied, thanks.