Re: [tsvwg] RFC2119 Keywords in: draft-ietf-tsvwg-rfc4960-bis-06.txt

[Michael Tuexen <[e-mail]>]

> On 7. Apr 2020, at 14:29, Gorry Fairhurst <[e-mail]> wrote:
> 
> Thanks for posting rev -06.
Thanks for the review. See my answers in-line.

Best regards
Michael
> 
> I have now read:
> https://tools.ietf.org/html/draft-ietf-tsvwg-rfc4960-bis-06
> 
> Overall, I found the document mature, but a spec of this size has lots of requirements and this review I went through these. I did not find many substantial issues, but I did find lots of places where the text was either unclear or the recommendation was not well-placed within the text.
> 
> Gorry
> 
> The following comments may help in preparation of the next revision:
> 
> (1) There are currently keywords used before the RFC2119 bolierplate. Perhaps the boilerplaet should be moved earlier?
Moved before the introduction.
> 
> (2)
> "The port number 0 MUST NOT be used." - this appears twice, but would be simpler if one inserted "destination" and one "source".
Added.
> 
> (3)
> "The 'Stream Sequence Number' field in a DATA chunk with U flag set to 1 has no significance. The sender can fill it with arbitrary value, but the receiver MUST ignore the field."
> - can we change "it" to the 'Stream Sequence Number' field or something more specific?
Done.
> 
> (4)
> This sentence seems slightly mangled:
> "The endpoint MUST also treat all the DATA chunks with TSNs not included in the Gap Ack Blocks reported by the SACK as "missing".
> - Maybe this could say something like:
> "All DATA chunks with TSNs not included in the Gap Ack Blocks reported by a SACK MUST be treated as "missing" by the sending endpoint."
Done.
> 
> (5)
> "The number of "missing" reports for each outstanding DATA chunk MUST be recorded by the data sender in order to make retransmission decisions."
> - is "in order" denoting a time-order, or can this be omitted to avoid potential confusion?
Omitted.
> 
> (6)
> This seems very long:
> "When a single SACK cannot cover all the Gap Ack Blocks needed to be reported due to the MTU limitation, the endpoint MUST send only one SACK, reporting the Gap Ack Blocks from the lowest to highest TSNs, within the size limit set by the MTU, and leave the remaining highest TSN numbers unacknowledged."
> Is this a possible alternative:
> "When a single SACK cannot cover all the Gap Ack Blocks needed to be reported due tothe MTU limitation, the endpoint MUST send only one SACK. This single SACK MUST report the Gap Ack Blocks from the lowest to highest TSNs, within the size limit set by the MTU, and leave the remaining highest TSN numbers unacknowledged."
Done.
> 
> (7) See this, and another comment later:
> "However, in so doing, it MUST react just like an
>    implementation that does NOT support fragmentation, i.e., it MUST
>    reject sends that exceed the current Path MTU (PMTU)."
> - Would be more complete as:
> "An implementation that disables fragmentation MUST react just like an
>    implementation that does NOT support fragmentation, i.e., it MUST
>    reject sends that exceed the current Path MTU (PMTU)."
Done.
> 
> (8)
> "Partial chunks MUST NOT be placed in an SCTP packet.  A partial chunk
>    is a chunk that is not completely contained in the SCTP packet; i.e.,
>    the SCTP packet is too short to contain all the bytes of the chunk as
>    indicated by the chunk length."
> - what does the specification say to do if the chunk size is too large
> to be sent in a full-sized SCTP packet with the current PMTU?
I don't think the spec says anything. The only way to progress is making
use of IP-layer fragmentation. Do you think we need to add text for this?
> 
> (9)
> "The basic
>    guidelines for incrementing cwnd during congestion avoidance are as  follows:
> 
>    o  SCTP MAY increment cwnd by 1*MTU.
> 
>    o  SCTP SHOULD increment cwnd by 1*MTU once per RTT when the sender
>       has cwnd or more bytes of data outstanding for the corresponding
>       transport address.
> 
>    o  SCTP MUST NOT increment cwnd by more than 1*MTU per RTT."
> 
> - these seem IETF "recommended" rather than guidelines?
OK. Changed to
The basic recommendations for incrementing cwnd during congestion avoidance
are as follows:

