[PATCH 2/2] sctp: COOKIE-ACK should back to where the COOKIE-ECHO

[PATCH 2/2] sctp: COOKIE-ACK should back to where the COOKIE-ECHO

[Wei Yongjun]

An endpoint SHOULD transmit reply chunks (e.g., SACK, HEARTBEAT ACK,
etc.) to the same destination transport address from which it
received the DATA or control chunk to which it is replying.

But if endpoint is multi-homed, retransmit COOKIE-ECHO will cause
COOKIE-ACK be sent back to the destination transport address from
which the INIT chunk is received. This patch fixed it while
retransmit COOKIE-ECHO is because of either COOKIE-ECHO lost or
COOKIE-ACK lost.

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
 net/sctp/sm_statefuns.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index abf601a..e95f6e5 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -724,10 +724,15 @@ sctp_disposition_t sctp_sf_do_5_1D_ce(const struct sctp_endpoint *ep,
 	peer_init = &chunk->subh.cookie_hdr->c.peer_init[0];
 
 	if (!sctp_process_init(new_asoc, chunk->chunk_hdr->type,
-			       &chunk->subh.cookie_hdr->c.peer_addr,
+			       sctp_source(chunk),
 			       peer_init, GFP_ATOMIC))
 		goto nomem_init;
 
+	/* The peer's original address may not appear in address parameters */
+	if (!sctp_assoc_add_peer(new_asoc, &chunk->subh.cookie_hdr->c.peer_addr,
+				 GFP_ATOMIC, SCTP_UNCONFIRMED))
+		goto nomem_init;
+
 	/* SCTP-AUTH:  Now that we've populate required fields in
 	 * sctp_process_init, set up the assocaition shared keys as
 	 * necessary so that we can potentially authenticate the ACK
@@ -1914,6 +1919,9 @@ static sctp_disposition_t sctp_sf_do_dupcook_d(const struct sctp_endpoint *ep,
 		}
 	}
 
+	sctp_add_cmd_sf(commands, SCTP_CMD_TRANSPORT_ON,
+			SCTP_TRANSPORT(chunk->transport));
+
 	repl = sctp_make_cookie_ack(new_asoc, chunk);
 	if (!repl)
 		goto nomem;
-- 
1.6.5.2

Re: [PATCH 2/2] sctp: COOKIE-ACK should back to where the COOKIE-ECHO

[Vlad Yasevich]

Wei Yongjun wrote:
> An endpoint SHOULD transmit reply chunks (e.g., SACK, HEARTBEAT ACK,
> etc.) to the same destination transport address from which it
> received the DATA or control chunk to which it is replying.
> 
> But if endpoint is multi-homed, retransmit COOKIE-ECHO will cause
> COOKIE-ACK be sent back to the destination transport address from
> which the INIT chunk is received. This patch fixed it while
> retransmit COOKIE-ECHO is because of either COOKIE-ECHO lost or
> COOKIE-ACK lost.
> 
> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
> ---
>  net/sctp/sm_statefuns.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index abf601a..e95f6e5 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -724,10 +724,15 @@ sctp_disposition_t sctp_sf_do_5_1D_ce(const struct sctp_endpoint *ep,
>  	peer_init = &chunk->subh.cookie_hdr->c.peer_init[0];
>  
>  	if (!sctp_process_init(new_asoc, chunk->chunk_hdr->type,
> -			       &chunk->subh.cookie_hdr->c.peer_addr,
> +			       sctp_source(chunk),
>  			       peer_init, GFP_ATOMIC))
>  		goto nomem_init;
>  
> +	/* The peer's original address may not appear in address parameters */
> +	if (!sctp_assoc_add_peer(new_asoc, &chunk->subh.cookie_hdr->c.peer_addr,
> +				 GFP_ATOMIC, SCTP_UNCONFIRMED))
> +		goto nomem_init;
> +

Actually, the interesting thing here is that here is what rfc 4960 states

5.4.  Path Verification
...
 the following rules are applied to all addresses of the
   new association:

   1)  Any address passed to the sender of the INIT by its upper layer
      is automatically considered to be CONFIRMED.

   2)  For the receiver of the COOKIE ECHO, the only CONFIRMED address
      is the one to which the INIT-ACK was sent.

   3)  All other addresses not covered by rules 1 and 2 are considered
      UNCONFIRMED and are subject to probing for verification.


