Re: [PATCH net v2 1/2] tcp: process the 3rd ACK with sk_socket for TFO/MPTCP

[Kuniyuki Iwashima]

From: Eric Dumazet <edumazet@xxxxxxxxxx>
Date: Tue, 23 Jul 2024 17:38:27 +0200
[...]
> > > //
> > > // Test the simultaneous open scenario that both end sends
> > > // SYN/data. Although we don't support that the connection should
> > > // still be established.
> > > //
> > > `../../common/defaults.sh
> > >  ../../common/set_sysctls.py /proc/sys/net/ipv4/tcp_timestamps=0`
> > >
> > > // Cache warmup: send a Fast Open cookie request
> > >     0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
> > >    +0 fcntl(3, F_SETFL, O_RDWR|O_NONBLOCK) = 0
> > >    +0 sendto(3, ..., 0, MSG_FASTOPEN, ..., ...) = -1 EINPROGRESS
> > > (Operation is now in progress)
> > >    +0 > S 0:0(0) <mss 1460,nop,nop,sackOK,nop,wscale 8,FO,nop,nop>
> > >  +.01 < S. 123:123(0) ack 1 win 14600 <mss
> > > 1460,nop,nop,sackOK,nop,wscale 6,FO abcd1234,nop,nop>
> > >    +0 > . 1:1(0) ack 1
> > >  +.01 close(3) = 0
> > >    +0 > F. 1:1(0) ack 1
> > >  +.01 < F. 1:1(0) ack 2 win 92
> > >    +0 > .  2:2(0) ack 2
> > >
> > >
> > > //
> > > // Test: simulatenous fast open
> > > //
> > >  +.01 socket(..., SOCK_STREAM, IPPROTO_TCP) = 4
> > >    +0 fcntl(4, F_SETFL, O_RDWR|O_NONBLOCK) = 0
> > >    +0 sendto(4, ..., 1000, MSG_FASTOPEN, ..., ...) = 1000
> > >    +0 > S 0:1000(1000) <mss 1460,nop,nop,sackOK,nop,wscale 8,FO
> > > abcd1234,nop,nop>
> > > // Simul. SYN-data crossing: we don't support that yet so ack only remote ISN
> > > +.005 < S 1234:1734(500) win 14600 <mss 1040,nop,nop,sackOK,nop,wscale
> > > 6,FO 87654321,nop,nop>
> > >    +0 > S. 0:0(0) ack 1235 <mss 1460,nop,nop,sackOK,nop,wscale 8>
> > >
> > > // SYN data is never retried.
> > > +.045 < S. 1234:1234(0) ack 1001 win 14600 <mss
> > > 940,nop,nop,sackOK,nop,wscale 6,FO 12345678,nop,nop>
> > >    +0 > . 1001:1001(0) ack 1
> >
> > I recently sent a PR -- already applied -- to Neal to remove this line:
> >
> >   https://github.com/google/packetdrill/pull/86
> >
> > I thought it was the intension of Kuniyuki's patch not to send this ACK
> > in this case to follow the RFC 9293's recommendation. This TFO test
> > looks a bit similar to the example from Kuniyuki's patch:
> >
> >
> > --------------- 8< ---------------
> >  0 socket(..., SOCK_STREAM|SOCK_NONBLOCK, IPPROTO_TCP) = 3
> > +0 connect(3, ..., ...) = -1 EINPROGRESS (Operation now in progress)
> >
> > +0 > S  0:0(0) <mss 1460,sackOK,TS val 1000 ecr 0,nop,wscale 8>
> > +0 < S  0:0(0) win 1000 <mss 1000>
> > +0 > S. 0:0(0) ack 1 <mss 1460,sackOK,TS val 3308134035 ecr 0,nop,wscale 8>
> > +0 < S. 0:0(0) ack 1 win 1000
> >
> >   /* No ACK here */
> >
> > +0 write(3, ..., 100) = 100
> > +0 > P. 1:101(100) ack 1
> > --------------- 8< ---------------
> >
> >
> >
> > But maybe here that should be different for TFO?
> >
> > For my case with MPTCP (and TFO), it is fine to drop this 'goto consume'
> > but I don't know how "strict" we want to be regarding the RFC and this
> > marginal case.
> 
> Problem of this 'goto consume' is that we are not properly sending a
> DUPACK in this case.
> 
>  +.01 socket(..., SOCK_STREAM, IPPROTO_TCP) = 4
>    +0 fcntl(4, F_SETFL, O_RDWR|O_NONBLOCK) = 0
>    +0 sendto(4, ..., 1000, MSG_FASTOPEN, ..., ...) = 1000
>    +0 > S 0:1000(1000) <mss 1460,nop,nop,sackOK,nop,wscale 8,FO
> abcd1234,nop,nop>
> // Simul. SYN-data crossing: we don't support that yet so ack only remote ISN
> +.005 < S 1234:1734(500) win 14600 <mss 1040,nop,nop,sackOK,nop,wscale
> 6,FO 87654321,nop,nop>
>    +0 > S. 0:0(0) ack 1235 <mss 1460,nop,nop,sackOK,nop,wscale 8>
> 
> +.045 < S. 1234:1234(0) ack 1001 win 14600 <mss
> 940,nop,nop,sackOK,nop,wscale 6,FO 12345678,nop,nop>
>    +0 > . 1001:1001(0) ack 1 <nop,nop,sack 0:1>  // See here
> 
> Not sending a dupack seems wrong and could hurt.

