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

On Tue, Jul 24, 2012 at 09:50:18AM +0800, xufeng zhang wrote:

On 07/23/2012 08:14 PM, Neil Horman wrote:

quoted

On Mon, Jul 23, 2012 at 10:30:34AM +0800, xufeng zhang wrote:

quoted

On 07/23/2012 08:49 AM, Neil Horman wrote:

quoted

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.

diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
index e7aa177c..eeac32f 100644
--- a/net/sctp/outqueue.c
+++ b/net/sctp/outqueue.c

@@ -300,7 +300,7 @@ void sctp_outq_free(struct sctp_outq *q)
 int sctp_outq_tail(struct sctp_outq *q, struct sctp_chunk *chunk)
 {
 	int error = 0;
-
+	struct sctp_chunk *cptr;
 	SCTP_DEBUG_PRINTK("sctp_outq_tail(%p, %p[%s])\n",
 			  q, chunk, chunk && chunk->chunk_hdr ?
 			  sctp_cname(SCTP_ST_CHUNK(chunk->chunk_hdr->type))

@@ -344,7 +344,21 @@ int sctp_outq_tail(struct sctp_outq *q, struct sctp_chunk *chunk)
 			break;
 		}
 	} else {
-		list_add_tail(&chunk->list, &q->control_chunk_list);
+		list_del_init(&chunk->list);
+		if (chunk->chunk_hdr->type == SCTP_CID_COOKIE_ACK)
+			list_add_head(&chunk->list, &q->control_chunk_list);
+		else if (!list_empty(&q->control_chunk_list) &&
+		    chunk->chunk_hdr->type == SCTP_CID_SACK) {
+			list_for_each_entry(cptr, &q->control_chunk_list, list) {
+				if (cptr->chunk_hdr->type == SCTP_CID_COOKIE_ACK)
+					continue;
+				list_add(&chunk->list, &cptr->list);
+				break;
+			}
+		} 
+
+		if (list_empty(&chunk->list))
+			list_add_tail(&chunk->list, &q->control_chunk_list);
 		SCTP_INC_STATS(SCTP_MIB_OUTCTRLCHUNKS);
 	}
 

Thanks,
Xufeng Zhang

quoted

Regards
Neil

On 7/24/12, Neil Horman [off-list ref] wrote:

On Tue, Jul 24, 2012 at 09:50:18AM +0800, xufeng zhang wrote:

quoted

On 07/23/2012 08:14 PM, Neil Horman wrote:

quoted

On Mon, Jul 23, 2012 at 10:30:34AM +0800, xufeng zhang wrote:

quoted

On 07/23/2012 08:49 AM, Neil Horman wrote:

quoted

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.


Thanks,
Xufeng Zhang

On Wed, Jul 25, 2012 at 10:34:32AM +0800, Xufeng Zhang wrote:

On 7/24/12, Neil Horman [off-list ref] wrote:

quoted

On Tue, Jul 24, 2012 at 09:50:18AM +0800, xufeng zhang wrote:

quoted

On 07/23/2012 08:14 PM, Neil Horman wrote:

quoted

On Mon, Jul 23, 2012 at 10:30:34AM +0800, xufeng zhang wrote:

quoted

On 07/23/2012 08:49 AM, Neil Horman wrote:

quoted

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