The proper way to solve this is to implement the following text:

   -  A COOKIE ACK MAY be sent to an UNCONFIRMED address, but it MUST be
      bundled with a HEARTBEAT including a nonce.  An implementation
      that does NOT support bundling MUST NOT send a COOKIE ACK to an
      UNCONFIRMED address.

   -  A COOKIE ECHO MAY be sent to an UNCONFIRMED address, but it MUST
      be bundled with a HEARTBEAT including a nonce, and the packet MUST
      NOT exceed the path MTU.  If the implementation does NOT support
      bundling or if the bundled COOKIE ECHO plus HEARTBEAT (including
      nonce) would exceed the path MTU, then the implementation MUST NOT
      send a COOKIE ECHO to an UNCONFIRMED address.

>  	/* SCTP-AUTH:  Now that we've populate required fields in
>  	 * sctp_process_init, set up the assocaition shared keys as
>  	 * necessary so that we can potentially authenticate the ACK
> @@ -1914,6 +1919,9 @@ static sctp_disposition_t sctp_sf_do_dupcook_d(const struct sctp_endpoint *ep,
>  		}
>  	}
>  
> +	sctp_add_cmd_sf(commands, SCTP_CMD_TRANSPORT_ON,
> +			SCTP_TRANSPORT(chunk->transport));
> +

sctp_cmd_transport_on() assumes that the chunk passed to it is a HB chunk.
This is not the right thing to do.  I am truly amazed that this worked for you.

-vlad

>  	repl = sctp_make_cookie_ack(new_asoc, chunk);
>  	if (!repl)
>  		goto nomem;

[PATCH 2/2] sctp: COOKIE-ACK should back to where the COOKIE-ECHO

[Wei Yongjun]

This patch fixed to send COOKIE-ACK back to where the COOKIE-ECHO
came from, and if COOKIE ACK is sent to an UNCONFIRMED address,
bundled it with a HEARTBEAT including a nonce. Based on RFC4960:

  Section 6.4.  Multi-Homed SCTP Endpoints

  An endpoint SHOULD transmit reply chunks (e.g., SACK, HEARTBEAT ACK,
  etc.) to the same destination transport address from which it
  received the DATA or control chunk to which it is replying.

  Section 5.4.  Path Verification:

  -  A COOKIE ACK MAY be sent to an UNCONFIRMED address, but it MUST be
     bundled with a HEARTBEAT including a nonce.  An implementation
     that does NOT support bundling MUST NOT send a COOKIE ACK to an
     UNCONFIRMED address.

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
 net/sctp/output.c        |   22 ++++++++++++++++++++++
 net/sctp/outqueue.c      |    1 +
 net/sctp/sm_make_chunk.c |    3 +++
 3 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/net/sctp/output.c b/net/sctp/output.c
index fad261d..a68a57d 100644
--- a/net/sctp/output.c
+++ b/net/sctp/output.c
@@ -260,6 +260,25 @@ static sctp_xmit_t sctp_packet_bundle_sack(struct sctp_packet *pkt,
 	return retval;
 }
 
+/* Try to bundle a HEARTBEAT with the packet. */
+static sctp_xmit_t sctp_packet_bundle_hb(struct sctp_packet *pkt,
+					 struct sctp_chunk *chunk)
+{
+	sctp_xmit_t retval = SCTP_XMIT_OK;
+
+	if (pkt->transport->state = SCTP_UNCONFIRMED &&
+	    chunk->chunk_hdr->type = SCTP_CID_COOKIE_ACK) {
+		struct sctp_transport *transport = pkt->transport;
+		struct sctp_chunk *hb;
+
+		hb = sctp_make_heartbeat(transport->asoc, transport);
+		if (hb)
+			retval = sctp_packet_append_chunk(pkt, hb);
+	}
+
+	return retval;
+}
+
 /* Append a chunk to the offered packet reporting back any inability to do
  * so.
  */
