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

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