[multipathtcp] 6824bis -11 review

[Rahul Arvind Jadhav <[e-mail]>]

Hi,

As promised  in IETF102, here is my review for 6824bis-11. As Phil had suggested, I didn't take the trouble to check if my comments overlap with any of the previous discussions/comments. 

1. Section 3.6: Subflow Reset
[RJ]: Is MP_TCPRST a SHOULD or MUST to be included along with the TCP_RST ? Can this be stated explicitly in section 3.6?

2. Section 3.4.2. Remove Address
   "The sending and receipt (if no keepalive response was received) of
   this message SHOULD trigger the sending of RSTs by both hosts on the
   affected subflow(s) "
[RJ]: Since a MP_TCPRST should be associated with this RST, what should be the corresponding Reason Code that can be used in MP_TCPRST for this case? I checked existing reason codes in section 3.6 and none of those made sense (may be except for 0x0 ?).

3. Section 3.4.1. Address Advertisement 
   "A host can 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, and the Port MUST be different from one already in use
   for this Address ID."
[RJ]: Didn't understand the rationale for allowing reuse of Address ID only for port change and not for IP change.

4. Section 3.4.1. Address Advertisement
[RJ]: For long-lived MPTCP connection, the ADD_ADDR needs to be resent if the sub-flow is closed and if it needs to be reopened in the future. Is this correct? The reason for me to believe this is, if REMOVE_ADDR is sent and if the sub-flow is active/functioning, then the REMOVE_ADDR will be ignored. This means that when the sub-flow is closed, the corresponding Address ID is removed. Thus it means to establish a new sub-flow from the peer an ADD_ADDR has to be sent.

5. passive opener
[RJ]: The term "passive opener" is used several times in the document. This term needs to be clarified in Terminology section.

6. Section 3 Figure 3
[RJ]: Length field in figure 3 encompasses Kind, Length and Option payload. This should be explicitly stated.

7. Section 3.1 says: "it is expected that A will have generated a key locally before the initial SYN" 
[RJ]: Is there any advantage of generating the key in advance? The key generation can be avoided by A if node B responds without MP_CAPABLE. Can this be left to the implementation?

8. Section 3.2 says: The MP_JOIN option includes an "Address ID".  This is an identifier that only has significance within a single connection,
[RJ]: The significance of "Address ID" is only within the connection and __only on the peer__ who generates the ID. Is this correct? 

9. Section 3.3 says: M = Data Sequence Number (DSN), Subflow Sequence Number (SSN), Data-Level Length, and Checksum present
[RJ]: and Checksum present (if negotiated)

10. [RJ]: Can an attacker spoof MP_FAIL option to the receiver with DSN=0 (infinite mapping)? This will cause the receiver to discard all the data following DSN and the failed data won't be DATA_ACKed. Is there any way to protect this?

11. [RJ]: I was hoping to find a section in the introduction which summarizes changes in v1. This could be of help for someone who aims to migrate/understand briefly the implications of switching to v1. 

------------Editorials:
Section 2.7:
Currently: "To meet the threats identified in ..."
Should be: "To address the threats identified in ..."

Section 2.7:
Currently: "keys are sent in the clear in the MP_CAPABLE messages"
Should be: "keys are sent in clear in the MP_CAPABLE messages"

Section 3.1:
Spellfix: requsted -> requested
Spellfix: wishs -> wishes

Section 3.1:
Currently: "The key MUST be hard to guess, and it MUST be unique for the sending host at any one time."
Should be: "The key MUST be hard to guess, and it MUST be unique for the sending host across all MPTCP connections."

Section 3.1:
Currently: "there is not a collision"
Should be: "there is no collision"

Section 3.1:
Currently: "The receiver will acknowledge this data a the connection"
Should be: "The receiver will acknowledge this data at the connection"

Section 3.4.1:
Spellfix: assinged -> assigned

Section 3.6:
Currently: "Regular TCP RST options remain used to at the subflow-level"
Should be: "Regular TCP RST options continue to be used at the subflow-level"

Section 3.6:
Spellfix: re-estbalish -> re-establish
Spellfix: ressources -> resources

Section 3.6:
Currently: "It would be beneficial for a host to the reasons ..."
Should be: "It would be beneficial for a host to provide the reasons ..."