Bug 250363

Summary: [tcp] data in syn_ack should be ignored
Product: Base System Reporter: Yonghao Zou <yonghaoz1994>
Component: kernAssignee: Michael Tuexen <tuexen>
Status: Closed Works As Intended    
Severity: Affects Some People CC: net, rscheff, transport, tuexen
Priority: ---    
Version: Unspecified   
Hardware: Any   
OS: Any   

Description Yonghao Zou 2020-10-15 09:57:22 UTC 
data in syn_ack should be ignored as the following packetdrill script shows:

```
0.000 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
0.000 fcntl(3, F_GETFL) = 0x2 (flags O_RDWR)
0.000 fcntl(3, F_SETFL, O_RDWR|O_NONBLOCK) = 0

// Establish connection and verify that there was no error.
0.100 connect(3, ..., ...) = -1
0.100 > S 0:0(0) <...>

+0.1 < S. 1:513(512) ack 1 win 19712 
// f-stack/FreeBSD ack 513, Linux ack 1
+0.1 > . 1:1(0) ack 1

```
Comment 1 Richard Scheffenegger freebsd_committer freebsd_triage 2021-02-17 12:48:31 UTC 
With TFO, this codepath would be valid - but without TFO, data on SYN,ACK should be rejected.
Comment 2 Michael Tuexen freebsd_committer freebsd_triage 2021-02-17 13:20:56 UTC 
(In reply to Richard Scheffenegger from comment #1)
This report is not about TFO... Are you sure it is not allowed? My reading of RFC 793 is as follows. We are in SYN-SENT. So we are on page 66. The next text on page 68, which applies is:

If SND.UNA > ISS (our SYN has been ACKed), change the connection
state to ESTABLISHED, form an ACK segment

          <SEQ=SND.NXT><ACK=RCV.NXT><CTL=ACK>

and send it.  Data or controls which were queued for
transmission may be included.  If there are other controls or
text in the segment then continue processing at the sixth step
below where the URG bit is checked, otherwise return.

Then is step 7, we would process the text and send

 <SEQ=SND.NXT><ACK=RCV.NXT><CTL=ACK>

So my reading is that the response according to RFC 793 should be
* send an ACK for the SYN in the SYN-ACK, move to established
* send an ACK for the received data.

Is my understanding wrong?
Comment 3 Richard Scheffenegger freebsd_committer freebsd_triage 2021-02-17 14:27:18 UTC 
Reading 793bis, it makes it clear that your understanding is actually correct, data on syn,ack has been historically allowed.

However, it seems like the SYN bit should elicit an ACK for itself, immediately followed by an ACK for the data.

Unless I misundestand this aspect, this SYN-bit only ACK is missing currently, correct?
Comment 4 Michael Tuexen freebsd_committer freebsd_triage 2021-02-17 14:49:01 UTC 
Here is a script which shows what should happen according to RFC 793 and what happens on FreeBSD main. I just tested the script:

 0.000 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
+0.000 fcntl(3, F_GETFL) = 0x2 (flags O_RDWR)
+0.000 fcntl(3, F_SETFL, O_RDWR|O_NONBLOCK) = 0
+0.000 connect(3, ..., ...) = -1 EINPROGRESS (Operation now in progress)
+0.000 > S    0:0(0)             win 65535 <mss 1460,nop,wscale 6,sackOK,TS val 100 ecr 0>
+0.050 < S.   0:512(512) ack   1 win 65535 <mss 1460,sackOK,eol,eol>
#ifdef RFC793
+0.000 >  .   1:1(0)     ack   1 win 65535
+0.000 >  .   1:1(0)     ack 513 win 65188
#endif
#ifdef FreeBSD
+0.000 >  .   1:1(0)     ack 513 win 65188
#endif
+0.000 recv(3, ..., 1024, 0) = 512
+0.000 close(3) = 0
+0.000 > F.   1:1(0)     ack 513 win 65535
+0.000 < F. 513:513(0)   ack   2 win 65535
+0.000 >  .   2:2(0)     ack 514 win 65535

I actually prefer the FreeBSD way of handling this and would consider it a weakness of the specification to require the sending of two acks back-to-back where the second ack updates the first. The peer has to handle it, since the first ack can always get lost...
Comment 5 Michael Tuexen freebsd_committer freebsd_triage 2021-02-21 22:29:05 UTC 
I think we will keep this behaviour. Hopefully, RFC 793bis will make it explicit, that it is allowed.