[PATCH] SYN issue

[George Neville-Neil]

On May 19, 2009, at 17:13 , Zachary Loafman wrote:

> net@ -
>
> A short patch attached that requires 3 paragraphs of explanation.
>
> We found an issue in TCP when the a client connects to our server,
> establishes a connection, reboots and chooses the same source port to
> re-establish the connection. This isn't hard from other vendors'
> clients. On Solaris, the same NFS mount order at boot time will
> frequently result in source port re-use for the NFS connections.  In
> this case, the customer was seeing mounts hang until the keepalive on
> our side would kick the established connection.
>
> The problem in the code is probably best explained using the patch
> itself:
>
> ---
> Index: sys/netinet/tcp_input.c
> ===================================================================
> --- sys/netinet/tcp_input.c     (revision xxxx)
> +++ sys/netinet/tcp_input.c     (working copy)
> @@ -1818,7 +1818,11 @@ tcp_do_segment(struct mbuf *m, struct tc
>
>        todrop = tp->rcv_nxt - th->th_seq;
>        if (todrop > 0) {
> -               if (thflags & TH_SYN) {
> +               /*
> +                * If this is a duplicate SYN for our current  
> connection,
> +                * advance over it and pretend and it's not a SYN.
> +                */
> +               if (thflags & TH_SYN && th->th_seq == tp->irs) {
>                        thflags &= ~TH_SYN;
>                        th->th_seq++;
>                        if (th->th_urp > 1)
> ---
>
> The problem is that when our TCP stack gets a SYN packet for a
> connection that's already in ESTABLISHED state, it runs through the
> above code. The above code is basically noticing that the packet is
> coming in left of the receive window and then saying "Ah, a SYN! This
> must be a duplicate SYN for our existing connect." After that, it just
> turns off SYN and treats it as a normal packet (after advancing past
> the SYN seq number). The code is broken, though: the only condition
> under which this is a duplicate SYN is if the th_seq matches the irs,
> the initial receive sequence.
>
> After correcting the above, any SYN that doesn't exactly match the
> initial sequence number results in a RST|ACK response and the
> ESTABLISHED connection being dropped. Before this change, this is also
> what happened if a SYN arrived within or past the window, so I'm
> basically making the before-window behavior match the other
> behavior. I tested this using telnet to establish a TCP connection and
> raw packet injection to throw SYNs at it.
>
> Comments?
>

Yes, nice catch.

Best,
George