Do not drop packets w/ missing TCP timestamps.

RFC7323 recommends that if the timestamp option was negotiated
then all packets should carry a TCP Timestamp and any packets that
do not should be dropped.

Netstack implemented this behaviour. Linux OTOH does not and will
accept such packets. This change makes Netstack behaviour compatible
with Linux.

Also now that we allow such packets, we do need to update RTO calculations
based on these packets even if timestamp option is enabled.

PiperOrigin-RevId: 233432268
Change-Id: I9f4742ae6b63930ac3b5e37d8c238761e6a4b29f

3 files changed, 10 insertions, 39 deletions

diff --git a/pkg/tcpip/transport/tcp/connect.go b/pkg/tcpip/transport/tcp/connect.go
index cc34e74b2..7b80f7519 100644
--- a/pkg/tcpip/transport/tcp/connect.go
+++ b/pkg/tcpip/transport/tcp/connect.go
@@ -289,8 +289,9 @@ func (h *handshake) synRcvdState(s *segment) *tcpip.Error {
 		}
 
 		// Update timestamp if required. See RFC7323, section-4.3.
-		h.ep.updateRecentTimestamp(s.parsedOptions.TSVal, h.ackNum, s.sequenceNumber)
-
+		if h.ep.sendTSOk && s.parsedOptions.TS {
+			h.ep.updateRecentTimestamp(s.parsedOptions.TSVal, h.ackNum, s.sequenceNumber)
+		}
 		h.state = handshakeCompleted
 		return nil
 	}
@@ -740,16 +741,6 @@ func (e *endpoint) handleSegments() *tcpip.Error {
 			// send window scale.
 			s.window <<= e.snd.sndWndScale
 
-			// If the timestamp option is negotiated and the segment
-			// does not carry a timestamp option then the segment
-			// must be dropped as per
-			// https://tools.ietf.org/html/rfc7323#section-3.2.
-			if e.sendTSOk && !s.parsedOptions.TS {
-				e.stack.Stats().DroppedPackets.Increment()
-				s.decRef()
-				continue
-			}
-
 			// RFC 793, page 41 states that "once in the ESTABLISHED
 			// state all segments must carry current acknowledgment
 			// information."
diff --git a/pkg/tcpip/transport/tcp/snd.go b/pkg/tcpip/transport/tcp/snd.go
index 122c52f32..5c9e76fa1 100644
--- a/pkg/tcpip/transport/tcp/snd.go
+++ b/pkg/tcpip/transport/tcp/snd.go
@@ -635,14 +635,15 @@ func (s *sender) checkDuplicateAck(seg *segment) (rtx bool) {
 // updating the send-related state.
 func (s *sender) handleRcvdSegment(seg *segment) {
 	// Check if we can extract an RTT measurement from this ack.
-	if !s.ep.sendTSOk && s.rttMeasureSeqNum.LessThan(seg.ackNumber) {
+	if !seg.parsedOptions.TS && s.rttMeasureSeqNum.LessThan(seg.ackNumber) {
 		s.updateRTO(time.Now().Sub(s.rttMeasureTime))
 		s.rttMeasureSeqNum = s.sndNxt
 	}
 
 	// Update Timestamp if required. See RFC7323, section-4.3.
-	s.ep.updateRecentTimestamp(seg.parsedOptions.TSVal, s.maxSentAck, seg.sequenceNumber)
-
+	if s.ep.sendTSOk && seg.parsedOptions.TS {
+		s.ep.updateRecentTimestamp(seg.parsedOptions.TSVal, s.maxSentAck, seg.sequenceNumber)
+	}
 	// Count the duplicates and do the fast retransmit if needed.
 	rtx := s.checkDuplicateAck(seg)
 
diff --git a/pkg/tcpip/transport/tcp/tcp_timestamp_test.go b/pkg/tcpip/transport/tcp/tcp_timestamp_test.go
index b08df0fec..87c640967 100644
--- a/pkg/tcpip/transport/tcp/tcp_timestamp_test.go
+++ b/pkg/tcpip/transport/tcp/tcp_timestamp_test.go
@@ -255,7 +255,7 @@ func TestSendGreaterThanMTUWithOptions(t *testing.T) {
 	testBrokenUpWrite(t, c, maxPayload)
 }
 
-func TestSegmentDropWhenTimestampMissing(t *testing.T) {
+func TestSegmentNotDroppedWhenTimestampMissing(t *testing.T) {
 	const maxPayload = 100
 	c := context.New(t, uint32(header.TCPMinimumSize+header.IPv4MinimumSize+maxPayload))
 	defer c.Cleanup()
@@ -270,37 +270,16 @@ func TestSegmentDropWhenTimestampMissing(t *testing.T) {
 	droppedPacketsStat := c.Stack().Stats().DroppedPackets
 	droppedPackets := droppedPacketsStat.Value()
 	data := []byte{1, 2, 3}
-	// Save the sequence number as we will reset it later down
-	// in the test.
-	savedSeqNum := rep.NextSeqNum
+	// Send a packet with no TCP options/timestamp.
 	rep.SendPacket(data, nil)
 
 	select {
 	case <-ch:
-		t.Fatalf("Got data to read when we expect packet to be dropped")
-	case <-time.After(1 * time.Second):
-		// We expect that no data will be available to read.
-	}
-
-	// Assert that DroppedPackets was incremented by 1.
-	if got, want := droppedPacketsStat.Value(), droppedPackets+1; got != want {
-		t.Fatalf("incorrect number of dropped packets, got: %v, want: %v", got, want)
-	}
-
-	droppedPackets = droppedPacketsStat.Value()
-	// Reset the sequence number so that the other endpoint accepts
-	// this segment and does not treat it like an out of order delivery.
-	rep.NextSeqNum = savedSeqNum
-	// Now send a packet with timestamp and we should get the data.
-	rep.SendPacketWithTS(data, rep.TSVal+1)
-
-	select {
-	case <-ch:
 	case <-time.After(1 * time.Second):
 		t.Fatalf("Timed out waiting for data to arrive")
 	}
 
-	// Assert that DroppedPackets was not incremented by 1.
+	// Assert that DroppedPackets was not incremented.
 	if got, want := droppedPacketsStat.Value(), droppedPackets; got != want {
 		t.Fatalf("incorrect number of dropped packets, got: %v, want: %v", got, want)
 	}