@@ -329,6 +348,9 @@ sctp_xmit_t sctp_packet_append_chunk(struct sctp_packet *packet,
 	list_add_tail(&chunk->list, &packet->chunk_list);
 	packet->size += chunk_len;
 	chunk->transport = packet->transport;
+
+	/* Try to bundle HEARTBEAT chunk */
+	retval = sctp_packet_bundle_hb(packet, chunk);
 finish:
 	return retval;
 }
diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
index abfc0b8..322ecd1 100644
--- a/net/sctp/outqueue.c
+++ b/net/sctp/outqueue.c
@@ -788,6 +788,7 @@ static int sctp_outq_flush(struct sctp_outq *q, int rtx_timeout)
 			 */
 			if (chunk->chunk_hdr->type != SCTP_CID_HEARTBEAT &&
 			    chunk->chunk_hdr->type != SCTP_CID_HEARTBEAT_ACK &&
+			    chunk->chunk_hdr->type != SCTP_CID_COOKIE_ACK &&
 			    chunk->chunk_hdr->type != SCTP_CID_ASCONF_ACK)
 				new_transport = asoc->peer.active_path;
 		}
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 04c45dd..b9f08f0 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -584,6 +584,9 @@ struct sctp_chunk *sctp_make_cookie_ack(const struct sctp_association *asoc,
 	if (retval && chunk)
 		retval->transport = chunk->transport;
 
+	if (retval && !retval->transport)
+		retval->dest = chunk->source;
+
 	return retval;
 }
 
-- 
1.6.5.2

Re: [PATCH 2/2] sctp: COOKIE-ACK should back to where the COOKIE-ECHO

[Vlad Yasevich]

Hi Wei

