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

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

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!

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.

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.

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...?

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.

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.

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.

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).

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.

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.

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

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."


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.

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)."

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."

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)."

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..."


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."

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!

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:"

That's it. So nothing big. Let move on with this draft!




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