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

@@ -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 {

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++ {

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
 )

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"
 	}

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"))
 	})
 })