From d52e9f35bc58864196226c8d9f7129f14315ee4d Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Tue, 12 Sep 2023 13:18:33 +0700 Subject: [PATCH] ackhandler: detect ECN mangling (#4080) * ackhandler: detect ECN mangling Mangling means that a path is re-marking all ECN-marked packets as CE. * ackhandler: only detect ECN mangling once all testing packets were sent --- internal/ackhandler/ecn.go | 23 +++++++++++++++++------ internal/ackhandler/ecn_test.go | 30 ++++++++++++++++++++++++++++++ logging/types.go | 4 +++- qlog/types.go | 2 ++ qlog/types_test.go | 1 + 5 files changed, 53 insertions(+), 7 deletions(-) diff --git a/internal/ackhandler/ecn.go b/internal/ackhandler/ecn.go index ff0dfe652..bb8b37f83 100644 --- a/internal/ackhandler/ecn.go +++ b/internal/ackhandler/ecn.go @@ -218,7 +218,21 @@ func (e *ecnTracker) HandleNewlyAcked(packets []*packet, ect0, ect1, ecnce int64 return false } + // update our counters + e.numAckedECT0 = ect0 + e.numAckedECT1 = ect1 + e.numAckedECNCE = ecnce + if e.state == ecnStateTesting || e.state == ecnStateUnknown { + // Detect mangling (a path remarking all ECN-marked testing packets as CE). + if e.numSentECT0+e.numSentECT1 == e.numAckedECNCE && e.numAckedECNCE >= numECNTestingPackets { + if e.tracer != nil { + e.tracer.ECNStateUpdated(logging.ECNStateFailed, logging.ECNFailedManglingDetected) + } + e.state = ecnStateFailed + return false + } + var ackedTestingPacket bool for _, p := range packets { if e.isTestingPacket(p.PacketNumber) { @@ -236,12 +250,9 @@ func (e *ecnTracker) HandleNewlyAcked(packets []*packet, ect0, ect1, ecnce int64 } } - // update our counters - e.numAckedECT0 = ect0 - e.numAckedECT1 = ect1 - e.numAckedECNCE = ecnce - - return newECNCE > 0 + // Don't trust CE marks before having confirmed ECN capability of the path. + // Otherwise, mangling would be misinterpreted as actual congestion. + return e.state == ecnStateCapable && newECNCE > 0 } func (e *ecnTracker) ecnMarking(pn protocol.PacketNumber) protocol.ECN { diff --git a/internal/ackhandler/ecn_test.go b/internal/ackhandler/ecn_test.go index 74fff0944..984a97a96 100644 --- a/internal/ackhandler/ecn_test.go +++ b/internal/ackhandler/ecn_test.go @@ -175,6 +175,36 @@ var _ = Describe("ECN tracker", func() { Expect(ecnTracker.HandleNewlyAcked(getAckedPackets(4, 5, 6, 15), 3, 0, 2)).To(BeFalse()) }) + It("detects ECN mangling", func() { + sendAllTestingPackets() + for i := 10; i < 20; i++ { + Expect(ecnTracker.Mode()).To(Equal(protocol.ECNNon)) + ecnTracker.SentPacket(protocol.PacketNumber(i), protocol.ECNNon) + } + // ECN capability not confirmed yet, therefore CE marks are not regarded as congestion events + Expect(ecnTracker.HandleNewlyAcked(getAckedPackets(0, 1, 2, 3), 0, 0, 4)).To(BeFalse()) + Expect(ecnTracker.HandleNewlyAcked(getAckedPackets(4, 5, 6, 10, 11, 12), 0, 0, 7)).To(BeFalse()) + // With the next ACK, all testing packets will now have been marked CE. + tracer.EXPECT().ECNStateUpdated(logging.ECNStateFailed, logging.ECNFailedManglingDetected) + Expect(ecnTracker.HandleNewlyAcked(getAckedPackets(7, 8, 9, 13), 0, 0, 10)).To(BeFalse()) + }) + + It("only detects ECN mangling after sending all testing packets", func() { + tracer.EXPECT().ECNStateUpdated(logging.ECNStateTesting, logging.ECNTriggerNoTrigger) + for i := 0; i < 9; i++ { + Expect(ecnTracker.Mode()).To(Equal(protocol.ECT0)) + ecnTracker.SentPacket(protocol.PacketNumber(i), protocol.ECT0) + Expect(ecnTracker.HandleNewlyAcked(getAckedPackets(protocol.PacketNumber(i)), 0, 0, int64(i+1))).To(BeFalse()) + } + // Send the last testing packet, and receive a + tracer.EXPECT().ECNStateUpdated(logging.ECNStateUnknown, logging.ECNTriggerNoTrigger) + Expect(ecnTracker.Mode()).To(Equal(protocol.ECT0)) + ecnTracker.SentPacket(9, protocol.ECT0) + // This ACK now reports the last testing packets as CE as well. + tracer.EXPECT().ECNStateUpdated(logging.ECNStateFailed, logging.ECNFailedManglingDetected) + Expect(ecnTracker.HandleNewlyAcked(getAckedPackets(9), 0, 0, 10)).To(BeFalse()) + }) + It("declares congestion", func() { sendAllTestingPackets() for i := 10; i < 20; i++ { diff --git a/logging/types.go b/logging/types.go index bfee91c32..0d79b0a90 100644 --- a/logging/types.go +++ b/logging/types.go @@ -121,6 +121,8 @@ const ( ECNFailedLostAllTestingPackets // ECNFailedMoreECNCountsThanSent is emitted when an ACK contains more ECN counts than ECN-marked packets were sent ECNFailedMoreECNCountsThanSent - // ECNFailedTooFewECNCounts is emitted when contains fewer ECT(0) / ECT(1) counts than it acknowledges packets + // ECNFailedTooFewECNCounts is emitted when an ACK contains fewer ECN counts than it acknowledges packets ECNFailedTooFewECNCounts + // ECNFailedManglingDetected is emitted when the path marks all ECN-marked packets as CE + ECNFailedManglingDetected ) diff --git a/qlog/types.go b/qlog/types.go index 49b4853a6..e0c0b2434 100644 --- a/qlog/types.go +++ b/qlog/types.go @@ -364,6 +364,8 @@ func (e ecnStateTrigger) String() string { return "ACK contains more ECN counts than ECN-marked packets sent" case logging.ECNFailedTooFewECNCounts: return "ACK contains fewer new ECN counts than acknowledged ECN-marked packets" + case logging.ECNFailedManglingDetected: + return "ECN mangling detected" default: return "unknown ECN state trigger" } diff --git a/qlog/types_test.go b/qlog/types_test.go index d08eb7392..d6be3e68a 100644 --- a/qlog/types_test.go +++ b/qlog/types_test.go @@ -151,6 +151,7 @@ var _ = Describe("Types", func() { Expect(ecnStateTrigger(logging.ECNFailedLostAllTestingPackets).String()).To(Equal("all ECN testing packets declared lost")) Expect(ecnStateTrigger(logging.ECNFailedMoreECNCountsThanSent).String()).To(Equal("ACK contains more ECN counts than ECN-marked packets sent")) Expect(ecnStateTrigger(logging.ECNFailedTooFewECNCounts).String()).To(Equal("ACK contains fewer new ECN counts than acknowledged ECN-marked packets")) + Expect(ecnStateTrigger(logging.ECNFailedManglingDetected).String()).To(Equal("ECN mangling detected")) Expect(ecnStateTrigger(42).String()).To(Equal("unknown ECN state trigger")) }) })