diff --git a/pkg/tcpip/transport/tcp/rcv.go b/pkg/tcpip/transport/tcp/rcv.go index 9981e4d88..292e0d0e6 100644 --- a/pkg/tcpip/transport/tcp/rcv.go +++ b/pkg/tcpip/transport/tcp/rcv.go @@ -481,13 +481,26 @@ func (r *receiver) handleRcvdSegment(s *segment) (drop bool, err tcpip.Error) { // Defer segment processing if it can't be consumed now. if !r.consumeSegment(s, segSeq, segLen) { if segLen > 0 || s.flags.Contains(header.TCPFlagFin) { - // We only store the segment if it's within our buffer size limit. + // We only store the segment if it's within our buffer + // size limit. // - // Only use 75% of the receive buffer queue for out-of-order - // segments. This ensures that we always leave some space for the inorder - // segments to arrive allowing pending segments to be processed and + // Only use 75% of the receive buffer queue for + // out-of-order segments. This ensures that we always + // leave some space for the inorder segments to arrive + // allowing pending segments to be processed and // delivered to the user. - if rcvBufSize := r.ep.ops.GetReceiveBufferSize(); rcvBufSize > 0 && (r.PendingBufUsed+int(segLen)) < int(rcvBufSize)>>2 { + // + // The ratio must be at least 50% (the size of rwnd) to + // leave space for retransmitted dropped packets. 51% + // would make recovery slow when there are multiple + // drops by necessitating multiple round trips. 100% + // would enable the buffer to be totally full of + // out-of-order data and stall the connection. + // + // An ideal solution is to ensure that there are at + // least N bytes free when N bytes are missing, but we + // don't have that computed at this point in the stack. + if rcvBufSize := r.ep.ops.GetReceiveBufferSize(); rcvBufSize > 0 && (r.PendingBufUsed+int(segLen)) < int(rcvBufSize-rcvBufSize/4) { r.ep.rcvQueueMu.Lock() r.PendingBufUsed += s.segMemSize() r.ep.rcvQueueMu.Unlock() diff --git a/pkg/tcpip/transport/tcp/test/e2e/tcp_test.go b/pkg/tcpip/transport/tcp/test/e2e/tcp_test.go index d70dac7b4..e78c6a6ab 100644 --- a/pkg/tcpip/transport/tcp/test/e2e/tcp_test.go +++ b/pkg/tcpip/transport/tcp/test/e2e/tcp_test.go @@ -2099,7 +2099,8 @@ func TestOutOfOrderFlood(t *testing.T) { ept := endpointTester{c.EP} ept.CheckReadError(t, &tcpip.ErrWouldBlock{}) - // Send 100 packets before the actual one that is expected. + // Send 100 packets with seqnum iss + 6 before the actual one that is + // expected. data := []byte{1, 2, 3, 4, 5, 6} iss := seqnum.Value(context.TestInitialSequenceNumber).Add(1) for i := 0; i < 100; i++ { @@ -2123,8 +2124,11 @@ func TestOutOfOrderFlood(t *testing.T) { ) } - // Send packet with seqnum as initial + 3. It must be discarded because the - // out-of-order buffer was filled by the previous packets. + // Send packet with seqnum as initial + 3. It won't be discarded + // because the receive window limits the sender to rcvBufSize/2 bytes, + // but we allow (3/4)*rcvBufSize to be used for out-of-order bytes. So + // the sender hasn't filled the buffer and we still have space to + // receive it. c.SendPacket(data[3:], &context.Headers{ SrcPort: context.TestPort, DstPort: c.Port, @@ -2154,13 +2158,13 @@ func TestOutOfOrderFlood(t *testing.T) { RcvWnd: 30000, }) - // Check that only packet with initial sequence number is acknowledged. + // Check that all packets are acknowledged. v = c.GetPacket() defer v.Release() checker.IPv4(t, v, checker.TCP( checker.DstPort(context.TestPort), checker.TCPSeqNum(uint32(c.IRS)+1), - checker.TCPAckNum(uint32(iss)+3), + checker.TCPAckNum(uint32(iss)+9), checker.TCPFlags(header.TCPFlagAck), ), )