Subject: Re: [quicwg/base-drafts] Make language in ECN validation clearer (#3777)

From: Martin Thomson <[email-masked]>
Date: Wed, 17 Jun 2020 23:01:24 -0700

@martinthomson pushed 1 commit.

0cc27c7fc32445326bd0ea7b3a3d7529c585fdcc  Remove normative change

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3777/files/5e06b318a75fdb6bb936bd63ec84ec70e70f6a6a..0cc27c7fc32445326bd0ea7b3a3d7529c585fdcc

---

From: Martin Thomson <[email-masked]>
Date: Thu, 18 Jun 2020 06:03:44 +0000 (UTC)

This avoids &quot;no smaller than&quot;, unqualified &quot;total increase&quot;, and other such ambiguities.

However, in doing this, I realized that I might interpret the original language as allowing a network to ADD markings.  That seems wrong.  But it&#39;s a non-editorial change.  This version has that change.  I&#39;ll make another PR with the hole left in place in case I&#39;m wrong.

Closes #3733.
You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/3777

-- Commit Summary --

  * Make language in ECN validation clearer
  * Remove normative change

-- File Changes --

    M draft-ietf-quic-transport.md (31)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/3777.patch
https://github.com/quicwg/base-drafts/pull/3777.diff

-- 
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/3777

---

From: Martin Thomson <[email-masked]>
Date: Wed, 17 Jun 2020 23:04:37 -0700

OK, I realize now my mistake.  The draft allows for an increase because of the possibility of ACK loss.  Counts might increase more than there are newly acknowledged packets because an endpoint might not receive an ACK for every packet that is accounted for in the increased count.  I think that's worth a note.  

Though now I'm concerned about the third clause, which doesn't include any protection against this problem.  I'll open an issue for that.

-- 
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/3777#issuecomment-645791453

---

From: Martin Thomson <[email-masked]>
Date: Wed, 17 Jun 2020 23:42:58 -0700

2c168299e72a17ca9aff36c529f49fb9710c7bcb  Don't use MUST for ECN validation

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3777/files/0cc27c7fc32445326bd0ea7b3a3d7529c585fdcc..2c168299e72a17ca9aff36c529f49fb9710c7bcb

---

From: mirjak <[email-masked]>
Date: Thu, 18 Jun 2020 00:09:03 -0700

@mirjak commented on this pull request.

> -  packets that were sent with an ECT codepoint.  This step detects any network
-  remarking from ECT(0), ECT(1), or CE codepoints to Not-ECT.
-
-* Any increase in either ECT(0) or ECT(1) counts, plus any increase in the CE
-  count, MUST NOT exceed the number of packets sent with the corresponding ECT
-  codepoint that are newly acknowledged in this ACK frame.  This step detects
-  any erroneous network remarking from ECT(0) to ECT(1) (or vice versa).
+  either ECT(0) or ECT(1) codepoints set, ECN validation fails if ECN counts
+  are not present in the ACK frame.  This step protects against both a network
+  element that zeroes out ECN bits and a peer that is unable to access ECN
+  markings, since the peer could respond without ECN feedback in either case.
+
+* ECN validation fails if the sum of the increases to ECT(0), ECT(1), and CE
+  counts in the ACK frame are less than the number of newly acknowledged
+  packets that were sent with an ECT codepoint.  This step detects any
+  network remarking from ECT(0), ECT(1), or CE codepoints to Not-ECT.

As you use the word "erroneous" below you should also use it here, however, effectively it unknown if this is intentional or not. Most cases we observes it old boxes that still set the whole byte as ToS field. I would actually also recommend to use the term "overwriting" instead of "remarking" as that probably more indicates that, not matter why this happens, it shouldn't.

-- 
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/3777#pullrequestreview-432996998

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 18 Jun 2020 14:18:10 -0700

@janaiyengar approved this pull request.

A few suggestions, but LGTM.

> +An endpoint that sets ECT(0) or ECT(1) codepoints on packets records the ECN
+counts it has received in ACK frames. When processing an ACK frame, the
+endpoint MUST validate the increase in ECN counts based on the markings that
+were applied to packets that are newly acknowledged.

```suggestion
An endpoint that sets ECT(0) or ECT(1) codepoints on packets records the ECN
counts it receives in ACK frames. When processing an ACK frame, the endpoint
MUST validate any increases from previously received ECN counts, as follows:
```
... and drop the line below about ECN validation.

-* For validation to succeed, the total increase in ECT(0), ECT(1), and CE counts
-  MUST be no smaller than the total number of QUIC packets sent with an ECT
-  codepoint that are newly acknowledged in this ACK frame.  This step detects
-  any network remarking from ECT(0), ECT(1), or CE codepoints to Not-ECT.
+* ECN validation fails if the sum of the increases to ECT(0), ECT(1), and CE
+  counts in the ACK frame are less than the number of newly acknowledged
+  packets that were sent with an ECT codepoint.  This step detects any
+  network remarking from ECT(0), ECT(1), or CE codepoints to Not-ECT.

```suggestion
  erroneous network overwriting from ECT(0), ECT(1), or CE codepoints to
  Not-ECT.
```

>    This step detects any erroneous network remarking from ECT(0) to ECT(1) (or
   vice versa).

```suggestion
  This step detects any erroneous network overwriting from ECT(0) to ECT(1) (or
  vice versa).
```

+ECN validation involves the following checks:

```suggestion
```

-- 
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/3777#pullrequestreview-433642378

---

From: Martin Thomson <[email-masked]>
Date: Thu, 18 Jun 2020 21:53:46 -0700

@martinthomson pushed 2 commits.

49507b4c9e38c201919a7241d70cc45b2f962fe0  Rework ECN validation rules
b760aa929a353f76cddf6e0fc0ff6c417d66d703  Remove the complicated check, simplify

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3777/files/2c168299e72a17ca9aff36c529f49fb9710c7bcb..b760aa929a353f76cddf6e0fc0ff6c417d66d703

---

From: Martin Thomson <[email-masked]>
Date: Thu, 18 Jun 2020 22:02:00 -0700

7ed70077c8006566c7acd27bbc06fe131140580e  Tweak language more

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3777/files/b760aa929a353f76cddf6e0fc0ff6c417d66d703..7ed70077c8006566c7acd27bbc06fe131140580e

---

From: Martin Thomson <[email-masked]>
Date: Thu, 18 Jun 2020 22:04:18 -0700

Sorry to ask for another review, but I had a few misfires yesterday (apparently).  I think that this structure is much better.

I have removed the check that uses the sum of ECT0+ECT1+CE.  Any check on ECT0+CE or ECT1+CE is more precise anyway.  The sum is only able to detect double-counting of CE-marked packets, but as the check is an inequality, we would allow what to happen.  (That was what motivated my proposed additional check, but I agree that is not worth documenting.)

-- 
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/3777#issuecomment-646436608

---

From: Jana Iyengar <[email-masked]>
Date: Fri, 19 Jun 2020 02:55:41 -0700

@janaiyengar commented on this pull request.

Thank you for patiently working through this! This is complicated, so I totally get that it needs doing a few times over.

> -the largest packet number acknowledged in this connection.
+Erroneous application of markings in the network can result in ECN marking
+providing bad information.  Before using ECN counts, an endpoint validates the
+ECN counts by comparing the counts in each ACK frame it processes against the
+last ACK frame that was successfully processed.  The increase in ECN counts is
+validated based on the markings that were applied to packets that are newly
+acknowledged in the ACK frame.
+
+If an ACK frame newly acknowledges a packet that the endpoint sent with either
+ECT(0) or ECT(1) codepoints set, ECN validation fails if ECN counts are not
+present in the ACK frame.  This check detects a network element that zeroes out
+ECN bits or a peer that is unable to access ECN markings.
+
+ECN validation fails if the sum of the increase in ECT(0) and ECN-CE counts is
+less than the number of newly acknowledged packets that were originally sent
+with an ECT(0) marking.  Similarly, if the sum of the increases to ECT(1) and

"Similarly, ECN validation fails if ..."

 An endpoint could miss acknowledgements for a packet when ACK frames are lost.
-It is therefore possible for the total increase in ECT(0), ECT(1), and CE counts
-to be greater than the number of packets acknowledged in an ACK frame.  When
-this happens, and if validation succeeds, the local reference counts MUST be
-increased to match the counts in the ACK frame.
+It is therefore possible for the total increase in ECT(0), ECT(1), and ECN-CE
+counts to be greater than the number of packets acknowledged in an ACK frame.
+This is why counts are permitted to be larger than might be accounted for by
+newly acknowledged packets.

```suggestion
newly acknowledged packets. When this happens, and if validation succeeds, the
local reference counts MUST be increased to match the counts in the ACK frame.
```

We still want this bit, right? Ah, I guess we don't need this because the validation is against the counts from the last processed ACK frame. That works for me. Does this make this a design change?

> -to be greater than the number of packets acknowledged in an ACK frame.  When
-this happens, and if validation succeeds, the local reference counts MUST be
-increased to match the counts in the ACK frame.
+It is therefore possible for the total increase in ECT(0), ECT(1), and ECN-CE
+counts to be greater than the number of packets acknowledged in an ACK frame.
+This is why counts are permitted to be larger than might be accounted for by
+newly acknowledged packets.
+
+ECN validation MAY fail if the total count for an ECT(0) or ECT(1) marking
+exceeds the total number of packets sent with the corresponding marking.   In
+particular, an endpoint that never applies a particular marking can fail
+validation when a non-zero count for the corresponding marking is received.
+This check can detect when packets are marked ECT(0) or ECT(1) in the network.
+
+Processing ECN counts out of order can result in validation failure.  An
+endpoint SHOULD skip ECN validation when an ACK frame does not increase the

```suggestion
endpoint SHOULD skip ECN validation on an ACK frame that does not increase the
```

> +Erroneous application of markings in the network can result in ECN marking
+providing bad information.  Before using ECN counts, an endpoint validates the
+ECN counts by comparing the counts in each ACK frame it processes against the
+last ACK frame that was successfully processed.  The increase in ECN counts is
+validated based on the markings that were applied to packets that are newly
+acknowledged in the ACK frame.

```suggestion
Erroneous application of ECN marks in the network can result in degraded
connection performance.  An endpoint that receives an ACK frame with ECN
counts therefore validates the counts before using them. An endpoint validates
these counts by comparing newly received ones against those from the last
successfully processed ACK frame. Any increase in ECN counts is validated
based on the markings that were applied to packets that are newly
acknowledged in the ACK 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/3777#pullrequestreview-433936915

---

From: Martin Thomson <[email-masked]>
Date: Fri, 19 Jun 2020 03:48:42 -0700

@martinthomson commented on this pull request.

Yep.  We never really said what value you save, but implied that you took the value from the last ACK.  This simplifies and says "last ACK".  I think that's non-design.

-- 
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/3777#discussion_r442770230

---

From: Martin Thomson <[email-masked]>
Date: Fri, 19 Jun 2020 03:50:31 -0700

56370b0f6111a90f61a6800bafea05515bf65f45  Small improvements

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3777/files/7ed70077c8006566c7acd27bbc06fe131140580e..56370b0f6111a90f61a6800bafea05515bf65f45

---

From: Martin Thomson <[email-masked]>
Date: Fri, 19 Jun 2020 03:52:07 -0700

911dbe41ac275d8c3f188fe07e768ee57b1069d9  Similarly

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3777/files/56370b0f6111a90f61a6800bafea05515bf65f45..911dbe41ac275d8c3f188fe07e768ee57b1069d9

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 23 Jun 2020 15:27:31 -0700

> +counts therefore validates the counts before using them. An endpoint validates
+these counts by comparing newly received counts against those from the last

```suggestion
counts therefore validates the counts before using them. It performs this
validation by comparing newly received counts against those from the last
```

-- 
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/3777#pullrequestreview-436202859

---

From: Martin Thomson <[email-masked]>
Date: Tue, 23 Jun 2020 15:27:55 -0700

b3efc66dfc41fee56bf0663ad766f83fe80df4eb  it performs...

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3777/files/911dbe41ac275d8c3f188fe07e768ee57b1069d9..b3efc66dfc41fee56bf0663ad766f83fe80df4eb

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 23 Jun 2020 15:28:15 -0700

-- 
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/3777#pullrequestreview-436203208

---

From: Martin Thomson <[email-masked]>
Date: Tue, 23 Jun 2020 15:37:19 -0700

Merged #3777 into master.

-- 
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/3777#event-3475068047