From efe5e737d7da48e6f99a1d9a53fe04b6d21b7511 Mon Sep 17 00:00:00 2001 From: Bhasker Hariharan Date: Mon, 11 Feb 2019 10:22:42 -0800 Subject: [PATCH] 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 --- pkg/tcpip/transport/tcp/connect.go | 15 +++-------- pkg/tcpip/transport/tcp/snd.go | 7 ++--- pkg/tcpip/transport/tcp/tcp_timestamp_test.go | 27 +++---------------- 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) }