Page MenuHomeFreeBSD

Improve RFC 7323 Compliance
ClosedPublic

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

Details

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

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 34705

Event Timeline

tuexen edited the summary of this revision. (Show Details)
gnn added a subscriber: gnn.

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

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

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.

sys/netinet/tcp_timewait.c
420

Ah, good point. Sorry for the noise.