I think the situation where we should send ACK after simultaneous
SYN+ACK would be:

---8<---
 +.01 socket(..., SOCK_STREAM, IPPROTO_TCP) = 4
   +0 fcntl(4, F_SETFL, O_RDWR|O_NONBLOCK) = 0
   +0 sendto(4, ..., 1000, MSG_FASTOPEN, ..., ...) = 1000
   +0 > S 0:1000(1000) <mss 1440,nop,nop,sackOK,nop,wscale 8,FO abcd1234,nop,nop>
// Simul. SYN-data crossing: we don't support that yet so ack only remote ISN
+.005 < S 1234:1734(500) win 14400 <mss 1040,nop,nop,sackOK,nop,wscale 6,FO 87654321,nop,nop>
   +0 > S. 0:0(0) ack 1235 <mss 1440,nop,nop,sackOK,nop,wscale 8>

// SYN data is not ACKed too.
+.045 < S. 1234:1234(0) ack 1 win 14400 <mss 940,nop,nop,sackOK,nop,wscale 6,FO 12345678,nop,nop>

   +0 > . 1:1001(1000) ack 1
---8<---

When the first data in SYN is not ACKed, it must be retransmitted,
and it can be done just after SYN+ACK is received, which is skipped
by 'goto consume'.

Retransmitting data in SYN is not supported though.

---8<---
sendto syscall: 1721769223.194675
outbound sniffed packet:  0.040288 S 833802090:833803090(1000) win 65535 <mss 1440,nop,nop,sackOK,nop,wscale 8,FO abcd1234,nop,nop>
inbound injected packet:  0.045323 S 1234:1734(500) win 14400 <mss 1040,nop,nop,sackOK,nop,wscale 6,FO 87654321,nop,nop>
outbound sniffed packet:  0.045355 S. 833802090:833802090(0) ack 1235 win 65535 <mss 1440,nop,nop,sackOK,nop,wscale 8>
inbound injected packet:  0.090429 S. 1234:1234(0) ack 833802091 win 14400 <mss 940,nop,nop,sackOK,nop,wscale 6,FO 12345678,nop,nop>
outbound sniffed packet:  0.090460 . 833803091:833803091(0) ack 1235 win 1052 
outbound sniffed packet:  1.051776 S. 833802090:833802090(0) ack 1235 win 65535 <mss 1440,nop,nop,sackOK,nop,wscale 8>
simul2.pkt:38: error handling packet: live packet field tcp_data_offset: expected: 5 (0x5) vs actual: 8 (0x8)
   script packet:  0.090462 . 1:1001(1000) ack 1 
actual #0 packet:  0.090460 . 1001:1001(0) ack 1 win 1052 
actual #1 packet:  1.051776 S. 0:0(0) ack 1235 win 65535 <mss 1440,nop,nop,sackOK,nop,wscale 8>
---8<---

Except the case, I think the dupack is not needed in theory.

But I understand the dupack could help the other quickly retransmit the
not-yet-ACKed data in SYN instead of waiting for a timer as expected in
the comment.

---8<---
// The other end retries
  +.1 < P. 1:501(500) ack 1000 win 257
---8<---