Re: [tcpm] Reviewed for draft-ietf-tcpm-accurate-ecn-23

[Bob Briscoe <[e-mail]>]

Gorry,

Thank you as always for your extensive review. See responses tagged [BB] 
inline.
No comment = proposal accepted.

On 25/03/2023 17:28, Gorry Fairhurst wrote:
> I have just reviewed draft-ietf-tcpm-accurate-ecn-23 for a WGLC.
>
> This ID is significantly easier to read than an earlier version I 
> read, but still is not an easy read - and I did not have time to 
> re-check all the logic, but I did look at the text and have the 
> following set of comments. Most of these relate to sentences were 
> clarity could be improved, or RFC2119 language could be more explicit 
> about the subject. Many of these could be resolved by simple text 
> changes, and where possible I have provided the text from the ID, and 
> in most cases a suggestion remedy.
>
> Best wishes,
>
> Gorry
>
> P.S. When you complete all the updates after the WGLC, I suggest you 
> post a review request to TSVWG, just to keep the process good - 
> because this does plan to update a TSVWG spec.
>
> =========
>
> This statement causes concern:
> AccECN is intended to be a complete replacement for classic TCP/ECN 
> feedback, not a fork in the design of TCP.
> Is it possible the intent is:
> AccECN is intended to offer a complete replacement for classic TCP/ECN 
> feedback, not a fork in the design of TCP.
> - To me the importance being that this does not immediately obsolete 
> current TCP, it simply provides a negotiated mechanism to replace it.
>
> ===================
> Section 1
> ===
> Recently, proposed mechanisms like Congestion Exposure (ConEx 
> [RFC7713]), DCTCP [RFC8257] or L4S [RFC9330] need to know when more 
> than one marking is received in one RTT which is information that 
> cannot be provided by the feedback scheme as specified in [RFC3168].
> - Add comma, so that:
> Recently, proposed mechanisms like Congestion Exposure (ConEx 
> [RFC7713]), DCTCP [RFC8257] or L4S [RFC9330] need to know when more 
> than one marking is received in one RTT, which is information that 
> cannot be provided by the feedback scheme as specified in [RFC3168].
> ===
> This document specifies an update to the ECN feedback scheme of RFC 
> 3168 that provides more accurate information and could be used by 
> these and potentially other future TCP extensions.
> - could we note this is an alternative rather than replacement:
> This document specifies an update to the ECN feedback scheme of RFC 
> 3168 that provides an alternative more accurate information and could 
> be used by these and potentially other future TCP extensions.
> ===
> This documents specifies a standards track scheme
> - remove s
> This document specifies a standards track scheme
> ===
> Then terminology is defined
> - do we need a comma:
> Then, terminology is defined
> ===
> Section 1.2
> ===
> Therefore an AccECN receiver aims to act as a generic (dumb) reflector 
> of congestion information so that in future new sender behaviours can 
> be deployed unilaterally.
> - why /aims to/ ... why does this not specify this?
> - is /dumb/ necessary, it's an unfortunate word choice - simple could 
> be alternative?

[BB] Yes, 'aims to' should be moved to the later part of the sentence.
The terminology 'dumb' was borrowed from the "intelligent vs dumb 
network" phrasing of the e2e argument. 'Simple' would lose the intended 
nuance of 'mechanistic' or 'robotic', and the former is least overloaded 
with other meanings these days (robotic is starting to mean the 
opposite!). So how about:

     Therefore an AccECN receiver acts as a generic (mechanistic) 
reflector of congestion information with the aim that in future new 
sender behaviours can be deployed unilaterally.

There are other occurrences of 'dumb reflector' - so I'll change them all.


> ===
> Classic ECN: the ECN protocol specified in [RFC3168].
> - Upper case /The/ to be consistent?
> ===
> Classic ECN feedback: the
> - Upper case /The/ to be consistent?
> ===
> which ensures the signal is received reliably even if ACKs are lost.
> - well not if all are lost! Could this be:
> which provides resilience of the signal to loss or reordering of ACKs.
> ====
> Para after table talks of a TCP sender, but the terminology talks of a 
> data sender and a TCP server; but not a TCP sender. Presumably this is 
> a data sender.
> ====
> - It might be useful to say that a TCP connection between a TCP client 
> and a TCP server provides bidirectional communication between two 
> pairs of TCP senders and TCP receivers, forming two independent half 
> connections/.

