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/24/2012 10:27 AM, Vlad Yasevich wrote:
>> 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.
>>    
>Aha, you are right, this may happens.
>So I think the general solution is to fix this problem in the enqueue
>side.
>What do you think? any better suggestion!
>

Don't have code in front of me but what if we carry the error condition to where we queue the Sack and add the error side effect then?

-vlad

>
>Thanks,
>Xufeng Zhang
>> - 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.