From bb179edfe6f967bc7e94e7f62fabdff883072380 Mon Sep 17 00:00:00 2001 From: cliffc-spirent Date: Thu, 13 Oct 2022 17:02:28 -1000 Subject: [PATCH 1/4] limiting exponential pto delay to a constant --- internal/ackhandler/sent_packet_handler.go | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/internal/ackhandler/sent_packet_handler.go b/internal/ackhandler/sent_packet_handler.go index 9dbba574635..007d987887c 100644 --- a/internal/ackhandler/sent_packet_handler.go +++ b/internal/ackhandler/sent_packet_handler.go @@ -23,6 +23,8 @@ const ( amplificationFactor = 3 // We use Retry packets to derive an RTT estimate. Make sure we don't set the RTT to a super low value yet. minRTTAfterRetry = 5 * time.Millisecond + // PTO duration is exponential but truncated to the following maximum. + maxPTODuration = 10 * time.Second ) type packetNumberSpace struct { @@ -457,6 +459,14 @@ func (h *sentPacketHandler) getLossTimeAndSpace() (time.Time, protocol.Encryptio return lossTime, encLevel } +func (h *sentPacketHandler) getScaledPTO(includeMaxAckDelay bool) time.Duration { + pto := h.rttStats.PTO(includeMaxAckDelay) << h.ptoCount + if pto > maxPTODuration { + return maxPTODuration + } + return pto +} + // same logic as getLossTimeAndSpace, but for lastAckElicitingPacketTime instead of lossTime func (h *sentPacketHandler) getPTOTimeAndSpace() (pto time.Time, encLevel protocol.EncryptionLevel, ok bool) { // We only send application data probe packets once the handshake is confirmed, @@ -465,7 +475,7 @@ func (h *sentPacketHandler) getPTOTimeAndSpace() (pto time.Time, encLevel protoc if h.peerCompletedAddressValidation { return } - t := time.Now().Add(h.rttStats.PTO(false) << h.ptoCount) + t := time.Now().Add(h.getScaledPTO(false)) if h.initialPackets != nil { return t, protocol.EncryptionInitial, true } @@ -475,18 +485,18 @@ func (h *sentPacketHandler) getPTOTimeAndSpace() (pto time.Time, encLevel protoc if h.initialPackets != nil { encLevel = protocol.EncryptionInitial if t := h.initialPackets.lastAckElicitingPacketTime; !t.IsZero() { - pto = t.Add(h.rttStats.PTO(false) << h.ptoCount) + pto = t.Add(h.getScaledPTO(false)) } } if h.handshakePackets != nil && !h.handshakePackets.lastAckElicitingPacketTime.IsZero() { - t := h.handshakePackets.lastAckElicitingPacketTime.Add(h.rttStats.PTO(false) << h.ptoCount) + t := h.handshakePackets.lastAckElicitingPacketTime.Add(h.getScaledPTO(false)) if pto.IsZero() || (!t.IsZero() && t.Before(pto)) { pto = t encLevel = protocol.EncryptionHandshake } } if h.handshakeConfirmed && !h.appDataPackets.lastAckElicitingPacketTime.IsZero() { - t := h.appDataPackets.lastAckElicitingPacketTime.Add(h.rttStats.PTO(true) << h.ptoCount) + t := h.appDataPackets.lastAckElicitingPacketTime.Add(h.getScaledPTO(true)) if pto.IsZero() || (!t.IsZero() && t.Before(pto)) { pto = t encLevel = protocol.Encryption1RTT From f5d43b00ac03b3c1bbffa79ec304627ad48fb4db Mon Sep 17 00:00:00 2001 From: cliffc-spirent Date: Mon, 17 Oct 2022 11:19:53 -1000 Subject: [PATCH 2/4] updating maximum pto to 60s, referring to rfc --- internal/ackhandler/sent_packet_handler.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/ackhandler/sent_packet_handler.go b/internal/ackhandler/sent_packet_handler.go index 007d987887c..4f4f6d3e8eb 100644 --- a/internal/ackhandler/sent_packet_handler.go +++ b/internal/ackhandler/sent_packet_handler.go @@ -23,8 +23,8 @@ const ( amplificationFactor = 3 // We use Retry packets to derive an RTT estimate. Make sure we don't set the RTT to a super low value yet. minRTTAfterRetry = 5 * time.Millisecond - // PTO duration is exponential but truncated to the following maximum. - maxPTODuration = 10 * time.Second + // PTO duration is exponential but truncated to the following maximum as allowed by RFC 8961 Section 4.4. + maxPTODuration = 60 * time.Second ) type packetNumberSpace struct { From de80d4a63ae36e672aeb81f7e46f5e470c3df90f Mon Sep 17 00:00:00 2001 From: cliffc-spirent Date: Thu, 20 Oct 2022 11:14:31 -1000 Subject: [PATCH 3/4] updating exponential backoff test for truncation fixed an issue where a large number of lost packets would cause the scaled PTO to rollover/overflow to negative and then zero updating a related test that was using a large constant delay and would run up against the new max pto limit --- internal/ackhandler/sent_packet_handler.go | 2 +- internal/ackhandler/sent_packet_handler_test.go | 12 +++++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/internal/ackhandler/sent_packet_handler.go b/internal/ackhandler/sent_packet_handler.go index 4f4f6d3e8eb..d68405f1a30 100644 --- a/internal/ackhandler/sent_packet_handler.go +++ b/internal/ackhandler/sent_packet_handler.go @@ -461,7 +461,7 @@ func (h *sentPacketHandler) getLossTimeAndSpace() (time.Time, protocol.Encryptio func (h *sentPacketHandler) getScaledPTO(includeMaxAckDelay bool) time.Duration { pto := h.rttStats.PTO(includeMaxAckDelay) << h.ptoCount - if pto > maxPTODuration { + if (pto > maxPTODuration) || (pto <= 0) { return maxPTODuration } return pto diff --git a/internal/ackhandler/sent_packet_handler_test.go b/internal/ackhandler/sent_packet_handler_test.go index c39b50f48df..2663a69bbe9 100644 --- a/internal/ackhandler/sent_packet_handler_test.go +++ b/internal/ackhandler/sent_packet_handler_test.go @@ -687,6 +687,14 @@ var _ = Describe("SentPacketHandler", func() { handler.ptoCount = 2 handler.setLossDetectionTimer() Expect(handler.GetLossDetectionTimeout().Sub(sendTime)).To(Equal(4 * timeout)) + // truncated when the exponential gets too large + handler.ptoCount = 20 + handler.setLossDetectionTimer() + Expect(handler.GetLossDetectionTimeout().Sub(sendTime)).To(Equal(maxPTODuration)) + // protected from rollover + handler.ptoCount = 100 + handler.setLossDetectionTimer() + Expect(handler.GetLossDetectionTimeout().Sub(sendTime)).To(Equal(maxPTODuration)) }) It("reset the PTO count when receiving an ACK", func() { @@ -1036,7 +1044,7 @@ var _ = Describe("SentPacketHandler", func() { }) It("correctly sets the timer after the Initial packet number space has been dropped", func() { - handler.SentPacket(initialPacket(&Packet{PacketNumber: 1, SendTime: time.Now().Add(-42 * time.Second)})) + handler.SentPacket(initialPacket(&Packet{PacketNumber: 1, SendTime: time.Now().Add(-19 * time.Second)})) _, err := handler.ReceivedAck( &wire.AckFrame{AckRanges: []wire.AckRange{{Smallest: 1, Largest: 1}}}, protocol.EncryptionInitial, @@ -1048,6 +1056,8 @@ var _ = Describe("SentPacketHandler", func() { pto := handler.rttStats.PTO(false) Expect(pto).ToNot(BeZero()) + // pto is approximately 19 * 3. Using a number > 19 above will + // run into the maxPTODuration limit Expect(handler.GetLossDetectionTimeout()).To(BeTemporally("~", time.Now().Add(pto), 10*time.Millisecond)) }) From 9ec71c12c5d7293bf1fa16d1edb2cfe1293cb8d5 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Tue, 15 Nov 2022 14:58:41 -0800 Subject: [PATCH 4/4] Apply suggestions from code review --- internal/ackhandler/sent_packet_handler.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/ackhandler/sent_packet_handler.go b/internal/ackhandler/sent_packet_handler.go index d68405f1a30..12ffb918aa7 100644 --- a/internal/ackhandler/sent_packet_handler.go +++ b/internal/ackhandler/sent_packet_handler.go @@ -23,7 +23,7 @@ const ( amplificationFactor = 3 // We use Retry packets to derive an RTT estimate. Make sure we don't set the RTT to a super low value yet. minRTTAfterRetry = 5 * time.Millisecond - // PTO duration is exponential but truncated to the following maximum as allowed by RFC 8961 Section 4.4. + // The PTO duration uses exponential backoff, but is truncated to a maximum value, as allowed by RFC 8961, section 4.4. maxPTODuration = 60 * time.Second ) @@ -461,7 +461,7 @@ func (h *sentPacketHandler) getLossTimeAndSpace() (time.Time, protocol.Encryptio func (h *sentPacketHandler) getScaledPTO(includeMaxAckDelay bool) time.Duration { pto := h.rttStats.PTO(includeMaxAckDelay) << h.ptoCount - if (pto > maxPTODuration) || (pto <= 0) { + if pto > maxPTODuration || pto <= 0 { return maxPTODuration } return pto