[BB] Yes, we use half connection a number of times later, and this would 
be a good place to introduce it.

> ====
> I think the whole of section 2 could be informative? ~If it is, can we 
> say this?

[BB] We do already; in the first para of §2:
"This section provides an informative overview of the AccECN protocol 
that will be normatively specified in Section 3 
<https://datatracker.ietf.org/doc/html/draft-ietf-tcpm-accurate-ecn-23#accecn_Spec>"

> ====
> The RFC 2119 conformance terms are defined in 1.3, but then are 
> followed by an informative set of sections. I for one, would much 
> prefer that definition block moved to the start of section 3.

[BB] Much of the other terminology defined in §1.3 is needed sooner. So, 
to do as you ask, we'd have to to split up the terminology section, 
which I don't want to do. It's not incorrect as it is, so let's leave it.

> ===
> but it is more likely to suffer from middlebox interference.
> - I think we could be more future-looking, and propose:
> but is currently more likely to suffer from middlebox interference.
> ===
> However cycling is still a possibility at the Data Sender because a 
> whole sequence of ACKs carrying intervening values of the field might 
> all be lost or delayed in transit.
> - This is about reception of ACKs? I wasn't sure first read, could we 
> write:
> However ACKs received at the Data Sender could still cycle because a 
> whole sequence of ACKs carrying intervening values of the field might 
> all be lost or delayed in transit.
> ===
> had been an infeasibly long sequence of ACK losses.
> - what does infeasible mean here, it seems very imprecise. It would 
> have been easier not to judge the feasibility and simply said /a long/...

[BB] 'long' is less precise than 'infeasibly long', which conveys the 
meaning of 'not just long, but so long that we do not need to consider 
the possibility of it occurring in practice'.
The main meaning of Infeasible is impracticable, so I would like to 
leave this as-is.

> ===
> Therefore, as long as the AccECN Option is available
> - unfortunate use of /as long/ just after discussing length of ACK drops?
> Providing that the AccECN Option is available
> ===
> is provided in order to protect
> - ordering is not implied, this could be:
> is provided to protect

[BB] The intended meaning was not "provided in order, to protect." 
Rather it was "provided in_order_to protect." So your suggestion does 
indeed remove the ambiguity.

> ===
> The TCP server MUST NOT set one of these 4 combination of flags on the 
> SYN/ACK unless the preceding SYN requested support for AccECN as above.
> - What is the client response if this does happen?

[BB] To rephrase your question into a stand-alone question, "What does a 
non-AccECN client do if it somehow receives a SYN-ACK that implies the 
server supports AccECN?

This draft doesn't specify what non-AccECN clients do, and it would be 
pointless to try, because they are already out there, and their specs 
already say what they do.
Nonetheless, FYI, clients implementing either of the two non-AccECN 
specs currently out there would both handle a randomly appearing AccECN 
SYN-ACK correctly. Specifically:

1) RFC3168, §6.1.1:

         If a host has received at least one [...]
         non-ECN-setup SYN-ACK packet, then it SHOULD NOT set ECT on
         data packets.
         ...
         Receivers MUST correctly handle all forms of the non-ECN-setup
         [...] SYN-ACK packets.


Even if an RFC3168 client only checked the last 2 flags of the 3 AccECN 
flags, none of the three 2-bit combinations that imply AccECN support 
would be expected by an RFC3168 client (by design of AccECN).

Therefore:

  * An RFC3168 client would handle a randomly appearing AccECN SYN-ACK
    correctly,
      o whether it checked all the TCP flags (as required by the second
        sentence quoted from RFC3168 above)
      o or even if it only checked CWR and ECE.


2) RFC9293:
Altho this recommends TCP implementations support RFC3168, there are 
many RFC793 TCP implementations that do not. Such a non-ECN TCP client 
will have no logic to check the ECN flags during the 3WHS and therefore 
it will not notice that a SYN-ACK is (incorrectly) responding as if it 
was asked to support AccECN.

