netstack: don't RST newly established connections under load

Fixes a bug where new connections could RST if the stack switched from using to not using SYN cookies between retransmitted SYNs. Verified that the newly-added test here fails without these changes. PiperOrigin-RevId: 586445880
2026-05-22 17:12:49 -07:00 · 2023-11-29 14:00:50 -08:00
parent d5fc150056
commit 368fc605d6
2 changed files with 154 additions and 13 deletions
@@ -290,19 +290,9 @@ func (h *handshake) resetToSynRcvd(iss seqnum.Value, irs seqnum.Value, opts head
 }

 // checkAck checks if the ACK number, if present, of a segment received during
-// a TCP 3-way handshake is valid. If it's not, a RST segment is sent back in
-// response.
+// a TCP 3-way handshake is valid.
 func (h *handshake) checkAck(s *segment) bool {
-	if s.flags.Contains(header.TCPFlagAck) && s.ackNumber != h.iss+1 {
-		// RFC 793, page 72 (https://datatracker.ietf.org/doc/html/rfc793#page-72):
-		//   If the segment acknowledgment is not acceptable, form a reset segment,
-		//        <SEQ=SEG.ACK><CTL=RST>
-		//   and send it.
-		h.ep.sendEmptyRaw(header.TCPFlagRst, s.ackNumber, 0, 0)
-		return false
-	}
-
-	return true
+	return !(s.flags.Contains(header.TCPFlagAck) && s.ackNumber != h.iss+1)
 }

 // synSentState handles a segment received when the TCP 3-way handshake is in
@@ -324,6 +314,11 @@ func (h *handshake) synSentState(s *segment) tcpip.Error {
 	}

 	if !h.checkAck(s) {
+		// RFC 793, page 72 (https://datatracker.ietf.org/doc/html/rfc793#page-72):
+		//   If the segment acknowledgment is not acceptable, form a reset segment,
+		//        <SEQ=SEG.ACK><CTL=RST>
+		//   and send it.
+		h.ep.sendEmptyRaw(header.TCPFlagRst, s.ackNumber, 0, 0)
 		return nil
 	}

@@ -405,8 +400,30 @@ func (h *handshake) synRcvdState(s *segment) tcpip.Error {
 		return nil
 	}

+	// It's possible that s is an ACK of a SYN cookie. This can happen if:
+	//
+	//   - We receive a SYN while under load and issue a SYN/ACK with
+	//     cookie S.
+	//   - We receive a retransmitted SYN while space exists in the SYN
+	//     queue, and issue a SYN/ACK with seqnum S'.
+	//   - We receive the ACK based on S.
+	//
+	// If we receive a SYN cookie ACK, just use the cookie seqnum.
 	if !h.checkAck(s) {
-		return nil
+		iss := s.ackNumber - 1
+		data, ok := h.listenEP.listenCtx.isCookieValid(s.id, iss, s.sequenceNumber-1)
+		if !ok || int(data) >= len(mssTable) {
+			// This isn't a valid cookie.
+			// RFC 793, page 72 (https://datatracker.ietf.org/doc/html/rfc793#page-72):
+			//   If the segment acknowledgment is not acceptable, form a reset segment,
+			//        <SEQ=SEG.ACK><CTL=RST>
+			//   and send it.
+			h.ep.sendEmptyRaw(header.TCPFlagRst, s.ackNumber, 0, 0)
+			return nil
+		}
+		// This is a cookie that snuck its way in after we stopped using them.
+		h.mss = mssTable[data]
+		h.iss = iss
 	}

 	// RFC 793, Section 3.9, page 69, states that in the SYN-RCVD state, a
@@ -9295,6 +9295,130 @@ func TestReleaseDanglingEndpoints(t *testing.T) {
 	))
 }

