Re: [quicwg/base-drafts] Proposal for adding ECN support to QUIC. (#1372)

[Martin Thomson <[e-mail]>]

martinthomson commented on this pull request.



>  
 Details of how TLS is integrated with QUIC is provided in more detail in
 {{QUIC-TLS}}.
 
 
 ## Transport Parameters
 
-During connection establishment, both endpoints make authenticated declarations
-of their transport parameters.  These declarations are made unilaterally by each
-endpoint.  Endpoints are required to comply with the restrictions implied by
-these parameters; the description of each parameter includes rules for its
-handling.
+During connection establishment, both endpoints make authenticated
+declarations of their transport parameters. These declarations are made
+unilaterally by each endpoint. Endpoints are required to comply with the
+restrictions implied by these parameters; the description of each
+parameter includes rules for its handling.
+

Please keep to one blank line between paragraphs.

> @@ -1410,6 +1416,68 @@ a single packet.
 
 In TLS, the Retry packet type is used to carry the HelloRetryRequest message.
 
+## ECN capability check {#ecn-capability-check}

Title Case for Headings

> @@ -2882,6 +3020,83 @@ by a client in protected packets, because it is certain that the server is able
 to decipher the packet.
 
 
+## ACK_ECN Frame {#frame-ack-ecn}
+
+A QUIC connection MUST keep counters for each ECN codepoint, recording the
+number of packets that were received with the corresponding ECN codepoint in
+the IP header. If the header is not readable from the application, the
+codepoint 00 (Not-ECT) MUST be assumed.
+
+ACK_ECN Frame MUST be used when when an endpoint is acknowledging a packet were
+the IP header ECN field was marked as ECT(0), ECT(1) or ECN-CE when received.

oxford comma

> @@ -4196,6 +4410,21 @@ limit mitigates the effect of the stream commitment attack.  However, setting
 the limit too low could affect performance when applications expect to open
 large number of streams.
 
+## Explicit Congestion Notification Attacks
+
+An on-path attacker may manipulate the value of the field, affecting the
+congestion avoidance behavior of the sender. By clearing any CE marks the
+connection can help drive a bottle neck queue into a loss regime. By setting

Removing any ECN-CE marking causes senders to maintain or increase their sending rate beyond that the path can sustain, which will eventually result in loss.  Adding an ECN-CE marking causes senders to reduce their sending rate.

> +|              Number of ECT(0) marked packets (i)            ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|              Number of ECT(1) marked packets (i)            ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|              Number of ECN-CE marked packets (i)            ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+~~~
+{: #ECN-BLOCK-FIG title="ECN Block"}
+
+
+### ECN counters
+
+The receiver side should implement three 64-bit counters that are copied to the
+ECN block when an ACK_ECN frame is generated:
+
+ECT_0:

Please make these match the diagram.  "ECT(0) Count" might be more concise.

> + 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|              Number of ECT(0) marked packets (i)            ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|              Number of ECT(1) marked packets (i)            ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|              Number of ECN-CE marked packets (i)            ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+~~~
+{: #ECN-BLOCK-FIG title="ECN Block"}
+
+
+### ECN counters
+
+The receiver side should implement three 64-bit counters that are copied to the
+ECN block when an ACK_ECN frame is generated:

Rather than phrase this as "implement these counters" just define the fields in the frame.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#pullrequestreview-128169112