On Wed, Jul 25, 2012 at 10:34:32AM +0800, Xufeng Zhang wrote: > On 7/24/12, Neil Horman <nhorman@xxxxxxxxxxxxx> wrote: > > On Tue, Jul 24, 2012 at 09:50:18AM +0800, xufeng zhang wrote: > >> On 07/23/2012 08:14 PM, Neil Horman 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. > >> 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. > >> > > Ok, Fair point, but that just changes the ordering a bit to: > > COOKIE_ACK > > SACK > > OTHER CONTROL CHUNKS > > > > What about something like this? Its completely untested, and I'm sure it > > can be > > cleaned up a bunch, but this keeps us from having to add additional state to > > the > > packet structure. > Yeah! I like this modification, thank you very much for your work! > I'll try to send a V2 patch based on your changes and run some tests. > Awesome, thank you! Neil -- To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
