Re: [multipathtcp] 6824bis-11 review

[<[e-mail]>]

Thanks Alan and Xavier.
Are there any other open issues from the reviews?
Thanks
phil

From: multipathtcp [mailto:[e-mail]] On Behalf Of Alan Ford
Sent: 18 September 2018 18:04
To: Xavier de Foy <[e-mail]>
Cc: multipathtcp <[e-mail]>
Subject: Re: [multipathtcp] 6824bis-11 review

Hi Xavier,

Firstly I’d like to thank you very much for your review, and I apologise it has taken so long to get back to you. I have a few comments inline, but most of these changes seem strong enhancements to the document which can be easily incorporated. If I haven’t commented, then the change is incorporated as-is.

Best regards,
Alan


On 8 Aug 2018, at 20:05, Xavier de Foy <[e-mail]<mailto:[e-mail]>> wrote:

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

We have had other comments about this phrase being confusing. It was only used in four places and I have found ways to re-word all of them to get rid of this phrase entirely.


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

Have re-worded to "A connection is not closed unless there has been a DATA_FIN exchange, or an implementation-specific, connection-level timeout.”.

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

Have added a description of the use of the ‘B’ bit for this purpose, but if you’re going to the effort of exchanging MP_PRIO, I don’t see why you wouldn’t also do a clean FIN exchange and need to do a RST, therefore I don’t see a need for a MP_TCPRST reason code here.

"Another use of the MP_PRIO option is to set the 'B' flag on a subflow to cleanly retire its use before closing it and removing it with REMOVE_ADDR, for example to support make-before-break session continuity."


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

I think we would need a strong justification for the benefits of such a change. I don’t immediately see why you’d want a live but inactive path.


*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)?

"The Address ID MUST uniquely identify the address for the sender of the option (within the scope of the connection), but the mechanism for allocating such IDs is implementation specific."


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

"Note that an implementation MAY discard incoming address advertisements at will, for example, for avoiding updating mapping state, or because advertised addresses are of no use to it (for example, IPv6 addresses when it has IPv4 only). Therefore, a host MUST treat address advertisements as soft state, and it MAY choose to refresh advertisements periodically."

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

"In the unlikely event that the token is valid at the receiving host, subflow setup will continue, but the HMAC exchange must occur for authentication. This will fail, and will provide sufficient protection against two unconnected hosts accidentally setting up a new subflow upon the signal of a private address."


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

This relates to the recent discussions on whether an Address ID can be re-used. The more I think about this, the more confident I am that it should not be permitted - i.e., as we have it written now.

Therefore I think we can re-write this text as follows:

"A host that receives an ADD_ADDR but finds a connection set up to that IP address and port number is unsuccessful SHOULD NOT perform further connection attempts to this address/port combination for this connection. A sender that wants to trigger a new incoming connection attempt on a previously advertised address/port combination can therefore refresh ADD_ADDR information by sending the option again.

A host can therefore send an ADD_ADDR message with an already assigned Address ID, but the Address MUST be the same as previously assigned to this Address ID. A new ADD_ADDR may have the same, or different, port number. If the port number is different, the receiving host SHOULD try to set up a new subflow to this new address/port combination."

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

"If the path is found to still be alive, the receiving host SHOULD no longer use the specified address for future connections, but it is the responsibility of the host which sent the REMOVE_ADDR to shut down the subflow. The requesting host MAY also use MP_PRIO to request a path is no longer used, before removal."


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

I’ve reworded a chunk of Fallback to simplify implementation. I also agree that MP_FAIL should act bidirectionally - it would be much simpler for implementations this way.


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

What kind of ADD_ADDR error are you thinking of?


*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
[https://ssl.gstatic.com/ui/v1/icons/mail/images/cleardot.gif]

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

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