Full rewrite #3

janaiyengar · 2018-06-25T23:12:43Z

Several iterations later, I think I've rewritten the entire thing. PTAL.

martinthomson

I like these changes. They tighten things up a lot. I have a few small suggestions throughout. The only thing that gives me pause is the lost ACK problem I describe in more detail. I would be OK with taking that on in a follow-up though.

martinthomson · 2018-06-25T23:56:39Z

draft-ietf-quic-recovery.md

-to experiment with other response functions.
+codepoint in the IP header as a signal of congestion. This document specifies an
+endpoint's response when its peer receives packets with the Congestion
+Experienced codepoint.  As discussed in {!RFC8311}, endpoints are permitted to


Two braces {{!RFC8311}}.

martinthomson · 2018-06-25T23:59:54Z

draft-ietf-quic-recovery.md

+       // Start a new recovery epoch if the largest acked packet
+       // is larger than the end of the previous recovery epoch.
+       if (!InRecovery(ack.largest_acked_packet)):
+         NewCongestionEvent()


I would usually recommend moving the InRecovery() call to NewCongestionEvent() here. Why did you de-factor this?

martinthomson · 2018-06-26T00:00:12Z

draft-ietf-quic-recovery.md

@@ -1112,7 +1109,8 @@ are detected lost.
     largest_lost_packet = lost_packets.last()
     // Start a new recovery epoch if the lost packet is larger
     // than the end of the previous recovery epoch.
-     CongestionEvent(largest_lost_packet.packet_number)
+     if (!InRecovery(packet_number)):


I think that largest_lost_packet.packet_number is still correct.

martinthomson · 2018-06-26T00:01:39Z

draft-ietf-quic-transport.md


-### Continuous Verification of ECN {#ecn-continuous-verification}
+QUIC endpoints SHOULD use Explicit Congestion Notification (ECN) {{!RFC3168}} to


s/SHOULD// - let's not recommend this, let's assume that it is done. Better that way I think.

Agree with Martin here.

martinthomson · 2018-06-26T00:02:23Z

draft-ietf-quic-transport.md

-ECT marked packets then continuous verification is applied. This is to detect
-any cases when ECN field is bleached, that is, zeroed out by a network node,
-likely as the result of a routing changes since the ECN capability check.
+To use ECN, QUIC endpoints must first determine whether a path and peer support


martinthomson · 2018-06-26T00:08:27Z

draft-ietf-quic-transport.md

+
+If a packet sent with an ECT codepoint is newly acknowledged by the peer in an
+ACK frame, the endpoint stops setting ECT codepoints in subsequent packets, with
+the expectation that either the network or the peer no longer supports ECN.


I would merge this with the previous paragraph.

I'd like to keep this separate, since this action is independent of why an ACK frame is received (instead of an ACK_ECN frame). The para above is one reason that might happen.

martinthomson · 2018-06-26T00:08:36Z

draft-ietf-quic-transport.md

+ACK frame, the endpoint stops setting ECT codepoints in subsequent packets, with
+the expectation that either the network or the peer no longer supports ECN.
+
+A network device may bleach the ECN codepoint in the IP header by setting it to


martinthomson · 2018-06-26T00:18:49Z

draft-ietf-quic-transport.md

+  ACK_ECN frame.
+
+* The increase in ECT(0) and ECT(1) counters MUST be no greater than the number
+  of packets newly acknowledged that were sent with the corresponding codepoint.


I'm a little concerned that this makes things fragile (i.e., ECN will be disabled a little too aggressively), but I think that it's still correct. I think that with a good path, there is a failure mode that I think we need to address: lost ACKs. This is something in Magnus' original PR, and I think that it's worth being a little careful.

Say a marked packet gets through and gets acknowledged, but the acknowledgment is lost. And the recipient abandons the ACK. The counters will increase, but the number of newly acknowledged packets will be lower than the increase to the counters. That suggests a tweak, though it complicates things a little:

If the acknowledgments cover a range of packet numbers that extend past (or adjacent to) the largest acknowledged packet number that was previously acknowledged, then the counters must match precisely. If the acknowledgments do not extend to the largest previously acknowledged packet number, then the numbers MAY increase by the number of unacknowledged packets between the previously largest acknowledged and the newly lowest acknowledged.

That is, if an endpoint has seen an ACK_ECN for packet 10, then receives a new ACK_ECN for packet 20-30, it needs to allow for an increase in the counters for packets 11 through 19 (inclusive). It can then reset both the largest acknowledged and the values it tracks for each counter.

Fully agree with Martin here. I think the sender needs to accept counter increases larger than newly acknowledged if there are gaps in the ACKs.

I deliberately left this out; my plan is to file an issue and do this separately. In most cases, we expect the receiver to repeat ack information adequately for it to not matter. That said, it does need to be covered.

martinthomson · 2018-06-26T00:22:29Z

draft-ietf-quic-transport.md

+An ACK_ECN frame contains all the elements of the ACK frame ({{frame-ack}}) with
+the addition of three counts corresponding to the total number of packets
+received with those codepoints -- ECT(0), ECT(1), and CE -- since the beginning
+of the connection.


We should retain the enumeration of new fields, though it could be more terse.

ECT(0) Count: : A variable-length integer representing the number of ECT(0) marked packets received.

Also, we can't assume that the diagram is accessible, so it probably pays to say where the new field are inserted.

Good point on accessibility.

martinthomson · 2018-06-26T00:22:54Z

draft-ietf-quic-transport.md

