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

[ianswett <[e-mail]>]

ianswett commented on this pull request.

Looks good, some editorial comments.

> @@ -417,6 +417,13 @@ to accelerate loss recovery.  The receiver SHOULD send an immediate ACK
 when it receives a new packet which is not one greater than the
 largest received packet number.
 
+Also, reception of an packet marked as ECN Congestion Experience
+(ECN-CE) SHOULD be acknowledged immediately to quicker react to
+congesiton events. Additional ECN-CE marks received during the same

congesiton -> congestion

> @@ -417,6 +417,13 @@ to accelerate loss recovery.  The receiver SHOULD send an immediate ACK
 when it receives a new packet which is not one greater than the
 largest received packet number.
 
+Also, reception of an packet marked as ECN Congestion Experience

I might say "Similarly," instead of "Also," since this is similar to the preceding paragraph.

> @@ -880,13 +892,31 @@ QUIC hosts MUST NOT send packets if they would increase bytes_in_flight
 the packet is a probe packet sent after the TLP or RTO alarm fires, as described
 in {{tlp}} and {{rto}}.
 
+## Explicit Congestion Notification {#congestion-ecn}
+
+If ECN {!RFC3168} has been verified to work for the current path QUIC
+will use the ECN Congestion Experienced (ECN-CE) IP packet marking as a
+signal of congestion as a complement to packet loss. This document
+specifies to use the classical ECN-CE response, i.e. the same response
+as for packet loss. However, there exist potential for future
+experimentation in using other response functions as discussed in
+{!RFC8311}.
+
+The ACK_ECN frame defined in {{QUIC-TRANSPORT}} does not provide
+information on which of the newly acknowledged packets that
+was marked with ECN-CE. Therefore, it will be assumed that

I would suggest rewriting as: "Because newly received marks cause an ACK_ECN be sent immediately, is is assumed the congestion event starts at the highest newly acknowledged packet number."

> @@ -977,6 +1009,12 @@ kLossReductionFactor (default 0.5):
 Variables required to implement the congestion control mechanisms
 are described in this section.
 
+ack_ce_cntr:
+: The ACK_ECN counter for ECN-CE marks previously processed. Used to
+  determine when one or more packet acknowledged by the ACK_ECN frame
+  was marked with ECN-CE.
+

extra linebreak?

> @@ -977,6 +1009,12 @@ kLossReductionFactor (default 0.5):
 Variables required to implement the congestion control mechanisms
 are described in this section.
 
+ack_ce_cntr:
+: The ACK_ECN counter for ECN-CE marks previously processed. Used to

I might suggest calling this previous_ecn_ce or previous_ecn_ce_ctr

> @@ -1042,6 +1081,26 @@ acked_packet from sent_packets.
          kDefaultMss * acked_packet.bytes / congestion_window
 ~~~
 
+### On Packets Marked
+
+Invoked by an increment in the number of CE marked packets, as
+indicated by a newly received ACK_ECN frame when compared to
+ack_ce_cntr.
+
+~~~
+   OnPacketsMarked(ce_counter):
+     if (ce_counter > ack_ce_cntr):
+       // update ack_ce_cntr
+       ack_ce_cntr = ce_counter
+     if (!InRecovery(largest_acked_packet)):

I think we want to use largest newly acked packet ideally.

Also, possibly create a single method like "MaybeStartCongestionEvent" to avoid the code duplication between these methods.

> @@ -1061,20 +1063,21 @@ negotiation, which is the same no matter which reserved version was sent.
 A server MAY therefore send different reserved version numbers in the Version
 Negotiation Packet and in its transport parameters.
 
-A client MAY send a packet using a reserved version number.  This can be used to
-solicit a list of supported versions from a server.
+A client MAY send a packet using a reserved version number. This can be

This change and many that follow are unrelated to this PR.  Can you revert them to make this PR smaller?

> @@ -1410,6 +1416,71 @@ a single packet.
 
 In TLS, the Retry packet type is used to carry the HelloRetryRequest message.
 
+## ECN capability check {#ecn-capability-check}
+
+Explicit Congestion Notification (ECN) {{!RFC3168}} is feature that allows for
+non-destructive congestion notification by a network node.  That is, packets are
+marked instead of being discarded by routers and other devices along a network
+path. QUIC endpoints perform capability verification on connection establishment
+as well as connection migration. The feature can be used unidirectional or

This sentence is awkward, maybe "ECN can be used unidirectionally, bidirectionally, or disabled depending on endpoint and path capabilities."?

