Re: [tcpm] Review of draft-bagnulo-tcpm-generalized-ecn (David Black)

[Mirja Kühlewind <[e-mail]>]

Sorry, yes to be clear: this feedback was all meant as individual. It’s the chairs decision to run the adoption call. It’s just my personal opinion that we could do this right now with the current version (or we can also wait for your next version if you anyway working on it right now).

Mirja


> Am 24.04.2017 um 21:41 schrieb Bob Briscoe <[e-mail]>:
> 
> Mirja,
> 
> Thx for the rapid and extensive re-review. 
> 
> I shall get on with making the edits outlined below and, unless I see objections to anything in this email, I'll submit an update when done.
> 
> 
> On 20/04/17 18:26, Mirja Kühlewind wrote:
>> Hi Bob, 
>> 
>> given it seems I spend most of my time these days to worry about different ECN-related RFCs, I've also reviewed this draft (actually only sections 1-4; I may review section 5 at a later point of time but I trust you that you have addressed my previous comments here). 
>> 
>> Here is my feedback as individual contributor: In general, I'm happy with the spec and the overall draft now. Thanks for the work! It reads very well now. I have a few minor technical comments and some editorial proposals and a proposal to restructure some smaller parts below.       
>> 
>> So te document is in really good shape and the technical specification is good. So I think that this version is definitely ready for adoption and would like to see an adoption call very soon! 
> [BB] Strongly agree :)
> 
> Seriously, I think you should make it clear whether you are saying this as AD or individual.
> 
>> 
>> Mirja 
>> 
>> ----------------- 
>> High-level, minor technical points: 
>> 
>> 1) The description of TFO in section 4.2 is not quite correct: 
>> "If a TFO initiator has cached that the server supported ECN in the 
>>    previous connection, it would be safe to set ECT on any data segments 
>>    it sends before a SYN-ACK returns from the responder (server).  Note 
>>    that there is no space in the SYN-ACK itself (whether classic or 
>>    AccECN feedback has been negotiated) to include feedback about any CE 
>>    on data packets.  Nonetheless, it is safe to set ECT on data packets 
>>    within the handshake because any CE-marking on these data segments 
>>    can be fed back by the responder on the first data segment it sends 
>>    after the SYN-ACK (or on an additional pure ACK if it has no more 
>>    data to send)." 
>> TFO allows the initiator to send data in the SYN but no additional data packets, and the responder to send the SYN/ACK as well as an IW full of data packets. So I think the problem here goes away. However, accECN actually does feedback information of CE marked data in the SYN/ACK because the SYN/ACK carries the AccECN option that holds this information. 
> 
> [BB] I'll remove the whole section about TFO, and instead include a brief sentence explaining why TFO is not relevant in the intro to S.4. Thanks.
> 
> I was thinking that there is nothing in TFO [RFC7413] to preclude the initiator sending data segments before receiving the SYN-ACK.
> However, you've made me think this through further, and I've realized the initiator would have to send data without the ACK flag set (or with the ACK flag faked but no valid ackno). Altho the TFO cookie would make this safe from a security perspective, it's not going to happen due to middleboxes.
> 
> 
>> 2) Regarding this question in section 3.2.3 
>> "DISCUSSION: An AccECN deployment or an implementation of RFC 3168 
>>       that feeds back CE on pure ACKs will be at a disadvantage compared 
>>       to an RFC 3168 implementation that does not.  To solve this, the 
>>       WG could decide to prohibit setting ECT on pure ACKs unless AccECN 
>>       has been negotiated." 
>> I've checked REF3168 and it does not say anything about handling of pure ACKs that are CE marked because it probably just assumes that that case will never happen. However, implementations may or may not provide feedback. I think I would support to only have pure ACKs ECT marked if AccECN is supported because of this. I also think that losing ACKs is probably less bad that losing any of the other (control) packets that are discussed in this document. So I think this restriction would be okay. 
> [BB] OK, it's good to have one opinion on this.
> We'd like to hear more opinions before changing this tho.
> 
> FYI, RFC3168 does vaguely hint that the receiver of Pure ACKs carrying ECT (or CE) probably ought not to reject them (but it doesn't say what to do with a CE mark). In S.6.1.4 it says:
>       For the current generation of TCP congestion control algorithms,
>       pure acknowledgement packets [...] MUST be sent with the not-ECT codepoint.
> 
> Saying "current generation" implies a future generation might change this, which ought to make an implementer wary of rejecting ECT on Pure ACKs.
> 
>> 
>> 3) This is a suggestion to improve the situation, however it probably needs further discussion: in sect 3.2.1.4 you say that one should still use ECT SYNs even if a connection attempt previously failed. Maybe we can use a smaller time-out in this case...? 
> [BB] I'm wary of jumping to the conclusion that a loss is not due to congestion just because it's becoming more likely it could be due to a black hole. E.g. if there's a black hole, the probability of losing an ECT SYN is 100%, but if the underlying loss level is 1%, the probability that 2 SYNs are lost is 0.01% as an alternative. 
> 
> So, when 2 ECT SYNs at different times have timed out, but non-ECT SYNs got through, I suppose it's possible to argue that the probability that the second loss was due to congestion has reduced. So the timeout could be reduced.
> 
> I'll add this, and flag it "FOR DISCUSSION".
> 
>> 
>> And now first a couple of comments on the structure: 
>> 
>> 1) I would move the network behavior in any case AFTER the endpoint behavior spec. Actually I would even propose to have it in a different section (not under 3. Specification) because it actually does not specify anything; it only comments on the current situation. 
> [BB] I did consider this when I removed the normative text from this section. But this this section is intended to do more than just comment. We want it to incorporate the "SHOULD" in ecn-experimentation as part of the specification for this experiment. And we figured this text is short enough to put first, to reduce the chance it gets buried and overlooked operators, without distracting host implementers too much.
> 
> So I would rather make the following change than move the section:
> s/So operators are encouraged to alter their firewall rules/
>  /So operators are RECOMMENDED to alter their firewall rules/
> 
> Rationale: When updates to a system for an experiment are all spread about in different docs, it's not just helpful but also necessary to call out all the parts in their different docs. Similarly, we have incorporated RFC5562 (ECT on SYN-ACK) and AccECN into this draft - both as normative references.
> 
>> 
>> 2) And then you would actually not need a separate subsection for the Endpoint behavior; you could just call section 3 "Specification of Endpoint Behavior" or even "Specification of Sender Behavior" and move the first two paragraphs of section 3.2 to the intro that explains that this mechanism/document only needs to talk about the sender behavior. 
> [BB] Depends on resolution of the above.
>> 
>> 3) I would include the text on TFO and IW10 directly into section 3.2.1.3 instead of just giving a reference to a later section. 
> [BB] This is a matter of style. I prefer to distinguish normative dependencies between 
> * dependencies that are essential, 
> * and then to split optional dependencies between:
>   - STD
>   - EXPT
> 
> The above structure forces implementers (and authors) to make dependencies modular, in case an experiment fails. E.g. we do not yet know whether an attack will be invented against TFO. 
> 
> For instance, look at all the mess that has had to be cleared up because everyone assumed the ECN nonce experiment would succeed.
> 
>> 
>> 4) The rationale in section 3.2.6.1 could also go somewhere into section 5 (to keep the spec part as short as possible). At least I think you don't need the whole text here. If you want to keep something you probably only need the second paragraph (and not an own subsection). 
> [BB] The scope of Section 5 is (currently) countering arguments in other RFCs (mostly RFC3168).
> ECT on RSTs wasn't mentioned in an RFC so far, which is I why I put this section here.
> 
> One solution would be to rename section 5 "Rationale". Then I would be happy to move 3.2.6.1 to section 5. I'll do that.
> 
>> 
>> 5) Maybe have the section on Retransmissions (3.2.7.) earlier before RST, FIN, and Window probe because it seems more "important". But actually I don't think the order matters that much. So this is a minor comment only. 
> [BB] Currently the order is chosen to match the order of the lists in the introduction so they can be summarized as "control packets and retransmissions". I'll leave it as it is.
>> 
>> 6) The paragraph on SCTP (section 4.1) could go into the intro. And if you then also integrate the TFP and IW10 stuff into section 3.2.1.3, section 4 would actually go away completely. 
> [BB] Now, with the removal of the TFO section, none of S.4 contains anything normative. Now it is just saying "For all these variants, here's why you don't need to do anything different". So I am inclined to move it /after/ section 5.
> 
> I certainly don't want to integrate the TFO and IW10 comments into the spec section, because that would just complicate the spec with text that essentially ends up saying "You didn't need to read this to implement the spec".
> 
> I also don't want to add a detailed para about SCTP to a nice 3-para Intro to a spec about a modification to TCP.
> However, I agree that "scope" is one of the important things to cover in an introduction.
> 
> In summary, I would rather move this section about variants to after S.5. And in the intro, say this is an experimental modification to standards track TCP, but also flag that it discusses (non-)interaction with variants such as SCTP, TFO, IW10. 
> 
>> 
>> And now a bunch of editorial comments/proposals: 
>> 
>> 1) I though that it might be good to mention AccECN already in the Intro because that the enabler for the SYN part... 
> [BB] OK, that would be sensible, along with the above sentence about variants. Will do.
>> 
>> 2) section 1.1: 
>> OLD 
>> "Preventing TCP control 
>>    packets from obtaining the benefits of ECN would not only expose them 
>>    to the prevailing level of congestion loss, but it would also stop 
>>    them from being classified into the low latency (L4S) queue, which 
>>    would greatly degrade L4S performance." 
>> I think this can be phrased more general: 
>> NEW 
>>  "Preventing TCP control 
>>    packets from obtaining the benefits of ECN would not only expose them 
>>    to the prevailing level of congestion loss, but it would also put control 
>>    packet into a different queue with different network treatment, which 
>>    may also lead to reordering and therefore can greatly degrade TCP 
>>    performance." 
> [BB] Good point. Given this sentence was in a para about L4S, I shall split it off and make it into a summary section.
>> 
>> 
>> 3) section 3.2.1.1: 
>> OLD 
>> "Accurate ECN (AccECN) 
>>    feedback [I-D.ietf-tcpm-accurate-ecn] provides two codepoints in the 
>>    SYN-ACK" 
>> NEW 
>> "Accurate ECN (AccECN) 
>>    feedback [I-D.ietf-tcpm-accurate-ecn] provides one flag in the 
>>    SYN-ACK" 
>> I think that's clearer, at least for me. 
> [BB] Eh? I'm not going to change this, cos the word flag would imply this is a bit with a meaning orthogonal to the other bits.
> 
> Here's a copy of the relevant part of the table...
>    |  NS CWR ECE  |                      |
>    |  0   1   0   | AccECN               |
>    |  1   1   0   | AccECN (CE on SYN)   |
>    |  1   0   1   | classic ECN          | 
> ... etc
> 
> 
> The first bit only distinguishes "whether or not the SYN arrived marked CE" when the 3-bit codepoint as a whole is 'X10'. So it's not a flag, it's 2 codepoints.
> 
>> 
>> 4) section 3.2.1.2 
>> This sentence is correct but I had to read it twice to make sure it's correct: 
>> "Servers that 
>>    do not support ECN as a whole probably do not need to be recorded 
>>    separately from non-support of AccECN because, even when a server 
>>    does not support classic [RFC3168] ECN, there is no performance 
>>    penalty in always requesting to use it." 
>> NEW 
>> "Servers that 
>>    do not support ECN as a whole probably do not need to be recorded 
>>    separately from non-support of AccECN. Meaning, if AccECN is noted as not supported by the server, the initiator may decide to not mark the SAN as ECT but it still may try to negotiate classic ECN or even AccECN (to quickly detect server upgrades)." 
> [BB] I realize now that this text is all wrong. I shall attempt a rewrite.
> 
> I now realize my heading of 3.2.1.2 is wrong; it was not meant to be about "Caching Failed Connection Attempts". It was meant to be mostly about "Caching Lack of Recognition of ECT SYNs", with just a reference forward to caching failed connection attempts ("3.2.1.4 Fall-Back Following No Response to an ECT SYN"). 
> 
> I intended to say that, when a server is cached as not supporting AccECN, subsequent attempts should still request AccECN, but not with ECT on the SYN. Because there is no harm in always attempting to negotiate AccECN (as long as such requests are not black-holed), because the responder will still respond properly to an AccECN request if it only supports classic ECN or no ECN at all, without any performance penalty. 
> 
> I shall also make the point in S.3.2.1.4 that if an ECT SYN is black-holed, it might still be possible to request AccECN, but without ECT on the SYN.
> 
>> 
>> 5) section 3.2.1.4: 
>> OLD 
>> "To expedite connection set-up if, after sending an ECT SYN, the 
>>    retransmission timer expires, the TCP initiator SHOULD send a SYN 
>>    with the not-ECT codepoint in the IP header and not attempt to 
>>    negotiate AccECN.  It would make sense to also remove any other 
>>    experimental fields or options on the SYN, but that will depend on 
>>    the specification of the other option(s). " 
>> I wouldn't make any statements about things that are not in scope of this doc, so I would remove the later part (including fallback for AccECN): 
>> NEW 
>> "To expedite connection set-up if, after sending an ECT SYN, the 
>>    retransmission timer expires, the TCP initiator SHOULD send a SYN 
>>    with the not-ECT codepoint in the IP header." 
> [BB] I agree that I shouldn't have said to not attempt AccECN. So I will use your new text.
> 
> But I disagree about not even mentioning that the cause might be other experimental options, and to encourage the reader to read those specs as well. I'll use the following unless you shout:
> 
> "To expedite connection set-up if, after sending an ECT SYN, the 
>    retransmission timer expires, the TCP initiator SHOULD send a SYN 
>    with the not-ECT codepoint in the IP header.  If other experimental 
>    fields or options were on the SYN, it will also be necessary to follow 
>    their specifications for fall-back too. It would make sense to co-
>    ordinate all the strategies for fall-back in order to isolate the specific 
>    cause of the problem."
> 
>> 
>> 6) section 3.2.2 says: 
>> "In either case no change is required to RFC 5562 or the AccECN specification." 
>> 
>> It would probably be good to also state in this draft what the procedure is that is described in RFC5562: 
>> "When the responder (node B) receives the ECN-Echo packet reporting 
>>    the Congestion Experienced indication in the SYN/ACK packet, the 
>>    responder sets the initial congestion window to one segment, instead 
>>    of two segments as allowed by [RFC2581], or three or four segments 
>>    allowed by [RFC3390].  As illustrated in Figure 2, if the responder 
>>    (node B) receives an ECN-Echo packet informing it of a Congestion 
>>    Experienced indication on its SYN/ACK packet, the responder sends a 
>>    SYN/ACK packet that is not ECN-Capable, in addition to setting the 
>>    initial window to one segment.  The responder does not advance the 
>>    send sequence number.  The responder also sets the retransmission 
>>    timer.  The responder follows RFC 2988 [RFC2988] in setting the RTO 
>>    (retransmission timeout)." 
> [BB] Two responses:
> 
> 1) I think it's dangerous to quote a small part of another spec. as if it's a good summary of the behaviour. This loses all the detail in RFC 5562 (e.g. what if the responder implements SYN cookies, yada yada).
> 
> 2) Oh dear. I had forgotten that RFC5562 specified a different protocol to the "ECN+" paper. It's ultra-ultra-conservative.
> 
> So, I think we had better include the original proposal in the "ECN+" paper within the generalized ECN draft, and include a counter-argument to RFC5562 as well as to RFC3168.
> 
>> 
>> 7) section 3.2.3: 
>> OLD 
>> "It MAY also 
>>    implement AckCC [RFC5690] to regulate the pure ACK rate, but this is 
>>    not required.  Note that, in comparison, [RFC5681] does not require..." 
>> NEW 
>> "It MAY also 
>>    implement Acknowledgement Congestion Control (AckCC) [RFC5690] to regulate the pure ACK rate, but this is 
>>    not required.  Note that, in comparison, TCP Congestion Control [RFC5681] does not require..." 
> [BB] OK.
>> 
>> 
>> 8) section 3.2.3: 
>> "The question of whether the receiver of pure ACKs is required to feed back any CE marks on them is a matter for the relevant feedback specification ([RFC3168] or [I-D.ietf-tcpm-accurate-ecn]).  It is outside the scope of the present specification." 
>> However, as mention about I believe that RFC3168 does not say anything about feedback how to handle CE marks of pure ACK because it assumes that this case never occurs. So that might be implementation dependent, so it might be better to spell that out: 
>> "[I-D.ietf-tcpm-accurate-ecn] includes handling of control packets. However, [RFC3168] does not specify handling of pure ACKs that are CE mark, so it might be implementation specific if       feedback is provided or not." 
> [BB] OK.
> 
> Anyway, this will be moot if we do not allow ECT on Pure ACKs unless AccECN has been negotiated.
>> 
>> Related to this comment in the same section: 
>> "DISCUSSION: Before the AccECN specification is published, a 
>>       requirement to count CE marks on all packets could be added." 
>> I pretty sure that this is what the AccECN draft is doing but I can double check. If not it clearly should be added. Maybe it's not spelled out clearly enough. I'll check! 
> 
> [BB] You're right. I will fix this.
> 
> I too thought that AccECN counted CE on pure ACKs, but I checked the AccECN draft by searching for "pure ACK" and couldn't find any mention of this aspect. However, I've just done a more thorough search, and you're right - it says so three times, but none use the words "pure ACK":
>    The fourth counts
>    the number of packets arriving marked with a CE codepoint (including
>    control packets without payload if they are CE-marked).
> 
> [...]
>    The CE
>    packet counter includes control packets that do not have payload
>    data,
> 
> [...]
>    The CE packet counter (r.cep), counts the number of packets
>    the host receives with the CE code point in the IP ECN field,
>    including CE marks on control packets without data.
> 
> 
>> 
>> 9) section 3.2.6.1: 
>> OLD 
>> "The following factors have been considered before deciding whether 
>>    ECT ought to be allowed on a RST message:" 
>> NEW 
>> "The following factors have been considered before deciding whether 
>>    the RST message should be ECT marked:" 
> [BB] 
> * I had already edited out any occurrence of the phrase "ECT marked" (because "marked" generally implies a congestion marking, so this could be misinterpreted). 
> * I always avoid the word "should" in lower case.
> * I try to avoid writing sentences in the passive (but I missed this one).
> 
> Other than all that, I grok what you mean; I will use:
> 
> "The following factors have been considered before deciding whether 
>    the sender ought to set ECT on a RST message:" 
>> 
>> That's it. So nothing big. Let move on with this draft! 
> 
> [BB] 
> Cheers
> 
> 
> Bob
>> 
>> 
>> 
>> 
>> On 18.04.2017 11:40, Bob Briscoe wrote: 
>>> David, Mirja, list, 
>>> 
>>> Thanks again for your reviews of the -00. I've been busy over the w/e after 
>>> thinking more deeply about some of your comments and after realizing that 
>>> RFC3168 contained an assumption that a single pure ACK implies that all other 
>>> packets in that direction are pure ACKs, which is not true in general. 
>>> 
>>> We've produced another rev (draft-bagnulo-tcpm-generalized-ecn-02 
>>> <https://tools.ietf.org/html/draft-bagnulo-tcpm-generalized-ecn-02>). 
>>> 
>>> This is a general invitation for anyone on the list, including you two, to 
>>> review this again, so we can ask for an informed adoption call. 
>>> 
>>> We need people with the following expertise: 
>>> * TCP hijacking and DoS/flooding attacks 
>>> * Specific TCP implementations (Linux, FreeBSD, iOS, Windows, etc). 
>>> * All the common variants of TCP 
>>> 
>>> We have identified clearly where decisions are needed. 
>>> As well as where more measurements are needed. 
>>> 
>>> Main changes: 
>>> * Moved more discursive text from S.3 (Spec) to S.5 (Arguments against RFCs) 
>>> * Major changes to Pure ACK sections and the reliability argument, to address 
>>> Mirja's arguments better, and to separately address cwnd response (required) 
>>> and ACK-rate response (optional but not required). 
>>> * Referred to Network Behaviour in ecn-experimentation, rather than repeating 
>>> it. 
>>> * Fixed description of window probe. 
>>> * Recommended RFC 5961, which gives much stronger packet validity checks for 
>>> retransmissions, RSTs & FINs. 
>>> 
>>> Cheers 
>>> 
>>> 
>>> Bob 
>>> 
>>> 
>>> On 14/04/17 02:13, Bob Briscoe wrote: 
>>>> David, 
>>>> 
>>>> I hope you will find that I have addressed all your concerns - so at least 
>>>> you will be able to understand it now. 
>>>> The draft is now unexpired. 
>>>> 
>>>> I have also addressed all the points in Mirja's review (some intersecting 
>>>> with your points). 
>>>> 
>>>> You will see that it's actually turned into quite a major re-write. The 
>>>> diff is nearly total, mainly cos I was turning Spanglish into English as I 
>>>> went (Marcelo, it was quite understandable, but just not quite how a native 
>>>> English speaker would say it). 
>>>> 
>>>> However, I have changed technical points in a few places too - the more one 
>>>> thinks about this, the more depth one finds. 
>>>> 
>>>> I just noticed I missed one of your nits (shortening the definition of 
>>>> Retransmission), which I have already fixed in my local copy of the xml for 
>>>> the next rev. 
>>>> 
>>>> 
>>>> 
>>>> Bob 
>>>> 
>>>> 
>>>> On 08/04/17 01:11, Bob Briscoe wrote: 
>>>>> David, 
>>>>> 
>>>>> On 2) I intend to replace the whole of "3.1 Network behaviour" with the 
>>>>> following, which calls out the cases explicitly: 
>>>>> 
>>>>> Previously RFC3168 required the sender to set not-ECT on certain TCP 
>>>>> control packets. Some readers might have erroneously interpreted this 
>>>>> aspect of RFC3168 as a requirement for firewalls, intrusion detection 
>>>>> systems, etc. to check and enforce this behaviour. Now that the present 
>>>>> experimental specification allows TCP senders to set ECT on all TCP 
>>>>> packets (control and data), it needs to be clear that a firewall (or any 
>>>>> network node) SHOULD NOT treat any ECT packet differently dependent on 
>>>>> what type of TCP packet it is. 
>>>>> 
>>>>> One potential exception is envisaged, otherwise the previous sentence 
>>>>> could have said "MUST NOT" rather than "SHOULD NOT". The exception allows 
>>>>> a security function that has detected an ongoing attack to drop more ECT 
>>>>> marked SYNs than not-ECT marked SYNs. Such a policy SHOULD NOT be applied 
>>>>> routinely. It SHOULD only be applied if an attack is detected, and then 
>>>>> only if it is determined that the ECT capability is intensifying the attack. 
>>>>> 
>>>>> 
>>>>> Bob 
>>>>> 
>>>>> On 07/04/17 23:33, Black, David wrote: 
>>>>>> Bob, 
>>>>>> 
>>>>>> Two comments: 
>>>>>> 
>>>>>> 1) I'll wait for new text before commenting further on Accurate ECN vs. 
>>>>>> vanilla RFC 3168 ECN.  The current text left me somewhat confused, and 
>>>>>> the summary of changes from RFC 3168 will almost certainly help. 
>>>>>> 
>>>>>> 2) On network node treatment of ECT, RFC 3168 is brief, e.g. (Section 5, 
>>>>>> top of p.7): 
>>>>>> 
>>>>>>     The CE codepoint '11' is set by a router to indicate congestion to 
>>>>>>     the end nodes. 
>>>>>> 
>>>>>> One wants to avoid text that could be read as modifying that sentence by 
>>>>>> adding possible exceptions for TCP control packets and/or retransmissions. 
>>>>>> 
>>>>>>> [BB] Unfortunately, RFC3168 does not specify anything about network node 
>>>>>>> forwarding behaviour wrt ECT. So some firewall policies could have 
>>>>>>> decided to block any TCP control packets that contravene RFC3168. That's 
>>>>>>> why we wrote this section. 
>>>>>> If "firewalls" is what was meant, use that word ;-) ... or as you write ... 
>>>>>> 
>>>>>>> However I admit that, by not explaining that DPI was the problem we were 
>>>>>>> trying to solve, rather than removing any ambiguity that might have been 
>>>>>>> interpreted as a need for DPI. We'll fix this. 
>>>>>> In doing so, please keep in mind that in this draft, discussion of 
>>>>>> forwarding  and dropping behavior is implicitly about router queue 
>>>>>> management, not middlebox access control (e.g., based on DPI), so access 
>>>>>> control has to be called out explicitly when that is what is meant. 
>>>>>> 
>>>>>> Thanks, --David 
>>>>>> 
>>>>> 
>>>>> 
>>>> 
>>> 
>>> -- 
>>> ________________________________________________________________ 
>>> Bob Briscoe                               http://bobbriscoe.net/ 
>>> 
> 
> -- 
> ________________________________________________________________
> Bob Briscoe                               
> http://bobbriscoe.net/