Wei Yongjun wrote:
> This patch fixed to send COOKIE-ACK back to where the COOKIE-ECHO
> came from, and if COOKIE ACK is sent to an UNCONFIRMED address,
> bundled it with a HEARTBEAT including a nonce. Based on RFC4960:
> 
>   Section 6.4.  Multi-Homed SCTP Endpoints
> 
>   An endpoint SHOULD transmit reply chunks (e.g., SACK, HEARTBEAT ACK,
>   etc.) to the same destination transport address from which it
>   received the DATA or control chunk to which it is replying.
> 
>   Section 5.4.  Path Verification:
> 
>   -  A COOKIE ACK MAY be sent to an UNCONFIRMED address, but it MUST be
>      bundled with a HEARTBEAT including a nonce.  An implementation
>      that does NOT support bundling MUST NOT send a COOKIE ACK to an
>      UNCONFIRMED address.
> 
> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
> ---
>  net/sctp/output.c        |   22 ++++++++++++++++++++++
>  net/sctp/outqueue.c      |    1 +
>  net/sctp/sm_make_chunk.c |    3 +++
>  3 files changed, 26 insertions(+), 0 deletions(-)
> 
> diff --git a/net/sctp/output.c b/net/sctp/output.c
> index fad261d..a68a57d 100644
> --- a/net/sctp/output.c
> +++ b/net/sctp/output.c
> @@ -260,6 +260,25 @@ static sctp_xmit_t sctp_packet_bundle_sack(struct sctp_packet *pkt,
>  	return retval;
>  }
>  
> +/* Try to bundle a HEARTBEAT with the packet. */
> +static sctp_xmit_t sctp_packet_bundle_hb(struct sctp_packet *pkt,
> +					 struct sctp_chunk *chunk)
> +{
> +	sctp_xmit_t retval = SCTP_XMIT_OK;
> +
> +	if (pkt->transport->state = SCTP_UNCONFIRMED &&
> +	    chunk->chunk_hdr->type = SCTP_CID_COOKIE_ACK) {
> +		struct sctp_transport *transport = pkt->transport;
> +		struct sctp_chunk *hb;
> +
> +		hb = sctp_make_heartbeat(transport->asoc, transport);
> +		if (hb)
> +			retval = sctp_packet_append_chunk(pkt, hb);
> +	}
> +
> +	return retval;
> +}
> +
>  /* Append a chunk to the offered packet reporting back any inability to do
>   * so.
>   */
> @@ -329,6 +348,9 @@ sctp_xmit_t sctp_packet_append_chunk(struct sctp_packet *packet,
>  	list_add_tail(&chunk->list, &packet->chunk_list);
>  	packet->size += chunk_len;
>  	chunk->transport = packet->transport;
> +
> +	/* Try to bundle HEARTBEAT chunk */
> +	retval = sctp_packet_bundle_hb(packet, chunk);

Not sure if this is the right place.  Also, this will add the HB after cookie-ACK.
I am wondering if it should go before.   Seems a more correct thing to do is
to elicit HB-ACK before any further data might flow over the transport.

Also, what about all the HB related things like error counters, timers, etc...
Seems that we should also do that.

May be we can just cue the HB to the control queue if we are not replying to the
source of INIT-ACK?

>  finish:
>  	return retval;
>  }
> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
> index abfc0b8..322ecd1 100644
> --- a/net/sctp/outqueue.c
> +++ b/net/sctp/outqueue.c
> @@ -788,6 +788,7 @@ static int sctp_outq_flush(struct sctp_outq *q, int rtx_timeout)
>  			 */
>  			if (chunk->chunk_hdr->type != SCTP_CID_HEARTBEAT &&
>  			    chunk->chunk_hdr->type != SCTP_CID_HEARTBEAT_ACK &&
> +			    chunk->chunk_hdr->type != SCTP_CID_COOKIE_ACK &&
>  			    chunk->chunk_hdr->type != SCTP_CID_ASCONF_ACK)
>  				new_transport = asoc->peer.active_path;
>  		}
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index 04c45dd..b9f08f0 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -584,6 +584,9 @@ struct sctp_chunk *sctp_make_cookie_ack(const struct sctp_association *asoc,
>  	if (retval && chunk)
>  		retval->transport = chunk->transport;
>  
> +	if (retval && !retval->transport)
> +		retval->dest = chunk->source;
> +

This one is good, but I am thinking that transport assignment may be incorrect.
 Like I said before, when we are restarting, the transport assigned is from the
existing association, but the packet is transmitted on the temporary association
that's about to vanish.  Not sure if this is ok.


>  	return retval;
>  }
>  

Re: [PATCH 2/2] sctp: COOKIE-ACK should back to where the COOKIE-ECHO

[Wei Yongjun]