> +endpoint individually performs an ECN capability check for its send path. The
+check is performed by attempting to set the ECN bits in the IP header to either
+ECT(0) or ECT(1) for packets generated by the endpoint, clients starting with
+the Initial packet ({{packet-initial}}) and servers with the Handshake packet
+(server) ({{packet-handshake}}). Note that Retry Packets ({{packet-retry}}) can
+be marked as ECT, but as they are not acknowledged by the peer, they provide no
+capability verification.
+
+The choice if ECT(0) and ECT(1) should be used is as per the guidelines in
+{{!RFC8311}}. ECT(0) is the default, if no ECN experiments are run. Upon
+reception of a packet with ECT(0), ECT(1) or ECN-CE marking by the peer
+endpoint, an ACK_ECN frame is transmitted back. This ACK_ECN frame indicates how
+many packets that are marked ECT(0), ECT(1) or ECN-CE. Thus the endpoint can
+determine if the endpoints and path is ECN capable.
+
+The ACK_ECN frame will, when received, confirm that the path direction supports

How about: "When ACK_ECN is received, the counter values can be used to confirm the expected number of marks were received."

> +packets that was marked as ECT in the sender, or if the comparision of the
+counters with the total of acknolwedged packets indicates that not all packets
+arrive as ECT(0), ECT(1) or ECN-CE, then the path is determined as not ECN
+Capable.  If the endpoint determine the path as currently not ECN capable it
+SHALL stop marking the packets as ECT, and instead mark them as Not-ECT. If the
+connection is migrated then the ECN capability check is rerun as specified in
+{{ecn-connection-migration}}.
+
+It is expected that QUIC discards duplicate packets early, however if that is
+not the case **\[ED note, have not seen any clear statement in the drafts]**,
+then it should be verified that the number of ECT marked packets are equal to or
+larger that the amount of ECT marked packets that have been transmitted.
+
+### Continous Verification of ECN {#ecn-continous-verification}
+
+If the ECN capabiity check was successful and the endpoint continus to send ECT

continues

> +connection is migrated then the ECN capability check is rerun as specified in
+{{ecn-connection-migration}}.
+
+It is expected that QUIC discards duplicate packets early, however if that is
+not the case **\[ED note, have not seen any clear statement in the drafts]**,
+then it should be verified that the number of ECT marked packets are equal to or
+larger that the amount of ECT marked packets that have been transmitted.
+
+### Continous Verification of ECN {#ecn-continous-verification}
+
+If the ECN capabiity check was successful and the endpoint continus to send ECT
+marked packets then continous 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.
+
+For each ACK_ECN frame that is received, the total number of ACKed packets are

Suggestion: "For each received ACK_ECN frame, the total number of newly acknowledged packets can be compared to the total increase in ECN counters.  If the increase in ECN counters is larger, then an ECN failure has occurred and ECN should be disabled."

> @@ -2816,6 +2960,10 @@ ACK Block (repeated):
 
 ### Sending ACK Frames
 
+ACK Frames MAY be sent when all packets to be acknowledge had an IP header with

acknowledged

> @@ -2816,6 +2960,10 @@ ACK Block (repeated):
 
 ### Sending ACK Frames
 
+ACK Frames MAY be sent when all packets to be acknowledge had an IP header with
+the ECN field marked as Not-ECT. If any packets where marked as ECT or ECN-CE

where -> were

> @@ -2816,6 +2960,10 @@ ACK Block (repeated):
 
 ### Sending ACK Frames
 
+ACK Frames MAY be sent when all packets to be acknowledge had an IP header with

Also, I think leading with this paragraph here is a bit confusing.  I would prefer not adding anything here about ACK_ECN and placing text in the ACK_ECN section that must be used when new marks are received.

> @@ -4196,6 +4416,23 @@ 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
+
+The ECN bits {{!RFC3168}} are an unauthenticated signal from the network. An
+on-path attacker may manipulate the value of the field. Thus, affecting the

I think this reads more clearly as one sentence.   ie: "An on-path attacker may manipulate the value of the field, affecting the congestion avoidance behavior of the sender."

> @@ -4196,6 +4416,23 @@ 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
+
+The ECN bits {{!RFC3168}} are an unauthenticated signal from the network. An
+on-path attacker may manipulate the value of the field. Thus, 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
+the ECN field to CE marking it can drive down the senders congestion window
+thus resulting in reduced throughput. The later could equally be accomplished
+by dropping packets for the connection. Section 18 and 19 of {{!RFC3168}}
+discusses the effects of undesired manipulation of the ECN field in more
+details.
+
+If a receiver would not have packet duplication detection and not discard any

How about "If a receiver does not discard duplicate packets, an off-path attacker can retransmit packets with ECN bits set and manipulate the senders congestion avoidance state."

> @@ -4196,6 +4416,23 @@ 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
+
+The ECN bits {{!RFC3168}} are an unauthenticated signal from the network. An
+on-path attacker may manipulate the value of the field. Thus, 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
+the ECN field to CE marking it can drive down the senders congestion window
+thus resulting in reduced throughput. The later could equally be accomplished
+by dropping packets for the connection. Section 18 and 19 of {{!RFC3168}}
+discusses the effects of undesired manipulation of the ECN field in more
+details.
+
+If a receiver would not have packet duplication detection and not discard any
+duplicates an off-path attacker that can receive copies of the connection's
+packets can manipulate the senders congestion avoidance state. If packet

packet duplicates -> duplicate packets

-- 
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-127544398