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

[Xufeng Zhang]

On 7/23/12, Neil Horman <nhorman@xxxxxxxxxxxxx> wrote:
> On Mon, Jul 23, 2012 at 10:30:34AM +0800, xufeng zhang wrote:
>> On 07/23/2012 08:49 AM, Neil Horman wrote:
>> >
>> >Not sure I understand how you came into this error.  If we get an
>> > invalid
>> >stream, we issue an SCTP_REPORT_TSN side effect, followed by an
>> > SCTP_CMD_REPLY
>> >which sends the error chunk.  The reply goes through
>> >sctp_outq_tail->sctp_outq_chunk->sctp_outq_transmit_chunk->sctp_outq_append_chunk.
>> >That last function checks to see if a sack is already part of the packet,
>> > and if
>> >there isn't one, appends one, using the updated tsn map.
>> Yes, you are right, but consider the invalid stream identifier's
>> DATA chunk is the first
>> DATA chunk in the association which will need SACK immediately.
>> Here is what I thought of the scenario:
>>     sctp_sf_eat_data_6_2()
>>         -->sctp_eat_data()
>>             -->sctp_make_op_error()
>>             -->sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(err))
>>             -->sctp_outq_tail()          /* First enqueue ERROR chunk */
>>         -->sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, SCTP_FORCE())
>>             -->sctp_gen_sack()
>>                 -->sctp_make_sack()
>>                 -->sctp_add_cmd_sf(commands, SCTP_CMD_REPLY,
>> SCTP_CHUNK(sack))
>>                 -->sctp_outq_tail()          /* Then enqueue SACK chunk
>> */
>>
>> So SACK chunk is enqueued after ERROR chunk.
> Ah, I see.  Since the ERROR and SACK chunks are both control chunks, and
> since
> we explicitly add the SACK to the control queue instead of going through
> the
> bundle path in sctp_packet_append_chunk the ordering gets wrong.
>
> Ok, so the problem makes sense.  I think the soultion could be alot easier
> though.  IIRC SACK chunks always live at the head of a packet, so why not
> just
> special case it in sctp_outq_tail?  I.e. instead of doing a list_add_tail,
> in
> the else clause of sctp_outq_tail check the chunk_hdr->type to see if its
> SCTP_CID_SACK.  If it is, use list_add_head rather than list_add_tail.  I
> think
> that will fix up both the COOKIE_ECHO and ESTABLISHED cases, won't it?  And
> then
> you won't have keep track of extra state in the packet configuration.

(Please ignore the duplicate messages if you received, sorry for this!)

Yes, it's a good idea, but I think the premise is not correct:
RFC 4960 page 57:
"D) Upon reception of the COOKIE ECHO chunk, endpoint "Z" will reply
   with a COOKIE ACK chunk after building a TCB and moving to the
   ESTABLISHED state. A COOKIE ACK chunk may be bundled with any
   pending DATA chunks (and/or SACK chunks), but the COOKIE ACK chunk
   MUST be the first chunk in the packet."

So we can't put SACK chunk always at the head of the packet.


Thanks,
Xufeng Zhang

>
> Regards
> Neil
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/