Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling

[Vlad Yasevich <vyasevich@gmail.com>]

xufeng zhang <xufeng.zhang@windriver.com> wrote:

>On 07/19/2012 01:57 PM, xufengzhang.main@gmail.com wrote:
>> When "Invalid Stream Identifier" ERROR happens after process the
>> received DATA chunks, this ERROR chunk is enqueued into outqueue
>> before SACK chunk, so when bundling ERROR chunk with SACK chunk,
>> the ERROR chunk is always placed first in the packet because of
>> the chunk's position in the outqueue.
>> This violates sctp specification:
>>      RFC 4960 6.5. Stream Identifier and Stream Sequence Number
>>      ...The endpoint may bundle the ERROR chunk in the same
>>      packet as the SACK as long as the ERROR follows the SACK.
>> So we must place SACK first when bundling "Invalid Stream Identifier"
>> ERROR and SACK in one packet.
>> Although we can do that by enqueue SACK chunk into outqueue before
>> ERROR chunk, it will violate the side-effect interpreter processing.
>> It's easy to do this job when dequeue chunks from the outqueue,
>> by this way, we introduce a flag 'has_isi_err' which indicate
>> whether or not the "Invalid Stream Identifier" ERROR happens.
>>
>> Signed-off-by: Xufeng Zhang<xufeng.zhang@windriver.com>
>> ---
>>   include/net/sctp/structs.h |    2 ++
>>   net/sctp/output.c          |   26 ++++++++++++++++++++++++++
>>   2 files changed, 28 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
>> index 88949a9..5adf4de 100644
>> --- a/include/net/sctp/structs.h
>> +++ b/include/net/sctp/structs.h
>> @@ -842,6 +842,8 @@ struct sctp_packet {
>>   	    has_sack:1,		/* This packet contains a SACK chunk. */
>>   	    has_auth:1,		/* This packet contains an AUTH chunk */
>>   	    has_data:1,		/* This packet contains at least 1 DATA chunk */
>> +	    has_isi_err:1,	/* This packet contains a "Invalid Stream
>> +				 * Identifier" ERROR chunk */
>>   	    ipfragok:1,		/* So let ip fragment this packet */
>>   	    malloced:1;		/* Is it malloced? */
>>   };
>> diff --git a/net/sctp/output.c b/net/sctp/output.c
>> index 817174e..77fb1ae 100644
>> --- a/net/sctp/output.c
>> +++ b/net/sctp/output.c
>> @@ -79,6 +79,7 @@ static void sctp_packet_reset(struct sctp_packet
>*packet)
>>   	packet->has_sack = 0;
>>   	packet->has_data = 0;
>>   	packet->has_auth = 0;
>> +	packet->has_isi_err = 0;
>>   	packet->ipfragok = 0;
>>   	packet->auth = NULL;
>>   }
>> @@ -267,6 +268,7 @@ static sctp_xmit_t sctp_packet_bundle_sack(struct
>sctp_packet *pkt,
>>   sctp_xmit_t sctp_packet_append_chunk(struct sctp_packet *packet,
>>   				     struct sctp_chunk *chunk)
>>   {
>> +	struct sctp_chunk *lchunk;
>>   	sctp_xmit_t retval = SCTP_XMIT_OK;
>>   	__u16 chunk_len = WORD_ROUND(ntohs(chunk->chunk_hdr->length));
>>
>> @@ -316,7 +318,31 @@ sctp_xmit_t sctp_packet_append_chunk(struct
>sctp_packet *packet,
>>   		packet->has_cookie_echo = 1;
>>   		break;
>>
>> +	    case SCTP_CID_ERROR:
>> +		if (chunk->subh.err_hdr->cause&  SCTP_ERROR_INV_STRM)
>> +			packet->has_isi_err = 1;
>> +		break;
>> +
>>   	    case SCTP_CID_SACK:
>> +		/* RFC 4960
>> +		 * 6.5 Stream Identifier and Stream Sequence Number
>> +		 * The endpoint may bundle the ERROR chunk in the same
>> +		 * packet as the SACK as long as the ERROR follows the SACK.
>> +		 */
>> +		if (packet->has_isi_err) {
>> +			if (list_is_singular(&packet->chunk_list))
>> +				list_add(&chunk->list,&packet->chunk_list);
>> +			else {
>> +				lchunk = list_first_entry(&packet->chunk_list,
>> +						struct sctp_chunk, list);
>> +				list_add(&chunk->list,&lchunk->list);
>> +			}
>>    
>And I should clarify the above judgment code.
>AFAIK, there should be two cases for the bundling when invalid stream 
>identifier error happens:
>1). COOKIE_ACK ERROR SACK
>2). ERROR SACK
>So I need to deal with the two cases differently.
>

Sorry but I just don't buy that the above are the only 2 cases.  What if there are addip chunks as well?  What if there are some other extensions also.  This code has to be generic enough to handle any condition.

- vlad

>
>Thanks,
>Xufeng Zhang
>> +			packet->size += chunk_len;
>> +			chunk->transport = packet->transport;
>> +			packet->has_sack = 1;
>> +			goto finish;
>> +		}
>> +
>>   		packet->has_sack = 1;
>>   		break;
>>
>>    


-- 
Sent from my Android phone with SkitMail. Please excuse my brevity.