Therefore, again,

  * a non-ECN TCP client will handle a randomly appearing AccECN SYN-ACK
    correctly, just continuing to not support any form of ECN.



> ==
> interference than other intermittent causes of loss, e.g. congestion, 
> wireless interference, etc.¶
> - two uses of the word /interference/ with different meaning, could 
> the second be replaced by /loss/?

[BB] Good point. I think "transmission loss" would be better.

> ==
> It MUST NOT send an ECN-setup SYN [RFC3168] within the same connection 
> as it has sent a SYN requesting AccECN feedback.
> /as/when/ or something else?
> It MUST NOT send an ECN-setup SYN [RFC3168] within the same connection 
> when it has sent a SYN requesting AccECN feedback.

[BB] I don't see the problem here. "The same ... as" is correct English, 
whereas "the same ... when" is not.
A similar sentence proves this is a correct construction: "It MUST NOT 
send a message within the same millisecond as it has sent another message"

If this makes the reader stumble, we could rephrase entirely:
     It MUST NOT send an ECN-setup SYN [RFC3168] during a connection in 
which it has also sent a SYN requesting AccECN feedback.
However, I don't think that's any better, so I'd prefer to leave it as-is.

> ===
> It MUST NOT send an ECN-setup SYN/ACK [RFC3168] within the same 
> connection as it has sent a SYN/ACK agreeing to use AccECN feedback.
> /as/when/ or something else?
> It MUST NOT send an ECN-setup SYN/ACK [RFC3168] within the same 
> connection when it has sent a SYN/ACK agreeing to use AccECN feedback.

[BB] Ditto.

> ===
> It is still obliged to respond appropriately to AccECN feedback that 
> indicates there were ECN marks on packets it had previously sent
> -Please explicitly say what is "it"?
> The Data Sender is still obliged to respond appropriately to AccECN 
> feedback that indicates there were ECN marks on packets it had 
> previously sent
> ==
> In general, it is obliged to respond to congestion feedback even when
> - what is it? Receiver/Sender?
> In general, the Data Sender is obliged to respond to congestion 
> feedback even when
> ===
> Unlike an RFC 3168 data sender, it MUST NOT set CWR to indicate it has 
> received and responded
> - what is it? Receiver/Sender?
> Unlike an RFC 3168 data sender, a Data Receiver MUST NOT set CWR to 
> indicate it has received and responded

[BB] The problem you've identified occurs because there is an aside 
about all the 'switching feedback negotiation' bullets, just before all 
the 'congestion response' bullets.
This tends to lose the thread of the Data-Sender context.

Nonetheless, I've also noticed that:
* some of the bullets under "As a Data Sender" are about sending control 
packets, not data.
* some of the bullets under "As a Data Receiver" are about receiving 
control packets, not data.

So I shall have to restructure the whole section.
Let me do that, then give it to you to review again.

==
> if it receives an ECN-setup SYN or ECN-setup SYN/ACK [RFC3168] during 
> the same
> - to avoid doubt in citing this RFC2119 requirement, can we write:
> if a Data Receiver receives an ECN-setup SYN or ECN-setup SYN/ACK 
> [RFC3168] during the same
> ===
> If for any reason it is not willing to
> - to avoid doubt in citing this RFC2119 requirement, can we write:
> If for any reason the Data Receiver is not willing to
> ===
> it MUST NOT use reception
>
> - to avoid doubt in citing this RFC2119 requirement, can we write:
>
> A Data Receiver MUST NOT use reception

[BB] As above, I'll restructure the whole of this section before giving 
to you to review again.
Sorry about this.


> ===
> correct ECN feedback logic, as the packets
> - could we replace /as/?
> correct ECN feedback logic, because packets
> ===
> Whenever a host feeds back the value
> - Is that the Data Receiver? Sender? or both? - it would better to 
> match the clauses above.

[BB] Yes, this could s/host/Data Receiver/
But I will also make it clear that the stuff about the feedback being on 
a retransmission is about a retransmission for the other half connection.

