lkml.org 
[lkml]   [2012]   [Jul]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [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:
    > >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.


    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
    > >Regards
    > >Neil
    > >
    > >
    >


    \
     
     \ /
      Last update: 2012-07-24 14:21    [W:6.821 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site