Details

Reviewers

rrs
tuexen
jtl
thj

Group Reviewers

transport

Commits

rGc0c928636156: TCP_NOOPT may prevent the negotiation of TCP features, which a server
rG2593f858d7d0: A TCP server has to take into consideration, if TCP_NOOPT is preventing

Summary

The strict adherance to RFC7323 (D27148) requires the server to take
into consideration, if it will be sending out a TSopt in the SYN,ACK.

Retaining the TF_RCVD_TSTMP would otherwise later indicate the
successful negotiation of the TSopt - even when the session was set up
without TS support.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 36984
Build 33873: arc lint + arc unit

Event Timeline

rscheff created this revision.Feb 13 2021, 4:55 PM

Herald added a reviewer: transport. · View Herald TranscriptFeb 13 2021, 4:55 PM

Herald added subscribers: melifaro, imp. · View Herald Transcript

rscheff requested review of this revision.Feb 13 2021, 4:55 PM

Harbormaster completed remote builds in B36973: Diff 83837.Feb 13 2021, 4:55 PM

tuexen added inline comments.Feb 14 2021, 12:21 AM

sys/netinet/tcp_input.c

1652–1653

We need to replace

if ((to.to_flags & TOF_SCALE) &&
    (tp->t_flags & TF_REQ_SCALE)) {

if ((to.to_flags & TOF_SCALE) &&
    (tp->t_flags & TF_REQ_SCALE) &&
    !(tp->t_flags & TF_NOOPT)) {

sys/netinet/tcp_syncache.c

1665

I guess the same argument applies to window scaling.
So I would suggest to replace

if (V_tcp_do_rfc1323) {

if (V_tcp_do_rfc1323 && !(ltflags & TF_NOOPT)) {

and remove the condition here.

tuexen requested changes to this revision.Feb 14 2021, 12:21 AM

This revision now requires changes to proceed.Feb 14 2021, 12:21 AM

The above modifications are necessary in combination with D28656 to allow the following packetdrill script to run successfully:

 0.00 `sysctl -w net.inet.tcp.hostcache.purgenow=1`
+0.00 `sysctl -w net.inet.tcp.syncookies_only=0`
+0.00 `sysctl -w net.inet.tcp.syncookies=1`
+0.00 `sysctl -w net.inet.tcp.rfc1323=1`
+0.00 `sysctl -w net.inet.tcp.sack.enable=1`
+0.00 `sysctl -w net.inet.tcp.ecn.enable=2`

+0.00 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
+0.00 fcntl(3, F_GETFL) = 0x02 (flags O_RDWR)
+0.00 fcntl(3, F_SETFL, O_RDWR | O_NONBLOCK) = 0
+0.00 setsockopt(3, IPPROTO_TCP, TCP_NOOPT, [1], 4) = 0
+0.00 connect(3, ..., ...) = -1 EINPROGRESS (Operation now in progress)
+0.00 > S    0:0(0)       win 65535
+0.05 < S    0:0(0)       win 65535 <mss 1460,nop,wscale 6,sackOK,TS val 100 ecr 0>
+0.00 > S.   0:0(0) ack 1 win 65535
+0.00 <  .   1:1(0) ack 1 win 65535
+0.00 < P.   1:2(1) ack 1 win 65535
+0.04 >  .   1:1(0) ack 2 win 65535

consider TF_NOOPT for all SYN options (simultaneous SYN)

Harbormaster completed remote builds in B36984: Diff 83862.Feb 14 2021, 9:04 AM

Generalizing, the other options need to be disabled too, in case of a simultaneous SYN. Disabling the request before that, during the tcp_newtcpcb() for an abundance of safety.

typo

Harbormaster completed remote builds in B36985: Diff 83863.Feb 14 2021, 9:10 AM

tuexen added inline comments.Feb 14 2021, 11:37 AM

sys/netinet/tcp_subr.c
1766–1771	I think this can't be true, since you just created the tcb and it is initialized with 0. So I would suggest to remove this change.

Programmatically, correct. Which makes me wonder, shouldn't the TF_NOOPT (and other flags) be inherited from a listening socket, when you accept() an incoming connection?

E.g. if you want to check if specific options are set on an accepted socket (rather then the listening socket), it currently doesn't seem to reflect the settings actually in effect, but the system global defaults...

remove check in tcp_newtcpcb() again

Harbormaster completed remote builds in B36991: Diff 83873.Feb 14 2021, 12:23 PM

tuexen added inline comments.Feb 14 2021, 12:42 PM

sys/netinet/tcp_input.c
1677–1678	I don't think we should never be here with TCP_NOOPT. Also disabling the path is not the correct action here.

moving the parallel SYN check further down, to gracefully disable FASTOPEN

Harbormaster completed remote builds in B36992: Diff 83874.Feb 14 2021, 1:00 PM

rscheff added inline comments.Feb 14 2021, 1:00 PM

sys/netinet/tcp_input.c
1677–1678	This also deals with the parallel open state: While we may not have sent TFO in the outgoing SYN, the incoming SYN could have a TFO option. I suggest to gracefully disable TFO in this case, instead of having one half-connecting in TFO state, and the other without TFO, as NOOPT prevents the transmission of TFO later in tcp_output().

tuexen added inline comments.Feb 14 2021, 1:43 PM

sys/netinet/tcp_input.c
1677–1678	If you want to do a check here, do it above. The way you do it now results in disabling TFO with a reason for doing so.
1677–1678	This also deals with the parallel open state: While we may not have sent TFO in the outgoing SYN, the incoming SYN could have a TFO option. I suggest to gracefully disable TFO in this case, instead of having one half-connecting in TFO state, and the other without TFO, as NOOPT prevents the transmission of TFO later in tcp_output().