> ===
> When a host first enters AccECN mode, in its role as a Data Receiver
> - could this be more simply said:
> When Data Receiver first enters AccECN mode,

[BB] Not really, because a host enters AccECN mode as a whole (not 
separately per half connection).

> ==
> When a host enters AccECN mode, in its role as a Data Sender
> - could this be more simply:
> When Data Sender first enters AccECN mode,

[BB] Ditto.

> ==
> A host MUST NOT interpret the 3
> - Is that the Data Receiver? Sender? or both? Client? Server?
> A Data Receiver MUST NOT interpret the 3

[BB] This does really mean "a host".
It applies across the 3WHS and data transfer, so it's not just about 
Data receiver or Data sender. And it applies to client and server. So it 
is a general statement about what hosts interpret.

> ==
> An implementer MAY develop an alternative algorithm as long as it 
> satisfies these requirements.¶
> - doesn't seem like a /MAY/ can this be /may/ or /is permitted to/?

[BB] I've checked the definition of 'MAY' in RFC2119, and I'm pretty 
sure we really do mean 'MAY', but I'm always willing to be corrected.
Can you try to explain why your understanding of 'MAY' would be 
inappropriate here?

I'm guessing that perhaps "an implementation MAY do x", but "an 
implementer may do y"?
If that's your concern, I would say in this case, "an implementer may 
produce code that MAY do y," so I don't see a great difference.

> ==
> the Data Sender SHOULD believe the ACE counter, unless it can be sure 
> that it is counting all control packets correctly.¶
> - what does believe mean? use?

     '...SHOULD consider the ACE counter to be correct (and its count of 
control packets to be incomplete)'

> ===
> Section 3.2.3. The AccECN Option
> - Could we please add RFC-Ed instructions to ensure replacement of 
> TBD1 and TBD2 during final edit?

[BB] In my local editor's copy, we already have the early assignment 
numbers now.

> ===
> All implementations of a Data Sender that read any AccECN Option MUST 
> be able to read in AccECN Options of any of the above lengths.
> - I think /in/ seems odd.
> - This makes me wonder what happens if they do not?
> - REQUIRED to implement?

[BB] I can s/read in/read/
For any spec (not only AccECN), if an implementation does not implement 
a 'MUST', it is not an implementation of the spec. So all bets would be off.

All this is clarifying is that, altho' it is optional to implement the 
AccECN Option, if you do implement it, you have to be able to read all 
the valid option lengths.

> ==
> Whenever the Data Sender receives ACK carrying
> - add /an/
> Whenever the Data Sender receives an ACK carrying
> ===
> If the ACK has not been superseded,
> - please explain superseded!

[BB] I'll add '(see Appendix A.1)', which defines superseded precisely.

> ===
> AccECN is for congestion control, which SHOULD generally be considered 
> important relative to other TCP options.
> - what does important mean here?
> - Does this have to be a /SHOULD/ could it be a should and explain?

I'll reword as
     "which implementations SHOULD generally prioritize over other TCP 
options when there is insufficient space for all the options in use."
Then I think 'SHOULD' becomes appropriate.

> ===
> it (the server) has to have confirmed
> - why not just say the server?
> the server has to have confirmed

[BB] In a SYN cookie scenario, the server deliberately forgets what it 
would normally remember. So I had to use 'it', because the server is 
reverse engineering what 'it' did earlier, where 'it' is another 
incarnation of itself that it doesn't remember. Otherwise, just saying 
"it can assume that the server has to have confirmed..." makes it sound 
like some other server.

A little rephrasing is needed here anyway, so I've tried to say this 
another way:

ORIGINAL:

If a TCP server using SYN Cookies supports AccECN and if ..., and if ...,
it can assume that:
*  the TCP client has to have requested AccECN support on the SYN
*  it (the server) has to have confirmed that it supported AccECN

PROPOSED:

If a TCP server using SYN Cookies supports AccECN and if ..., and if ...,
the server can infer the first two stages of the handshake:
*  the TCP client has to have requested AccECN support on the SYN
*  then, even though the server kept no state, it has to have confirmed
    that it supported AccECN


