Re: [tsvwg] WGLC for DPLPMTUD

[Maksim Proshin <[e-mail]>]

Hi,

I’ve reviewed this draft and below you can find my comments. I think the document is in a good shape and I have only a couple of comments to discuss while the rest are minor or suggestions.
I can also add that we support PLPMTUD in our SCTP implementation but it’s not exactly the same as in this draft. There are some minor deviations but I don’t consider them severe.

Major comments:

1.

RFC 4821 states that “an implementation SHOULD implement some form of MTU verification” and I don’t see how that aspect is addressed in DPLMTUD. Did I miss it or was it intentionally skipped?

2.

Sec 5.1.1: “This value MUST NOT be smaller than 1 second, and SHOULD be larger than 15 seconds.” and “PROBE_TIMER can be derived from the PL RTT estimate.” – I assume it means that if the RTT estimate is used, it still must not be less than 1 second. Is it true? If so, I think it would be good to understand where 1 second is taken from. Is it due to RTO.Min recommendations? If so, can it be better to say that the value must not be less than RTO.Min of that particular PL?


3.

Some PLs like SCTP can’t re-fragment data during retransmission which might lead to a problem if PMTU was decreased in run-time or the PLPMTU value was discovered wrongly. In some case the data would never be retransmitted. This aspect is not covered in the draft. I think it would be good at least to recommend to not set DF-bit for retransmitted data and to not fragment user data to pieces larger than BASE_PMTU (or even MIN_PMTU?).



4.

Sec 4.5.1: “PTB messages that have been validated MAY be utilized by the DPLPMTUD algorithm, but MUST NOT be used directly to set the PLPMTU.” – what is a purpose if this MUST NOT here? What is a risk to apply it after the validation? If it’s not applied immediately, it might cause data loss. Or did you mean something different here?

5.

Sec 3: “A method is REQUIRED to signal an appropriate MPS to the higher layer using the PL. ” – In general I don’t see how the draft addresses it. Is it up to the PL implementation or the PL specification? I assume it requires SCTP Socket API update. If so, is it out of scope of this doc?

Minor comments:

6.
Im: The default value is specified for some parameters but not for all and without clear explanation of why such values are used as default. The same is for recommended values. Shouldn’t it be consistent?
7.
Im: Sec 1.2: “This has become the recommended approach for implementing PMTU discovery.” – recommended by IETF? can it be referenced/clarified?
8.
Mi: Sec 1.2: “see Section 4.5<https://tools.ietf.org/html/draft-ietf-tsvwg-datagram-plpmtud-12#section-4.5>” - my understanding was that the section is reference here because it provides more details about “all PTB processing can be disabled and PLPMTUD can completely replace Classical PMTUD” but I failed to find it in that section. Either the reference is wrong or I misunderstood the purpose of having it there.
9.
Mi: Sec 1.3. why don’t you refer to SCTP specification RFC4960 instead of or additionally to RFC4821. Section 7.3 of RFC4960 has a statement “An endpoint SHOULD apply these techniques, and SHOULD do so on a per-destination-address basis.” so for me it’s more relelvant to refer to RFC4960.
10.
Im/Q: Sec 3: “A DPLPMTUD sender is RECOMMENDED to provide information about the maximum size of packet” – provide to whom? Propose to clarify or rephrase it.
11.
Im/Q: Sec 3: “The mechanism needs to be robust to the possibility that packets could be significantly delayed along a network path.” - Isn’t it MUST?
The local PL endpoint => The destination PL endpoint?
12.
Im: Sec 3, Req 6: I think it would be also good to mention that if data is used in a probe the sender must be able to retransmit it in a smaller packet(s) because otherwise it would be lost again and again.
13.
Im: Sec 3, Req 7: Can it be improved saying MUST NOT when data is not used in a probe? And SHOULD when “isolated” data probe is lost.
14.
Mi: Sec 4.2: “These PLs need to either rely on an application protocol to detect this loss.” – “either” should be removed.
15.
Mi: Sec 4.3: “There are two alternative mechanism:” –> “mechanisms”
16.
Mi: Something wrong with references to RFCs when 2 or more of them are used. E.g. [RFC1191<https://tools.ietf.org/html/rfc1191>][RFC8201]
17.
Im/Q: Sec 4.4: “MUST suspend” – I assume it’s valid in general even when PL is using the other PL like QUIC uses UDP. If so, it would be good to clarify.
18.
Mi/Q: Sec 4.5.2: “larger than minimum size accepted” – I assume “larger than or equal to”. If so, all further formulas should also be corrected to consider it.
19.
Im: Sec 5.1.2: “expected to work for most paths” – it’s better to say “all paths” due to “expected”.

20.

Im/Q: Sec 5.1.3: “PROBED_SIZE” – I wonder why it’s not called “PROBE_SIZE”.

21.

Im: Sec 5.1.4: I assume that Base phase confirms the connectivity for BASE_PMTU and not in general as the connectivity can be achieved by a smaller PLPMTU value.
22.

Im/Q: Sec 5.1.4: Is it assumed that Base is the first phase? If so, it would be good to clearly state it.

23.

Mi: Sec 5.2: “Note: Not all changes are not shown to simplify the diagram.” – the second “not” should be removed.

24.

Im: There are “may”, “should”, etc in a couple of places. They either should be capital or rephrased.

25.

Im: Sec 6.2 (maybe general for Sec 6): It would be good to recommend to enable fragmentation (do not set DF bit) for all packets except for probes or at least for retransmitted packets.

26.

Im/Q: Sec 6.2: Should or should not probe packets influence assoc and path error counters? I think it’s “SHOULD” when a probe is acknowledged and “SHOULD NOT” when it’s lost. In other words probes should only clear the error counters in my view.

27.

Im/Q: Sec 6.2.1.2: “The HEARTBEAT chunk carries a Heartbeat Information parameter which should include, besides the information suggested in [RFC4960], the probe size ...” – Why is it required? If the sender of HB is able to uniquely associate the HB with the HB ACK, it only needs to remember the current PROBED_SIZE value.

28.

Im: Sec 9: “A node supporting DPLPMTUD MUST therefore appropriately validate the payload of PTB messages” – in addition it must not be used to directly set PTB value as prescribed in section 4.5.1. I would add it.

29.

Im: Sec 9: A potential attack when PTB increases PLPMTU is not described here. It would be good to add it.


BR, Maxim

From: Wesley Eddy <[e-mail]<mailto:[e-mail]>>
Date: 5 December 2019 at 15:36:34 GMT
To: "[e-mail]<mailto:[e-mail]>" <[e-mail]<mailto:[e-mail]>>
Subject: [tsvwg] WGLC for DPLPMTUD
This email starts a Working Group Last Call for the Datagram Path MTU Discovery draft:

https://datatracker.ietf.org/doc/draft-ietf-tsvwg-datagram-plpmtud/

This is targeted for Proposed Standard.

Please in the next couple of weeks send any further inputs you have to the TSVWG list.