> Hi Wei
>
> Wei Yongjun wrote:
>   
>> This patch fixed to send COOKIE-ACK back to where the COOKIE-ECHO
>> came from, and if COOKIE ACK is sent to an UNCONFIRMED address,
>> bundled it with a HEARTBEAT including a nonce. Based on RFC4960:
>>
>>   Section 6.4.  Multi-Homed SCTP Endpoints
>>
>>   An endpoint SHOULD transmit reply chunks (e.g., SACK, HEARTBEAT ACK,
>>   etc.) to the same destination transport address from which it
>>   received the DATA or control chunk to which it is replying.
>>
>>   Section 5.4.  Path Verification:
>>
>>   -  A COOKIE ACK MAY be sent to an UNCONFIRMED address, but it MUST be
>>      bundled with a HEARTBEAT including a nonce.  An implementation
>>      that does NOT support bundling MUST NOT send a COOKIE ACK to an
>>      UNCONFIRMED address.
>>
>> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
>> ---
>>  net/sctp/output.c        |   22 ++++++++++++++++++++++
>>  net/sctp/outqueue.c      |    1 +
>>  net/sctp/sm_make_chunk.c |    3 +++
>>  3 files changed, 26 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/sctp/output.c b/net/sctp/output.c
>> index fad261d..a68a57d 100644
>> --- a/net/sctp/output.c
>> +++ b/net/sctp/output.c
>> @@ -260,6 +260,25 @@ static sctp_xmit_t sctp_packet_bundle_sack(struct sctp_packet *pkt,
>>  	return retval;
>>  }
>>  
>> +/* Try to bundle a HEARTBEAT with the packet. */
>> +static sctp_xmit_t sctp_packet_bundle_hb(struct sctp_packet *pkt,
>> +					 struct sctp_chunk *chunk)
>> +{
>> +	sctp_xmit_t retval = SCTP_XMIT_OK;
>> +
>> +	if (pkt->transport->state = SCTP_UNCONFIRMED &&
>> +	    chunk->chunk_hdr->type = SCTP_CID_COOKIE_ACK) {
>> +		struct sctp_transport *transport = pkt->transport;
>> +		struct sctp_chunk *hb;
>> +
>> +		hb = sctp_make_heartbeat(transport->asoc, transport);
>> +		if (hb)
>> +			retval = sctp_packet_append_chunk(pkt, hb);
>> +	}
>> +
>> +	return retval;
>> +}
>> +
>>  /* Append a chunk to the offered packet reporting back any inability to do
>>   * so.
>>   */
>> @@ -329,6 +348,9 @@ sctp_xmit_t sctp_packet_append_chunk(struct sctp_packet *packet,
>>  	list_add_tail(&chunk->list, &packet->chunk_list);
>>  	packet->size += chunk_len;
>>  	chunk->transport = packet->transport;
>> +
>> +	/* Try to bundle HEARTBEAT chunk */
>> +	retval = sctp_packet_bundle_hb(packet, chunk);
>>     
> Not sure if this is the right place.  Also, this will add the HB after cookie-ACK.
> I am wondering if it should go before.   Seems a more correct thing to do is
> to elicit HB-ACK before any further data might flow over the transport.
>   

I add the HB after cookie-ACK is because that the COOKIE-ECHO on
UNCONFIRMED address must also be bundled with a HEARTBEAT.
If HB is before COOKIE-ECHO, the peer is in CLOSED state and the
HB will be treat as OOTB chunk. And I think COOKIE-ACK need to
do the same thing.


> Also, what about all the HB related things like error counters, timers, etc...
> Seems that we should also do that.
>   

Not sure about that, the first HB may be sent then RTO is expired.
Do this may change the orig HB's action.


> May be we can just cue the HB to the control queue if we are not replying to the
> source of INIT-ACK?
>   

If the state is UNCONFIRMED, the transport should not be the source
of INIT-ACK, since INIT-ACK is sent to the source of INIT, and it should
be in ACTIVE state.


>   
>>  finish:
>>  	return retval;
>>  }
>> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
>> index abfc0b8..322ecd1 100644
>> --- a/net/sctp/outqueue.c
>> +++ b/net/sctp/outqueue.c
>> @@ -788,6 +788,7 @@ static int sctp_outq_flush(struct sctp_outq *q, int rtx_timeout)
>>  			 */
>>  			if (chunk->chunk_hdr->type != SCTP_CID_HEARTBEAT &&
>>  			    chunk->chunk_hdr->type != SCTP_CID_HEARTBEAT_ACK &&
>> +			    chunk->chunk_hdr->type != SCTP_CID_COOKIE_ACK &&
>>  			    chunk->chunk_hdr->type != SCTP_CID_ASCONF_ACK)
>>  				new_transport = asoc->peer.active_path;
>>  		}
>> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
>> index 04c45dd..b9f08f0 100644
>> --- a/net/sctp/sm_make_chunk.c
>> +++ b/net/sctp/sm_make_chunk.c
>> @@ -584,6 +584,9 @@ struct sctp_chunk *sctp_make_cookie_ack(const struct sctp_association *asoc,
>>  	if (retval && chunk)
>>  		retval->transport = chunk->transport;
>>  
>> +	if (retval && !retval->transport)
>> +		retval->dest = chunk->source;
>> +
>>     
> This one is good, but I am thinking that transport assignment may be incorrect.
>  Like I said before, when we are restarting, the transport assigned is from the
> existing association, but the packet is transmitted on the temporary association
> that's about to vanish.  Not sure if this is ok.
>   

It can works, but may be the transport should not be set, the only
thing we should do is set the retval->dest.


>
>   
>>  	return retval;
>>  }
>>  
>>     
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>   

