[multipathtcp] 6824bis-11 review

[Xavier de Foy <[e-mail]>]

 Hi, as promised at IETF 102, here is a review of the current working
version of draft-ietf-mptcp-rfc6824bis-11, which is in very good shape in
my opinion. Most of the comments are editorial in nature, or minor
clarifications: for those I tried to propose some rewording, but these are
for illustration only. There are a few higher level comments and questions
as well.

It is based on version 6091823... (Jul 30, 2018) of the live Github draft
https://github.com/multipath-tcp/draft-ietf-mptcp-rfc6824bis

I may not be able to participate in any email discussion for the next 2-3
weeks, but will answer back as soon as I can.

Best Regards,

Xavier.

---

*General

- The experimental extension mechanism was removed from the draft. What is
the current plan for dealing with experiments?


*1. Introduction

Minor text cleanup/reorganization:

(1) "This document is complemented by three others: [RFC6824]." could be
replaced with " This document is complemented by three others: " + (bullet
list could go here). And move "This document specifies MPTCP v1..." after
the bullet list.
(2) s/This document is an update to, and obsoletes, the v0 specification of
Multipath TCP/ This document is an update to, and obsoletes, the v0
specification of Multipath TCP ([RFC6824])./


*1.3 Terminology

s/similar/similarly/


*2. Operation Overview

- s/-- a single numerical type /-- using a single numerical type/


*2.1. Initiating an MPTCP Connection

- s/ is variable length / has a variable length /



*2.3.Informing the Other Host about Another Potential Address

- s/This option contains a HMAC/The ADD_ADDR option contains a HMAC/: (this
clarification is proposed because MP_JOIN is mentioned in the previous
sentence)


*2.4.

- In diagram (2.4 and 2.6): replace DATA_SEQUENCE_SIGNAL with DSS, which is
the name of the option

- s/The "Data Sequence Signal" carries/The Data Sequence Signal (DSS)
option carries/ (since this is the first place where DSS is mentioned in
the text)


*2.7. Notable Features

- This is the first mention of "passive opener". Even if it is a well-known
concept, it could help to illustrate what is meant in context, e.g. s/passive
opener/passive opener (i.e. host that can open a subflow or connection upon
reception of initial SYN packet)/.


*3.2. Starting a new subflow

- Here and in 3 other places in the doc, we find the words "with an
appropriate reason code.", while in some other places the exact codes are
spelled out. You may want to spell out the appropriate reason codes in
these 4 places.


*3.3. General MPTCP Operation

- " DATA_FIN exchange or a timeout ". Would it help to add a reference to
where the timeout case is described? (I guess it is in section 3.3.3)


*3.3.3. Closing a connection

- "It is also encouraged to reduce the timeouts (Maximum Segment Life) on
subflows at end hosts. ": would it make sense to precise: "... at end hosts
after receiving a DATA_FIN acknowledgement. "


*3.3.1. Data Sequence Mapping

- s/analogous/analogously/

- s/for that sequence space ignored/for that sequence space should be
ignored/

- s/ISN/Initial Sequence Number (ISN)/


*3.3.8. Subflow Policy

- It may be useful to document a clean removal of an active IP address
(this technique was suggested by Christoph to address a need for session
continuity support). A peer who wish to remove the IP address would start
with setting all subflows using this address to "Backup" with MP_PRIO
setting the "B" bit. Then the peer wait for in-flight segments to be
received and acked on both sides. After this, it can reset the subflow (a
new bullet with a description of this technique could be added in section
3.6.) A new subflow reset reason code may also be added in section 8.3. to
indicate that a peer took the decision to permanently remove a path, unless
an existing one like "administratively prohibited" is deemed sufficient.
(this technique is likely to be useful to support session continuity, to
stop using the old address in make-before-break cases)

- For future improvements of the "clean removal" above, I would suggest
considering using another flag for "inactive". "Inactive" would be similar
to backup, except that it would never become active, even if no other
active subflow exist. I understand it is late for such a change though,
which could have unforeseen consequences.


*3.4.1. Address Advertisement

- s/This is also used to identify MP_JOIN/The address ID is also used to
identify MP_JOIN/

- "The Address ID MUST uniquely identify ...": could we precise if the
address ID must be unique per peer, or across both peers (i.e. can a same
address ID be used for an address on host A and another address on host B)?

- "for avoiding the required mapping state": consider clarifying (do you
mean that the receiver wish to avoid maintaining a mapping state? It does
not look like a root cause, why would the receiver would like to avoid
this?)

- "In the unlikely event that the token is known": probably need to be
qualified better, because if the token is known, that would mean that the
MP_JOIN message is legitimate, right?

- "If these conditions are not met, the receiver SHOULD silently ignore the
ADD_ADDR.": in the next paragraph, it is said that a peer can re-trigger a
new subflow attempt by sending an ADD_ADDR (I assume with same address and
port), which seems to contradict this statement (" Port MUST be different "
is too strict, since same port is a possible case to "refresh" an ADD_ADDR).


*3.4.2. Remove Address