+// TestLateSynCookieAck ensures that we properly handle the following case
+// rather than sending a RST on a valid connection:
+//
+//   - We receive a SYN while under load and issue a SYN/ACK with cookie S.
+//   - We receive a retransmitted SYN while space exists in the SYN queue, and
+//     issue a SYN/ACK with seqnum S'.
+//   - We receive an ACK based on S.
+//   - We respond with an RST because we expected an ACK based on S'.
+func TestLateSynCookieAck(t *testing.T) {
+	c := context.New(t, e2e.DefaultMTU)
+	defer c.Cleanup()
+	stats := c.Stack().Stats()
+	wq := &waiter.Queue{}
+	ep, err := c.Stack().NewEndpoint(tcp.ProtocolNumber, ipv4.ProtocolNumber, wq)
+	if err != nil {
+		t.Fatalf("NewEndpoint failed: %s", err)
+	}
+
+	initial := stats.TCP.CurrentEstablished.Value()
+
+	if err := ep.Bind(tcpip.FullAddress{Port: context.StackPort}); err != nil {
+		t.Fatalf("Bind failed: %s", err)
+	}
+
+	// With a backlog of 2, we get one slot in the SYN queue before we
+	// start using SYN cookies. See
+	// //pkg/tcpip/transport/tcp/accept.go:handleListenSegment:useSynCookies
+	// for an explanation.
+	if err := ep.Listen(2); err != nil {
+		t.Fatalf("Listen failed: %s", err)
+	}
+
+	// To reach our desired state, we're gonna do the following:
+	//
+	//   - Send SYN S1 to force subsequent SYNs to return cookies.
+	//   - Send SYN S2, which returns a cookie SYN/ACK.
+	//   - Finish S1's handshake, opening space in the SYN queue.
+	//   - Retransmit S2, which will give use a different seqnum.
+	//   - Finish S2's handshake with the cookie SYN/ACK.
+
+	// Send S1.
+	const otherTestPort = context.TestPort + 1
+	iss := seqnum.Value(context.TestInitialSequenceNumber)
+	c.SendPacket(nil, &context.Headers{
+		SrcPort: otherTestPort,
+		DstPort: context.StackPort,
+		Flags:   header.TCPFlagSyn,
+		SeqNum:  iss,
+	})
+	// Receive the SYN-ACK reply.
+	s1Reply := c.GetPacket()
+	defer s1Reply.Release()
+	s1ReplyHdr := header.TCP(header.IPv4(s1Reply.AsSlice()).Payload())
+
+	// Send S2.
+	c.SendPacket(nil, &context.Headers{
+		SrcPort: context.TestPort,
+		DstPort: context.StackPort,
+		Flags:   header.TCPFlagSyn,
+		SeqNum:  iss,
+	})
+	// Receive the SYN-ACK reply.
+	s2CookieReply := c.GetPacket()
+	defer s2CookieReply.Release()
+	s2CookieReplyHdr := header.TCP(header.IPv4(s2CookieReply.AsSlice()).Payload())
+
+	// Finish the S1 handshake.
+	ackHeaders := &context.Headers{
+		SrcPort: otherTestPort,
+		DstPort: context.StackPort,
+		Flags:   header.TCPFlagAck,
+		SeqNum:  iss + 1,
+		AckNum:  seqnum.Value(s1ReplyHdr.SequenceNumber()) + 1,
+	}
+	c.SendPacket(nil, ackHeaders)
+
+	// Wait for S1's connection to move from the SYN to the accept queue.
+	metricPollFn := func() error {
+		if got, want := stats.TCP.CurrentEstablished.Value(), initial+1; got != want {
+			return fmt.Errorf("connection never established: got stats.TCP.CurrentEstablished.Value() = %d, want = %d", got, want)
+		}
+		return nil
+	}
+	if err := testutil.Poll(metricPollFn, 1*time.Second); err != nil {
+		t.Fatal(err)
+	}
+
+	// Retransmit S2.
+	c.SendPacket(nil, &context.Headers{
+		SrcPort: context.TestPort,
+		DstPort: context.StackPort,
+		Flags:   header.TCPFlagSyn,
+		SeqNum:  iss,
+	})
+	// Receive the SYN-ACK reply.
+	s2QueueReply := c.GetPacket()
+	defer s2QueueReply.Release()
+	s2QueueReplyHdr := header.TCP(header.IPv4(s2QueueReply.AsSlice()).Payload())
+	if s2CookieReplyHdr.SequenceNumber() == s2QueueReplyHdr.SequenceNumber() {
+		t.Fatalf("the SYN cookie and regular seqnum are equal; is the backlog too large?")
+	}
+
+	// Finish S2's handshake using the cookie.
+	ackHeaders = &context.Headers{
+		SrcPort: context.TestPort,
+		DstPort: context.StackPort,
+		Flags:   header.TCPFlagAck,
+		SeqNum:  iss + 1,
+		AckNum:  seqnum.Value(s2CookieReplyHdr.SequenceNumber()) + 1,
+	}
+	c.SendPacket(nil, ackHeaders)
+
+	// Verify that we've completed two connections.
+	metricPollFn = func() error {
+		if got, want := stats.TCP.CurrentEstablished.Value(), initial+2; got != want {
+			return fmt.Errorf("got stats.TCP.CurrentEstablished.Value() = %d, want = %d", got, want)
+		}
+		return nil
+	}
+	if err := testutil.Poll(metricPollFn, 1*time.Second); err != nil {
+		t.Error(err)
+	}
+}
+
 func TestMain(m *testing.M) {
 	refs.SetLeakMode(refs.LeaksPanic)
 	code := m.Run()