Improve RFC 7323 Compliance
ClosedPublic
Actions

Authored by tuexen on Nov 9 2020, 11:50 AM.

Details

Reviewers

Group Reviewers

transport

Commits

Summary

RFC 7323 describes:

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 ensures the above.

This issue was brought up in the context of the ACK segment in the 3-way handshake in PR250499.

Test Plan

Use

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 34705

tuexen requested review of this revision.Nov 9 2020, 11:50 AM

tuexen edited the summary of this revision. (Show Details)

Thanks for doing this and bringing the code up to date.

rrs accepted this revision.Nov 9 2020, 7:17 PM

This revision is now accepted and ready to land.Nov 9 2020, 7:17 PM

Committed in r367530.

Sorry for the tardy comments, but a few things in this changeset caught my eye during our internal Netflix upstream merge review.

sys/netinet/tcp_input.c
1698	`without` -> `with`?
sys/netinet/tcp_syncache.c
1216	`without` -> `with`? (and as an aside, would it also not make more sense to invert these 2 blocks similar to how Andre had them such that the "SCF_TIMESTAMP && !TOF_TS" case which returns is handled before the "!SCF_TIMESTAMP && TOF_TS" fallthrough case?)
sys/netinet/tcp_timewait.c
420	Nit: style here is inconsistent with the other parts of this diff which use `!(to->to_flags & TOF_TS)`

sys/netinet/tcp_input.c
1698	Fixed in r 367946.
sys/netinet/tcp_syncache.c
1216	Typo fixed in r 367946. I don't understand the second part of the comment. The old code was: "!SCF_TIMESTAMP && TOF_TS" before "SCF_TIMESTAMP && !TOF_TS" The new code is: "!SCF_TIMESTAMP && TOF_TS" before "SCF_TIMESTAMP && !TOF_TS" So it is the same. At least when looking at the `if` statements. The resulting action was wrong and has been fixed.
sys/netinet/tcp_timewait.c
420	Nit: style here is inconsistent with the other parts of this diff which use `!(to->to_flags & TOF_TS)` I was trying to be consistent within the file, where you can find `(to.to_flags & TOF_TS) != 0`...

sys/netinet/tcp_syncache.c
1216	I don't understand the second part of the comment. The old code was: "!SCF_TIMESTAMP && TOF_TS" before "SCF_TIMESTAMP && !TOF_TS" The new code is: "!SCF_TIMESTAMP && TOF_TS" before "SCF_TIMESTAMP && !TOF_TS" So it is the same. At least when looking at the `if` statements. The resulting action was wrong and has been fixed. Oops, sorry, I got confused by the change of action. The point I was trying to make is that evaluating the condition which returns (i.e. the `SCF_TIMESTAMP && !TOF_TS` case) before the condition which falls through seems preferable to me, but not a big deal either way.