Re: [PATCH 2/2] sctp: COOKIE-ACK should back to where the COOKIE-ECHO

[Vlad Yasevich]

Wei Yongjun wrote:
>> Hi Wei
>>
>> Wei Yongjun wrote:
>>   
>>> This patch fixed to send COOKIE-ACK back to where the COOKIE-ECHO
>>> came from, and if COOKIE ACK is sent to an UNCONFIRMED address,
>>> bundled it with a HEARTBEAT including a nonce. Based on RFC4960:
>>>
>>>   Section 6.4.  Multi-Homed SCTP Endpoints
>>>
>>>   An endpoint SHOULD transmit reply chunks (e.g., SACK, HEARTBEAT ACK,
>>>   etc.) to the same destination transport address from which it
>>>   received the DATA or control chunk to which it is replying.
>>>
>>>   Section 5.4.  Path Verification:
>>>
>>>   -  A COOKIE ACK MAY be sent to an UNCONFIRMED address, but it MUST be
>>>      bundled with a HEARTBEAT including a nonce.  An implementation
>>>      that does NOT support bundling MUST NOT send a COOKIE ACK to an
>>>      UNCONFIRMED address.
>>>
>>> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
>>> ---
>>>  net/sctp/output.c        |   22 ++++++++++++++++++++++
>>>  net/sctp/outqueue.c      |    1 +
>>>  net/sctp/sm_make_chunk.c |    3 +++
>>>  3 files changed, 26 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/net/sctp/output.c b/net/sctp/output.c
>>> index fad261d..a68a57d 100644
>>> --- a/net/sctp/output.c
>>> +++ b/net/sctp/output.c
>>> @@ -260,6 +260,25 @@ static sctp_xmit_t sctp_packet_bundle_sack(struct sctp_packet *pkt,
>>>  	return retval;
>>>  }
>>>  
>>> +/* Try to bundle a HEARTBEAT with the packet. */
>>> +static sctp_xmit_t sctp_packet_bundle_hb(struct sctp_packet *pkt,
>>> +					 struct sctp_chunk *chunk)
>>> +{
>>> +	sctp_xmit_t retval = SCTP_XMIT_OK;
>>> +
>>> +	if (pkt->transport->state = SCTP_UNCONFIRMED &&
>>> +	    chunk->chunk_hdr->type = SCTP_CID_COOKIE_ACK) {
>>> +		struct sctp_transport *transport = pkt->transport;
>>> +		struct sctp_chunk *hb;
>>> +
>>> +		hb = sctp_make_heartbeat(transport->asoc, transport);
>>> +		if (hb)
>>> +			retval = sctp_packet_append_chunk(pkt, hb);
>>> +	}
>>> +
>>> +	return retval;
>>> +}
>>> +
>>>  /* Append a chunk to the offered packet reporting back any inability to do
>>>   * so.
>>>   */
>>> @@ -329,6 +348,9 @@ sctp_xmit_t sctp_packet_append_chunk(struct sctp_packet *packet,
>>>  	list_add_tail(&chunk->list, &packet->chunk_list);
>>>  	packet->size += chunk_len;
>>>  	chunk->transport = packet->transport;
>>> +
>>> +	/* Try to bundle HEARTBEAT chunk */
>>> +	retval = sctp_packet_bundle_hb(packet, chunk);
>>>     
>> Not sure if this is the right place.  Also, this will add the HB after cookie-ACK.
>> I am wondering if it should go before.   Seems a more correct thing to do is
>> to elicit HB-ACK before any further data might flow over the transport.
>>   
> 
> I add the HB after cookie-ACK is because that the COOKIE-ECHO on
> UNCONFIRMED address must also be bundled with a HEARTBEAT.
> If HB is before COOKIE-ECHO, the peer is in CLOSED state and the
> HB will be treat as OOTB chunk. And I think COOKIE-ACK need to
> do the same thing.

Not really.  Heartbeats are allows in the COOKIE_ECHOED state.  See section
8.3.

So, it would be perfectly valid to include a HB first and attach a cookie-ack
to it.


> 
> 
>> Also, what about all the HB related things like error counters, timers, etc...
>> Seems that we should also do that.
>>   
> 
> Not sure about that, the first HB may be sent then RTO is expired.
> Do this may change the orig HB's action.
> 

Well, consider this scenario. If we send a HB+Cookie_ACK to the destination and
it gets lost, we wouldn't count this as a strike against the destination, while
in reality, it is.

Looks like the timers are taken care of, but a stike should be recorded.

-vlad
> 
>> May be we can just cue the HB to the control queue if we are not replying to the
>> source of INIT-ACK?
>>   
> 
> If the state is UNCONFIRMED, the transport should not be the source
> of INIT-ACK, since INIT-ACK is sent to the source of INIT, and it should
> be in ACTIVE state.
> 
> 
>>   
>>>  finish:
>>>  	return retval;
>>>  }
>>> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
>>> index abfc0b8..322ecd1 100644
>>> --- a/net/sctp/outqueue.c
>>> +++ b/net/sctp/outqueue.c
>>> @@ -788,6 +788,7 @@ static int sctp_outq_flush(struct sctp_outq *q, int rtx_timeout)
>>>  			 */
>>>  			if (chunk->chunk_hdr->type != SCTP_CID_HEARTBEAT &&
>>>  			    chunk->chunk_hdr->type != SCTP_CID_HEARTBEAT_ACK &&
>>> +			    chunk->chunk_hdr->type != SCTP_CID_COOKIE_ACK &&
>>>  			    chunk->chunk_hdr->type != SCTP_CID_ASCONF_ACK)
>>>  				new_transport = asoc->peer.active_path;
>>>  		}
>>> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
>>> index 04c45dd..b9f08f0 100644
>>> --- a/net/sctp/sm_make_chunk.c
>>> +++ b/net/sctp/sm_make_chunk.c
>>> @@ -584,6 +584,9 @@ struct sctp_chunk *sctp_make_cookie_ack(const struct sctp_association *asoc,
>>>  	if (retval && chunk)
>>>  		retval->transport = chunk->transport;
>>>  
>>> +	if (retval && !retval->transport)
>>> +		retval->dest = chunk->source;
>>> +
>>>     
>> This one is good, but I am thinking that transport assignment may be incorrect.
>>  Like I said before, when we are restarting, the transport assigned is from the
>> existing association, but the packet is transmitted on the temporary association
>> that's about to vanish.  Not sure if this is ok.
>>   
> 
> It can works, but may be the transport should not be set, the only
> thing we should do is set the retval->dest.
> 
> 
>>   
>>>  	return retval;
>>>  }
>>>  
>>>     
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>>   
>