- In relation with my first comment for section 3.3.8: there can be a use
case to remove an IP address which is not yet invalid (e.g. to support
session continuity). It could be useful to widen the range of application
of REMOVE_ADDR: e.g. add a new sentence at the end of the first paragraph:
"A host MAY also choose to announce that a still valid IP address should
not be used any longer."

In this case, "it must ensure the affected path(s) are no longer in use
before it instigates closure. " could be rephrased as "it must ensure the
affected active path(s) are no longer in use before it instigates closure
(i.e. only active, not backup paths should be checked as follows). "


*3.5. Fast Close

- Would it make sense to clarify by adding "by sending a TCP RST" in:
s/Upon receipt of an ACK with MP_FASTCLOSE by Host B, containing the valid
key, Host B answers on the same subflow with a TCP RST and tears down all
subflows./Upon receipt of an ACK with MP_FASTCLOSE by Host B, containing
the valid key, Host B answers on the same subflow with a TCP RST and tears
down all subflows by sending a TCP RST./


*3.6. Subflow Reset

- " The "U", "V" and "W" flags are not defined by this specification and
are reserved for future use. ": Should we add the same text used for other
unused bits, stating these must be set to 0, and must be ignored by
receiver?

- In relation with my first comment for section 3.3.8: there could be a
code for a voluntary decision by a peer to close a path (e.g. for session
continuity purpose). For example, its name could be "Path Management
Decision" or similar.


*3.7. Fallback

- In general, in this section I found the flow a bit confusing, mostly at
the beginning. Maybe it evolved over time, and you may want to check how
the text is organized ("these rules...", "the case described above..." seem
to point to some specific cases but it is not obvious which one).
For example, about "The case described above is a specialized case of
fallback": the text above includes "subflow breaks during operation", which
does not fall into the category of fallbacks "...before any data is
acknowledged at the connection level on a subflow".

- Consider clarifying what is meant by "there is the possibility that false
positives could be hit across MPTCP segment boundaries, corrupting the data"

- "It is not mandatory for the receiver of an MP_FAIL to also respond with
an MP_FAIL" and "However, implementations MAY choose to send a MP_FAIL in
the reverse direction and entirely revert to a regular TCP session.": Does
an infinite mapping affect a subflow unidirectionally or bidirectionally?
This paragraph can lead to some confusion on this point. It may be useful
to precise this in section 3.3.1 (last paragraph). Assuming the effect of
infinite mapping is bidirectional, the words "and entirely revert to a
regular TCP session" are the confusing ones here (since we are reverting to
TCP entirely anyway). Probably: the remote peer should (MUST?) send a
MP_FAIL if some segments were received with a bad checksum, and otherwise
should not (MAY?) send it.


*3.8. Error Handling

- Is the list of errors complete? (e.g. ADD_ADDR errors seem to belong to
the same class as remove request)


*3.9.1. Port Usage

- s/Add Address/ADD_ADDRESS/


*3.9.2.

- s/and thus never use MPTCP./and thus would never use MPTCP based on this
heuristic alone./


*4. Semantic Issues

- It would be informative to document shortly the reason why SYN and
DATA_FIN both occupy 1 octet of data-level sequence space. E.g. "This is to
ensure that they are acknowledged at the connection level in data
acknowledgements."


*6. Interaction with middleboxes

- "and the handshake mechanism ensures that connection attempts to private
addresses [RFC1918] do not cause problems": consider clarifying this
sentence (which part of the handshake mechanism address which potential
problems?).

- s/However, for an MPTCP-aware IDS, tokens can be read by such
systems/However, a MPTCP-aware IDS can read tokens/
- s/reassemble for analysis/reassemble them for analysis/
- s/If a fraction of options are stripped/If a fraction of options is
stripped/ (or "if only some of the options are stripped")


* Appendix C

- s/MPTCP control block/MPTCP protocol control block (PCB)/
(This is to introduce the term MPTCP PCB, which is used in appendix D's
diagram)


* Appendix D

- Consider detailing the text a bit more on the relation of the diagram
with "interations with subflow level FINs", and some details on the states
involved in "break-before-make" handovers.


*Other Editorial comments

- s/subfow/subflow/
- s/not longer/no longer/
- s/so long as/as long as/
- s/associated to/associated with/
- s/Lenght/Length/

- A couple of change was needed to generate draft from the github version
(there was an extra </section> and a missing reference), here is the diff:

$ diff -c rfc6824bis.ori draft-rfc6824bis.xml
*** rfc6824bis.ori      2018-08-06 09:39:16.608250000 -0400
--- draft-rfc6824bis.xml        2018-08-06 09:45:09.155126800 -0400
***************
*** 281,287 ****
     ACK + MP_CAPABLE (+ data) ->
     [A's key, B's key, flags, (data-level details)]
  ]]></artwork></figure>
- </section>

  <t>Retransmission of the ACK + MP_CAPABLE can occur if it is not known if
it has been received. The following diagrams show all possible exchanges
for the initial subflow setup to ensure this reliability.</t>

--- 281,286 ----
***************
*** 2217,2222 ****
--- 2216,2222 ----
        &RFC6824;
        &RFC7413;
        &RFC7430;
+       &RFC8174;


  <!--      &TCPLO; draft-ananth-tcpm-tcpoptext-00; Expired-->