-retransmit packets with ECN bits set and manipulate the senders congestion
-avoidance state. If duplicate packets are discarded, the off-path attacker will
-need to race the original packet to be successful in this attack.
+An on-path attacker may manipulate the value of ECN codepoints in the IP header


s/may/could/ - may implies that this is permissible :)

it's not? you mean... stuff happens even when the IETF does not permit it? :-P

gloinul

A number of things needs edits before this can replace what is currently in the pull request.

gloinul · 2018-06-26T06:27:01Z

draft-ietf-quic-transport.md


-### Continuous Verification of ECN {#ecn-continuous-verification}
+QUIC endpoints SHOULD use Explicit Congestion Notification (ECN) {{!RFC3168}} to


Agree with Martin here.

gloinul · 2018-06-26T06:31:16Z

draft-ietf-quic-transport.md

+the ECN Capable Transport (ECT) codepoints -- ECT(0) or ECT(1) -- in the IP
+header {{!RFC8311}} of all outgoing packets.
+
+If an ECT codepoint set in the IP header is not bleached or otherwise corrupted


I agree with Bleach being jargon, and what we might need to care about is also ECT(0) <-> ECT(1) changes by the network which isn't covered by Martin's suggestion. Maybe "... the path does not remove ECN markings or changes the applied ECT code point."

gloinul · 2018-06-26T06:35:41Z

draft-ietf-quic-transport.md

+codepoints when necessary (see {{processing-and-ack}}) with an ACK_ECN frame
+carrying the current state of its ECN counters (see {{frame-ack-ecn}}).
+
+If an endpoint receives a packet without an ECT or CE codepoint, it responds per


So, my proposal using three counters so far is using ACK_ECN frame as long as there are any ECN marked packets in the range being acknowledged. This formulation can be interpreted such that ACK frames shall be used to acknowledge all packets that are unmarked. This is a possible change and would be step in the direction of the two counter proposal as well as making it clearer which packets wasn't marked allowing better sender verification if it like to check things by not marking all packets. Is this your intention here?

Yes, this is a change from your proposal, but it is necessary to be able to detect all failure modes correctly.

There's a change earlier in the text that basically says that an endpoint marks all packets if it wants to do ECN. The idea here is that if an ACK is received at all, the endpoint can simply turn off ECN, since that only happens if the network or the peer don't support ECN.

gloinul · 2018-06-26T06:38:11Z

draft-ietf-quic-transport.md

+  ACK_ECN frame.
+
+* The increase in ECT(0) and ECT(1) counters MUST be no greater than the number
+  of packets newly acknowledged that were sent with the corresponding codepoint.


Fully agree with Martin here. I think the sender needs to accept counter increases larger than newly acknowledged if there are gaps in the ACKs.

gloinul · 2018-06-26T07:03:14Z

draft-ietf-quic-transport.md

+{{migration-cc}}.
+
+The new path might not have the same ECN capability. Therefore, the endpoint
+verifies ECN capability as described in {{ecn-connection-migration}}.


Reference to a section you have removed below ecn-connection-migration

janaiyengar · 2018-06-26T20:53:12Z

@gloinul : if this is basically good, can you merge so we can get this into the draft?

martinthomson · 2018-06-27T00:04:42Z

I merged this directly in the draft. There is the one issues that @janaiyengar opened, but aside from that this is good.

Align with origin

janaiyengar added 3 commits June 25, 2018 16:11

a few changes

c37afbb

Merge branch 'ecn' into ecn

f40dc9d

remove ecn block

490dc4f

martinthomson approved these changes Jun 26, 2018

View reviewed changes

gloinul reviewed Jun 26, 2018

View reviewed changes

mt's and magnus's comments

9acabc9

janaiyengar mentioned this pull request Jun 26, 2018

Fragility in ECN counters with lost acks quicwg/base-drafts#1481

Closed

gloinul merged commit b931633 into gloinul:ecn Jun 27, 2018

gloinul pushed a commit that referenced this pull request Sep 4, 2018

Merge pull request #3 from quicwg/master

94f4db9

Align with origin

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Full rewrite #3

Full rewrite #3

janaiyengar commented Jun 25, 2018

martinthomson left a comment

martinthomson Jun 25, 2018

martinthomson Jun 25, 2018

martinthomson Jun 26, 2018

martinthomson Jun 26, 2018

gloinul Jun 26, 2018

martinthomson Jun 26, 2018

martinthomson Jun 26, 2018

janaiyengar Jun 26, 2018

martinthomson Jun 26, 2018

janaiyengar Jun 26, 2018

martinthomson Jun 26, 2018

gloinul Jun 26, 2018

janaiyengar Jun 26, 2018

martinthomson Jun 26, 2018

janaiyengar Jun 26, 2018

martinthomson Jun 26, 2018

janaiyengar Jun 26, 2018

gloinul left a comment

gloinul Jun 26, 2018

gloinul Jun 26, 2018

gloinul Jun 26, 2018

janaiyengar Jun 26, 2018

gloinul Jun 26, 2018

gloinul Jun 26, 2018

janaiyengar commented Jun 26, 2018

martinthomson commented Jun 27, 2018


		### Continuous Verification of ECN {#ecn-continuous-verification}
		QUIC endpoints SHOULD use Explicit Congestion Notification (ECN) {{!RFC3168}} to

Full rewrite #3

Full rewrite #3

Conversation

janaiyengar commented Jun 25, 2018

martinthomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gloinul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janaiyengar commented Jun 26, 2018

martinthomson commented Jun 27, 2018