> 
> (10)
> 
> “The parameters SHOULD
>       decay if the address is not used for a long enough time period.”
> - how long? There needs to be some guidance, even if this is in the form of a note.
Hmm. https://tools.ietf.org/html/rfc5681#section-4.1 talks about "a relatively long
period of time". Lateron is uses a retransmission timeout for this amount of time.
So I added: <xref target='RFC5681'/> specifies this long enough time as a retransmission timeout.
> 
> (11)
> "The counter MUST be reset each time a DATA chunk sent to that peer
>    endpoint is acknowledged (by the reception of a SACK). "
> - Could this be: "The counter used for endpoint failure detection MUST be..."
OK. The while subsection is about this, but we can make is clearer.
> - Actually, this places a formal requiremen on a counter that is a "SHOULD" to implement!
My understanding is, if you implement it (which is a should) then you must do it as
described.
> - So, should this be the "The method used for endpoint failure detection MUST be..."?
Just tool the text above.
> - Please suggest something.
> 
> (12) The text:
> "The receiver of an OOTB packet MUST do the following:"
>  is followed by a list of shoulds and musts. I think this construct is not helpful.
> Please replace each bullet with a SHOULD or MUST requirement and remove the "MUST"
> from the top of the list!
Done.
> 
> (13)
> "Res: 4 bits Should be set to all '0's and ignored by the receiver."
> - Why is this not a MUST be set to zero and MUST be ignored by the receiver?
> - Albeit you could not that future RFCs might update this requirement?
In the other places we just use "Set to 0 on transmit and ignored on receipt."
I'm use it here also for consistency. We can change it globally, if wanted.
> 
> (14)
> 
> This didn't quite make sense to me:
> "The Gap Ack Blocks SHOULD be
>    isolated.  This means that the TSN just before each Gap Ack Block and
>    the TSN just after each Gap Ack Block have not been received. By
>    definition, all TSNs acknowledged by Gap Ack Blocks are greater than
>    the value of the Cumulative TSN Ack."
> - Please consider if you can make the SHOULD clearer and the following sentence.
What do you think is not clear. The second sentence make it as clear as possible to me...
> 
> (15)
> "An endpoint SHOULD send this chunk to its peer endpoint to probe"
> - ought to be something like:
> "An endpoint SHOULD send a Heartbeat Request chunk to its peer endpoint to probe"
Changed to
An endpoint SHOULD send a HEARTBEAT chunk to its peer endpoint to probe the
> 
> (16)
> "If the sender does not wish to
>       provide this information, it SHOULD set the Measure of Staleness
>       field to the value of zero."
> - ought to be something like:
> "If the sender does not wish to
>       provide the Measure of Staleness, it SHOULD set this
>       field to the value of zero."
Done.
> 
> (17)
> 
> "  An endpoint SHOULD NOT send a DATA chunk with no user data part."
> - Why not? What is the negative impact of not following this recommendation in 6.2?
The problem is in the socket layer. When using a connection oriented socket
(like a 1-to-1 style socket), a receive call returning 0 means that the
peer is not sending anything anymore and not that a user message of length
0 has been received. For UDP (not connection oriented), this is not a problem.
> 
> (18)  This appears a requirement in an implementation note. Please either
> remove the "implementation note", or replace by "ought" or something different:
> "  IMPLEMENTATION NOTE: RTT measurements SHOULD only be made using
>         a chunk with TSN r if no chunk with TSN less than or equal to r
>         is retransmitted since r is first sent."
Removed "IMPLEMENTATION NOTE".
> 
> 
> (19)
> 
> I wonder what the requirement really is here...
> This comes after a normative requirement to use a CRC32c, but I am not clear
> what action is needed in this recommendation:
> "   Any hardware implementation SHOULD be done in a way that is
>    verifiable by the software."
> - Is this intended to somehow utilise appendix C?
I guess it means that if the hardware does not tell you that the CRC is correct,
you still need to be able to do a verification in software. Intel cards have this
"feature" for small SCTP packets: the don't report that the CRC is correct, and
the software will do the computation and make the final decision. And Intel got
it wrong over and over again...
> 
> (20)  Section 6.9:
> - Section 6.9 has multiple “notes” that contain RFC2119 requirements. These need reworded to remove the requirement, or I think in this case to place the requirement outside of the note.
I just removed the Note:
> - Section 6.9 redefines PMTU - already defined earlier.
I don't think it is defined earlier, just used earlier. Should we put it in Section 2.3?
> - “Once a message is fragmented, it cannot be re-fragmented.” and then proceeds to talk about IP fragmentation and later SCTP fragmentation. The type of fragmentation needs to be more clearly explained, this is confusing.
Changed to "user message". It only talks about IP-level fragmentation when explicitly saying so.
All mentioning of "fragmentation" is SCTP level fragmentation of user messages.
> “If an implementation does not
>    support fragmentation of outbound user messages, the endpoint MUST
>    return an error to its upper layer and not attempt to send the user
>    message.”
> - This sentence needs the full context to provide a clear requirement, i.e. “An endpoint that does not support fragmentation and is requested to send a … MUST …”
Changed to
An endpoint that does not support fragmentation and is requested to send a
user message such that the outbound SCTP packet size would exceed the current MTU
MUST  return an error to its upper layer and MUST NOT attempt to send
the user message.</t>
> “The association Path MTU is
>    the smallest Path MTU of all destination addresses.”
> - could be:
> ““The association Path MTU is defined as the smallest Path MTU for all destination addresses.”
Done.
> 
> (21)
> “When the T3-rtx timer expires on an address, SCTP SHOULD perform slow
>    start by:
> 
>       ssthresh = max(cwnd/2, 4*MTU)
>       cwnd = 1*MTU
>       partial_bytes_acked = 0“
> - I have a question: Why is the reaction to the RTO not a requirement? I’d like to question whether this needs to be a “MUST”?
I guess it was to allow other algorithms to be implemented.
> 
> (22)
> 
> I suggest section 7.3 is normatively updated to replace RFC4821 with a reference to dplpmtud. - done in latest rev.
Yepp.
> 
> (23)
> 
> "An endpoint MUST  maintain separate MTU estimates for each destination address of
>        its peer."
> - I think this is about Path MTU, not MTU?
Correct. Fixed.
> 
> (24)
> “Note: When configuring the SCTP endpoint, the user SHOULD. “
> - RFC2119 text in a note. This note specifically is about user behaviour? Is this really just guidance - could it be “ought”?
Using ought
> 
> (25)
> “   The upper layer, i.e., the SCTP user, SHOULD standardize any specific
>    protocol identifier with IANA if it is so desired.  The use of any
>    specific payload protocol identifier is out of the scope of SCTP.”
> - I am not sure whether this is a SHOULD, and if it is, what registry and procedure is to be used.
The payload protocol identifier registry described in 14.5.
> - What happens if they do not?
They use 0.
> 
> (26)
> 
> Appendix A contains normative text. I appreciate that ECN could be optional, but I am less than happy to see normative text in an appendix. Could this be moved to the body of the specification?
Or be removed? I don't recall that ECN was really tested in interops.
The reason it was put in an Appendix was that people were not sure if it is right
approach.
If you look at https://tools.ietf.org/html/draft-stewart-tsvwg-sctpecn-05#section-4.1
you see that chunks are defined differently. However, that document was never accepted
as a WG document.