> P===
> Each host needs to maintain eight additional counters. The hosts have 
> to apply some additional tests to detect tampering by middleboxes,
> - Is this sender or receiver code?

[BB] Here, 'hosts' is appropriate because this is a general statement 
about all the additional tests; some are during the 3WHS (when either 
client or server is comparing what it received against what it sent); 
while other tests are conducted by the Data Receiver part of a 
half-connection.

> ===
> I would suggest section 3.3 is made a separate top level section 
> (section 4), to really call-out that this is not the TCP sender and 
> receiver but the part that other people need to read.

[BB] That would have been a really good idea earlier. I'd like to do 
this. But I'm going to think long and hard before doing it now though. 
I'm concerned that there might be cross refs within this or other 
documents to §3.3 or §4 -- §8 that might break.

Whether we do this or not, an additional piece in the doc roadmap about 
this section would be useful.

> ---
> With the importance of section 3.3 in mind, I was surprised to see the 
> top-level section had no text. Surely some text belongs here to say 
> that deployment of AccECN places new requirements on networking 
> equipment that forwards TCP, and inspects the TCP header information!

[BB] I believe that section headings are powerful ways to say exactly 
this, without having to repeat the same words with a few gratuitous 
little words added to make sentences. Specifically:

4. AccECN Compliance Requirements for TCP Proxies, Offload Engines and 
other Middleboxes
  4.1. Requirements for TCP Proxies
  ...
  4.2. Requirements for Transparent Middleboxes and TCP Normalizers
  ...
  4.3. Requirements for TCP ACK Filtering
  ...
  4.4. Requirements for TCP Segmentation Offload
  ...

Again, if this idea had come earlier, I might have been more inclined to 
add a sentence or two. But I'm not a fan of adding significant amounts 
of text at this stage. Will probably add a little tho.

> ===
> End of 3.3.2:
> However, to comply with
> - I could live without the "However", which to me at least gives more 
> weight - could even be a separate para to help this stand out!

[BB] I can see the need to emphasize, but a new para wouldn't work - the 
'However' sentence is very much a continuation of the same flow of thought.

> ===
> I think 3.3.4 is different to 3.3.1 and 3.3.2, and really belongs with 
> the rest or directly following 3.2. The reason is this, is about 
> implementing TCP.

[BB]
This is a question of whether separate processors are independent even 
if they run off the same power supply, or they're in the same housing. 
Similar questions apply on where you draw the line between an API and a 
protocol.

In a similar vein, TCP Proxies /are/ TCP implementations. So they ought 
not to be outside §3.
So it seems we're now tending towards the idea that §3.3.1 and § 3.3.4 
ought to remain in §3.3, while §3.2 & §3.3 would be moved to a new §4.

But then we return to your original point that the types of company that 
build all the functions of all four sections (§3.3.1-§3.3.4) tend to be 
"networking equipment vendors" rather than end-system developers. So we 
want to direct their eyes to their own section. But then NFV blurs the 
distinction. This is not clear-cut. As I said, I will think on it.

> ===
> Section 4
> For the avoidance of doubt it would have been nice to have one 
> starting sentence that explains if these statements in section 4 are 
> *additional* requirements to section 3, which I think they are likely 
> not, or a list of the impact of the changes in section 3 on RFC3168?

[BB] Good point. You're right. They are not additional, which needs saying.

> ===
> I don't have a strong opinion, but I think section 6 is much easier to 
> read than some of the other materials, and seeing this after section 2 
> mighty have been good. Equally could it be moved to join other 
> material before annexe B?

[BB] It is intended to be like a 'Conclusions' section. The idea is that 
it comes after the detailed spec, to sum up whether it meets the 
original requirements. On that basis, it can't really come before the 
detailed spec. I'd like to leave it where it is. But would it help to 
change the title to 'Summary: Protocol Properties'?

> ===
> Section 10 looks like it should have an RFC-Ed comment to request 
> removal before publication.
> ===
> Of course, other ways could be resorted to in order to extend AccECN 
> or ECN in future
> - could we be simpler and just say:
> Other ways could have been used to extend ECN

