Bug 250499

Summary:	[tcp] Should we reject the packet with timestamp if no timestamp in SYN and SYN_ACK?
Product:	Base System	Reporter:	Yonghao Zou <yonghaoz1994>
Component:	kern	Assignee:	Michael Tuexen <tuexen>
Status:	Closed FIXED
Severity:	Affects Some People	CC:	cy, net, novel, tuexen
Priority:	---	Keywords:	standards
Version:	12.1-RELEASE	Flags:	tuexen: mfc-stable12+ tuexen: mfc-stable11+
Hardware:	Any
OS:	Any

Description Yonghao Zou 2020-10-21 02:41:28 UTC

If there are no timestamp option in SYN and SYNACK packets, it seems FreeBSD now rejects the last ACK packet with timestamp as the following packetdrill script shows, but should we do that?

```
// Establish a connection.
0   socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
+0  setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
+0 fcntl(3, F_SETFL, O_RDWR|O_NONBLOCK) = 0
+0  bind(3, ..., ...) = 0
+0  listen(3, 1) = 0

+0 < S 0:0(0) win 12336
+0 ~ +1  > S. 0:0(0) ack 1 <mss 1460>

+.1 < . 1:1(0) ack 1 win 25710 <TS val 100 ecr 0,eol,eol>

+0.2 accept(3, ..., ...) = 4 // f-stack/FreeBSD return -1

```

Comment 1 commit-hook freebsd_committer

2020-11-09 21:49:56 UTC

A commit references this bug:

Author: tuexen
Date: Mon Nov  9 21:49:42 UTC 2020
New revision: 367530
URL: https://svnweb.freebsd.org/changeset/base/367530

Log:
  RFC 7323 specifies that:
  * TCP segments without timestamps should be dropped when support for
    the timestamp option has been negotiated.
  * TCP segments with timestamps should be processed normally if support
    for the timestamp option has not been negotiated.
  This patch enforces the above.

  PR:			250499
  Reviewed by:		gnn, rrs
  MFC after:		1 week
  Sponsored by:		Netflix, Inc
  Differential Revision:	https://reviews.freebsd.org/D27148

Changes:
  head/sys/netinet/tcp_input.c
  head/sys/netinet/tcp_stacks/bbr.c
  head/sys/netinet/tcp_stacks/rack.c
  head/sys/netinet/tcp_syncache.c
  head/sys/netinet/tcp_timewait.c

Comment 2 commit-hook freebsd_committer

2020-11-20 13:01:22 UTC

A commit references this bug:

Author: tuexen
Date: Fri Nov 20 13:00:29 UTC 2020
New revision: 367891
URL: https://svnweb.freebsd.org/changeset/base/367891

Log:
  Fix an issue I introuced in r367530: tcp_twcheck() can be called
  with to == NULL for SYN segments. So don't assume tp != NULL.
  Thanks to jhb@ for reporting and suggesting a fix.

  PR:			250499
  MFC after:		1 week
  XMFC-with:		r367530
  Sponsored by:		Netflix, Inc.

Changes:
  head/sys/netinet/tcp_timewait.c

Comment 3 commit-hook freebsd_committer

2020-11-30 09:46:10 UTC

A commit references this bug:

Author: tuexen
Date: Mon Nov 30 09:45:45 UTC 2020
New revision: 368181
URL: https://svnweb.freebsd.org/changeset/base/368181

Log:
  MFC r367530:
  RFC 7323 specifies that:
  * TCP segments without timestamps should be dropped when support for
    the timestamp option has been negotiated.
  * TCP segments with timestamps should be processed normally if support
    for the timestamp option has not been negotiated.
  This patch enforces the above.
  Manually resolved merge conflicts.

  MFC 367891:
  Fix an issue I introuced in r367530: tcp_twcheck() can be called
  with to == NULL for SYN segments. So don't assume tp != NULL.
  Thanks to jhb@ for reporting and suggesting a fix.

  MFC r367946:
  Fix two occurences of a typo in a comment introduced in r367530.
  Thanks to lstewart@ for reporting them.

  PR:			250499
  Reviewed by:		gnn, rrs
  Sponsored by:		Netflix, Inc.
  Differential Revision:	https://reviews.freebsd.org/D27148

Changes:
_U  stable/12/
  stable/12/sys/netinet/tcp_input.c
  stable/12/sys/netinet/tcp_stacks/rack.c
  stable/12/sys/netinet/tcp_syncache.c
  stable/12/sys/netinet/tcp_timewait.c

