TF_NEEDSYN flag never set.

[Barry Spinney]

Yes, I switched over to using src_current.tar.gz, and that specific bug seems to be gone.  Thanx a lot.

BUT, the original reason I was looking so closely at this code, still remains a mystery to me.

In particular, the comment on lines 2716-2717 of tcp_input.c, along with the following statement,
seems to be wrong to me.

Specifically:

                /*
                  * If no data (only SYN) was ACK'd,
                  *     skip rest of ACK processing.
                  */
                 if (acked == 0)
                         goto step6;


This comment implies to me that  if the current rcvd pkt is a (say pure) ACK,
that happens to ACK my SYN-ACK (here it is assumed that the stack is operating
as a server, and we previously rcvd a SYN, sent a SYN-ACK, and are now rcving the
subsequent ACK), then acked should be 0 and we should "goto step6".
However my analysis of the code seems to imply that in this case "acked" will have
the value 1, which will proceed to execute in the following code, most of which seems
to expect some real data bytes to have been ack'd.   Now, I am not sure that
continuing past this goto necessarily causes harm, but it sure seems unexpected, and
in the future someone else might add code here assuming that the current ACK acks
some real data bytes.

Regarding my analysis, that acked is 1 in this case, one can examine syncache_socket
(called by syncache_expand when we promote a syncache entry to a full entry upon receipt
of the aforementioned ACK pkt), on line 821 of tcp_syncache.c.  This invokes the
tcp_sendseqinit macro (defined on line 62 of tcp_seq.h), which sets "snd_una" to be "iss".
Also line 817 of tcp_syncache.c sets the t_state to be TCPS_SYN_RECEIVED.

One can also see that snd_una MUST be iss, at least by noticing that the incoming ackNum
must of course be equal to "iss + 1" (since it acks the SYN which takes a sequence number),
and seeing that line 1916 of tcp_input.c would drop the pkt if ackNum were <= snd_una 
(assuming the t_state is still TCPS_SYN_RECEIVED - which more boring analysis seems to
confirm).

By the way line 2398 of tcp_input.c will change the t_state from TCPS_SYN_RECEIVED to
TCPS_ESTABLISHED.  Also notice line 2438 of tcp_input.c, which would cause the code to
go down the duplicate ACK pathway if the ackNum were <= snd_una (the equality be
the key here).

Finally we reach line 2661 of tcp_input.c which is:

        acked = BYTES_THIS_ACK(tp, th);

Where BYTES_THIS_ACK is just "ackNum - snd_una" (see line 261 of tcp_var.h).
But since snd_una==iss and ackNum=iss+1, "acked" must be 1.


Thanx Barry.

(p.s. the reason I am curious is I have been doing some work with a tcp stack that
was original derived from some FreeBSD software, though with non-trivial changes,
and I have been trying to understand its workings).
 


-----Original Message-----
From: Mark Johnston [mailto:markjdb at gmail.com] 
Sent: Sunday, April 28, 2013 7:05 PM
To: Barry Spinney
Cc: freebsd-net at freebsd.org
Subject: Re: TF_NEEDSYN flag never set.

On Sun, Apr 28, 2013 at 10:31:48PM +0000, Barry Spinney wrote:
> I am sorry if this is a dumb question, but I was trying to understand 
> the FreeBSD TCP stack, and In particular I was trying to understand 
> the use of the TF_NEEDSYN flag.  This flag is referenced a number of 
> times in tcp_input.c and tcp_output.c, but I don't think that it can ever be set.
> 
> In particular grepping through the "../src/sys/netinet", one discovers 
> that the only code that can set this flag is lines 1018 and 1020 of 
> tcp_input.c.  But, it appears to me that none of the lines in 
> tcp_input.c from 999 thru 1023 are even reachable!  The reason they are not reachable is because just ahead of them are the following lines:
> 
>     if (!syncache_add(&inc, &to, th, &so, m))
>         goto drop;
>     if (so == NULL) {
>         ...  // uninteresting lines, but no gotos
>         return;
>     }
>     ... /unreachable code here
> 
> 
>   Studying syncache_add (in file tcp_syncache.c), reveals three return statements.
>   One of the returns, returns the value 0, which will cause the "goto drop" to be executed.
>   The other two returns, return both the value 1 AND set "*sop = NULL", which should cause
>   the following "if (so == NULL)" to execute the subsequent return statement.
> 
> Is this intentional? (i.e. dead code awaiting future development?), or a bug?
> Or I am going blind to something obvious?
> 
> Thanx Barry Spinney.
> 
> (p.s. I doubt it matters which version of code, but to be precise this 
> is from the /pub/FreeBSD/development/tarballs named 
> "src_stable_6.tar.gz" dated "4/21/2013 01:15 AM", gotten from 
> ftp1.us.freebsd.org<ftp://ftp1.us.freebsd.org>)

That tarball presumably contains sources for the stable branch of FreeBSD 6, which probably isn't what you're looking for - the last release from that branch was in 2008.

The relevant code in FreeBSD-CURRENT is different and your observations don't seem to apply there. Based on a comment added in r156125, you seem to be correct though. :)
http://svnweb.freebsd.org/base?view=revision&revision=156125

I'd suggest fetching src_current.tar.gz from the FTP same directory and looking at that instead - you're more likely to get replies to questions about the current tip of development.