[BB] Certainly, we can remove 'Of course'.

But you're suggesting we change the sense from 'could be in future' to 
'could have been in this spec'. That would be contrary to the purpose 
(and headings) of this appendix. So we'd rather not do that part of your 
suggestion.

> ===
> In section 2:
> could give conflicting feedback on the same segment
> - which segment - the received segment or the segment that carries the 
> feedback?

[BB] We'll s/on/about/ which is what the conflict concern is.


> ===
> In section 2.4:
> If the option is stripped,
> - Maybe worth explaining in parenthesis
> If a middlebox strips the option (removes the option from a TCP segment),
> ===
> In section 2.4:
> Feedback in bytes is provided in order to protect
> - seems one to confusion, can we remove /in order/ to avoid potential 
> misunderstanding of ordering
> Feedback in bytes provides protection against a receiver using attacks 
> similar to 'ACK-Division' to artificially inflate the congestion 
> window, which is why [RFC5681] now recommends that TCP counts 
> acknowledged bytes not packets.
> ===
> Can the last para of section 2.4, be modified like this to allow it to 
> appear in the guidance on middle boxes? :
> Feedback in bytes protects from middlebox attacks similar to 
> 'ACK-Division' that seek to artificially inflate the congestion 
> window, which is why [RFC5681] now recommends that TCP counts 
> acknowledged bytes not packets.

[BB] A sentence directed at bad actors, boasting that we've designed 
this so they cannot build middleboxes that mount this particular attack 
wouldn't fit in the middlebox section.
The section on middleboxes is guidance for /willing/ middlebox 
implementers.

The original sentence justifies a design choice of the AccECN protocol. 
So it would be appropriate to just
     s/against a receiver/against a receiver or middlebox/


> ===
> If it has set itself into classic ECN feedback mode it MUST then 
> comply with [RFC3168].
> - Can we replace the /it/s:
> If a Data Receiver has set itself into classic ECN feedback mode it 
> MUST then comply with [RFC3168].

[BB] Yes, except s/Data Receiver/TCP client/, as per the preceding 
sentence in the draft.

> =================
> Terms:
>
> /ECN bits/ is mentioned 3 times, but should always be /ECN Field/

Agree on occurrences 1 & 2.

Occurrence 3 is about the TCP flags - that needs
     s/using the ECN bits in the TCP header/using three flags in the TCP 
header already assigned to earlier ECN schemes/

>
> /ECN Field/ is not defined, should be as per [RFC3168] and define the 
> ECN codepoint.

[BB] OK. This just needs a change to the first sentence in §1.4 Recap of 
Existing ECN Feedback in IP/TCP.

OLD:
     ECN [RFC3168 <https://www.rfc-editor.org/info/rfc3168>] uses two 
bits in the IP header.
PROPOSED:
[RFC3168 <https://www.rfc-editor.org/info/rfc3168>] defines the two-bit 
ECN Field in the IP header, consisting of four codepoints.

Table 1. is titled "The ECN Field in the IP header" and recaps the 
definitions of the ECN codepoints, and the para before it describes the 
semantics.

>
> endpoint and end-point are variously used, please be consistent.
>
> /Data Sender/ is defined and then /ECN sender/ used in the text. Do we 
> need to define both terms? or are they the same?

[BB] For the one occurrence, we'll
     s/an ECN sender/a sender that supports ECN/
and
     s/in the IP header/in the IP header of a data packet/

>
> /TCP client/ and /TCP server/ use lower case, whereas Data Receiver 
> and Data Sender use upper case. the text later about a /TCP sender/ 
> lower case. These ought to be consistent?

[BB] OK. I'll upper case TCP client and server.

Of the 5 occurrences of 'TCP Sender', the middle 3 are within quotes 
from RFC3168 (where it is defined) and the 1st & 5th ought to be in 
quotes and upper-cased, because they are describing what RFC3168 says.

That's it.
As I said, I'll just do everything with no comment.

Thank you v much again.

Cheers



Bob


>
>
> _______________________________________________
> tcpm mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/tcpm

-- 
________________________________________________________________
Bob Briscoehttp://bobbriscoe.net/