Comment 4 commit-hook freebsd_committer

2020-11-30 10:58:18 UTC

A commit references this bug:

Author: tuexen
Date: Mon Nov 30 10:58:07 UTC 2020
New revision: 368183
URL: https://svnweb.freebsd.org/changeset/base/368183

Log:
   MFC r367530:
   RFC 7323 specifies that:
   * TCP segments without timestamps should be dropped when support for
     the timestamp option has been negotiated.
   * TCP segments with timestamps should be processed normally if support
     for the timestamp option has not been negotiated.
   This patch enforces the above.
   Manually resolved merge conflicts.

   MFC 367891:
   Fix an issue I introuced in r367530: tcp_twcheck() can be called
   with to == NULL for SYN segments. So don't assume tp != NULL.
   Thanks to jhb@ for reporting and suggesting a fix.

   MFC r367946:
   Fix two occurences of a typo in a comment introduced in r367530.
   Thanks to lstewart@ for reporting them.

  PR:			250499
  Reviewed by:		gnn, rrs
  Sponsored by:		Netflix, Inc.
  Differential Revision:	https://reviews.freebsd.org/D27148

Changes:
_U  stable/11/
  stable/11/sys/netinet/tcp_input.c
  stable/11/sys/netinet/tcp_syncache.c
  stable/11/sys/netinet/tcp_timewait.c

Comment 5 Roman Bogorodskiy freebsd_committer

2021-01-24 12:35:15 UTC

Hi,

Hope this is a right place to report this.
This commit appears to be causing issues with the multimedia/plexmediaserver port. One of the tasks of this software is to do video decoding, and after this patch that breaks. Sometimes its HTTP-based API fails to respond, and also there are a many connections in LAST_ACK state:

$ netstat -4n|grep LAST_ACK|wc -l
     415
$ netstat -4n|grep LAST_ACK|grep "127.0.0.1.32400"|wc -l
     396

127.0.0.1.32400 corresponds to the Plex API endpoint.

Reverting this patch fixes the issue.

Original bug report is here: https://forums.plex.tv/t/transcoder-errors-on-freebsd/676180 (thanks to forum user Volts for pointing to this commit).

Comment 6 Michael Tuexen freebsd_committer

2021-01-24 15:43:44 UTC

(In reply to Roman Bogorodskiy from comment #5)
Hi Roman,

I introduced a regression, which was fixed in https://cgit.FreeBSD.org/src/commit/?id=cc3c34859eab1b317d0f38731355b53f7d978c97 and MFCed to stable/12 in https://cgit.FreeBSD.org/src/commit/?id=d05d908d6d3c85479c84c707f931148439ae826b.
This was affecting the handling of received TCP RST segments.
Does this fix your issue?

In bug #252449 it was reported that there are broken TCP stacks, which negotiate TCP timestamp support but then don't behave as required.
To deal with such broken implementations I added a sysctl variable in https://cgit.FreeBSD.org/src/commit/?id=d2b3ceddccac60b563f642898e3a314647666a10
, which was MFCed to stable/12 in https://cgit.FreeBSD.org/src/commit/?id=e82353f84c58da9a5c38bd471a09936c16a5b6ea. In this case you can set the sysctl variable net.inet.tcp.tolerate_missing_ts to 1. I would be interested to now which stack is broken and it would be great if you could provide a .pcap file of the failing TCP connection.
Does this fix your issue?

Comment 7 Roman Bogorodskiy freebsd_committer

2021-01-28 05:05:29 UTC

(In reply to Michael Tuexen from comment #6)

Hi Michael,

Latest HEAD appears to fix the issue for me (without any additional configurations like the sysctl you mention or anything else). I hope to test it more on a weekend.

> I would be interested to now which stack is broken and it would be great if you could provide a .pcap file of the failing TCP connection.

Do I need to try to catch the moment when the connection fails (that implies rolling back the fix) or any dump will suffice?

Comment 8 Michael Tuexen freebsd_committer

2021-01-28 09:53:59 UTC

(In reply to Roman Bogorodskiy from comment #7)
I everything works now without tweaking the net.inet.tcp.tolerate_missing_ts sysctl variable, I don't need anything more.

Only if you need to tweak the net.inet.tcp.tolerate_missing_ts sysctl, I would be interested in a .pcap file and knowing what the involved parties are.

But if you say, head is fine, I expect stable/12 and stable/13 also to be fine.