Right now: leaving it as it is.
> 
> (27)
> 
> Appendix C contains normative text.  Could this be moved to the body of the specification?
OK. Moved after "Termination of Association".
> - Appendix C contains a final note with a normative requirement. Could in this case it be replace /MUST abort associations/this specification requires associations to aborted/
I removed the "Note that" and kept normative text.
> - “do NOT support SCTP” should be “do not support SCTP”.
Fixed.
> 
> (28)
> “Note that a parameter type MUST be unique across all chunks.  “
> - RFC2119 text in a note. remove “Note”?
Done.
> 
> (29)
> “Note that if the receiver of the INIT
>    chunk is NOT going to establish an association (e.g., due to lack of
>    resources), an 'Unrecognized Parameter' would NOT be included with
>    any ABORT being sent to the sender of the INIT.”
> /NOT/not/.
Fixed.
> 
> (30)
> “   Note: Any time a COOKIE ECHO is sent in a packet, it MUST be the
>    first chunk.”
> - RFC2119 text in a note. remove “Note:”?
Done.
> 
> (31)
> ”Note that this field is NOT touched
>       by an SCTP implementation; therefore, its byte order is NOT
>       necessarily big endian.  “
> /NOT/not/.
Done.
> 
> (32).
> Is this restating requirements elsewhere?
> “   Note 3: An INIT chunk MUST NOT contain the Host Name Address
>    parameter.  The receiver of an INIT chunk containing a Host Name
>    Address parameter MUST send an ABORT and MAY include an "Unresolvable
>    Address" error cause.”
> - If it is please make the keywords lowercase. If it is not please move this to the main body and remove the “note” tag.
Removed the note tag.
> 
> (33)
> “IMPLEMENTATION NOTE: If an INIT ACK chunk is received with known
>    parameters that are not optional parameters of the INIT ACK chunk,
>    then the receiver SHOULD process the INIT ACK chunk and send back a
>    COOKIE ECHO.  The receiver of the INIT ACK chunk MAY bundle an ERROR
>    chunk with the COOKIE ECHO chunk.  However, restrictive
>    implementations MAY send back an ABORT chunk in response to the INIT
>    ACK chunk.
> “
> - RFC2119 text in a note. remove “Note:”, or separate the NOTE into one para and the normative language into another.
Removed "IMPLEMENTATION NOTE".
> 
> 
> (34)
> “Note: Since the SHUTDOWN message does not contain Gap Ack Blocks,
>       it cannot be used to acknowledge TSNs received out of order.  In a
>       SACK, lack of Gap Ack Blocks that were previously included
>       indicates that the data receiver reneged on the associated DATA
>       chunks.  Since SHUTDOWN does not contain Gap Ack Blocks, the
>       receiver of the SHUTDOWN MUST NOT interpret the lack of a Gap Ack
>       Block as a renege.  (See Section 6.2 for information on reneging.)”
> - RFC2119 appear to follow in a note. Please separate the NOTE into one para and the normative language into another.
Splitted.
> 
> (35)
> Notes after figure 3 contain normative requirements. Suggest replace “NOTES”.
replaced by "The following applies".
> 
> (36)
> “Note: The COOKIE ECHO chunk MAY be bundled with any pending
>        outbound DATA chunks, but it MUST be the first chunk in the
>        packet and until the COOKIE ACK is returned the sender MUST NOT
>        send any other packets to the peer.
> “
> - RFC2119 appear to follow in a note. Please separate the NOTE into one para and the normative language into another.
Just removed "Note:"
> 
> (37)
> “   Note: T1-init timer and T1-cookie timer SHOULD follow the same rules
>    given in Section 6.3.  If the application provided multiple IP
>   addresses of the peer, there SHOULD be a T1-init and T1-cookie timer
>    for each address of the peer.  Retransmissions of INIT chunks and
>    COOKIE ECHO chunks SHOULD use all addresses of the peer similar to
>    retransmissions of DATA chunks.”
> - RFC2119 text in a note. remove “Note:”, or separate the NOTE into one para and the normative language into another.
Just removed "Note:"
> 
> (38)
> “   Note that a COOKIE ECHO chunk that does NOT pass the integrity check
>    is NOT considered an 'invalid parameter' and requires special
>    handling; see Section 5.1.5.”
> /NOT/not/.
Fixed.
> 
> (39)
> “IMPLEMENTATION NOTE: If an SCTP endpoint that only supports either
>    IPv4 or IPv6 receives IPv4 and IPv6 addresses in an INIT or INIT ACK
>    chunk from its peer, it MUST use all the addresses belonging to the
>    supported address family.  The other addresses MAY be ignored. The
>    endpoint SHOULD NOT respond with any kind of error indication.
> 
>    IMPLEMENTATION NOTE: If an SCTP endpoint lists in the 'Supported
>    Address Types' parameter either IPv4 or IPv6, but uses the other
>    family for sending the packet containing the INIT chunk, or if it
>    also lists addresses of the other family in the INIT chunk, then the
>    address family that is not listed in the 'Supported Address Types'
>    parameter SHOULD also be considered as supported by the receiver of
>    the INIT chunk.  The receiver of the INIT chunk SHOULD NOT respond
>    with any kind of error indication.”
> - RFC2119 appear to follow in a note.
Just removed "IMPLEMENTATION NOTE".
> 
> 
> (40)
> “   Note: For any case not shown in Table 2, the cookie SHOULD be
>    silently discarded.”
> - RFC2119 text in a note. remove “Note:”,
Done.
> -
> (41)
> Notes after figure 6 contain normative requirements. Suggest replace “NOTES”.
Changed to "The following applies"
> 
> (42)
> “Note that a zero window probe SHOULD only be
>        sent when all outstanding DATA chunks have been cumulatively
>        acknowledged and no DATA chunks are in flight.  “
> - RFC2119 text in a note. remove “Note that”,
Done.
> 
> (43)
> “IMPLEMENTATION NOTE: When the window is full (i.e., transmission is
>    disallowed by rule A and/or rule B), the sender MAY still accept send
>    requests from its upper layer, but MUST “
> - RFC2119 text in a note. remove “Note that”,  or “Implementation requirement”
Removed IMPLEMENTATION NOTE.
> 
> 
> (44)
> “   Note: The data sender SHOULD NOT use a TSN that is more than 2**31 -
>    1 above the beginning TSN of the current send window.
> 
>    Note: For each stream, the data sender SHOULD NOT have more than
>    2**16 - 1 ordered user messages in the current send window.”
> -RFC2119 text in a note. remove “Note that”,
Done.
> 
> —
> 
> I have three additional requests:
> 
> (a) Check the remaining text for RFC2119 text in notes. There are a few other cases and it would be silly to call all these out. The word “note” is always problematic when combined with a requirement.
Done.
> (b) Please check for RFC2119 text with “Implementation note”
> - could this be “Implementation requirements” or something similar calling out to implementors but avoiding the word note. The word “note” is always problematic when combined with a requirement.
Done.
> (c) Please check for upper case “NOT” this appears to be using in several places where lower case “not” is actually correct/
Fixed all occurrences.
>