Re: [tsvwg] Review of draft-ietf-tsvwg-rfc4960-bis-08

[Timo Völker <[e-mail]>]

Hi Michael,

I looked at the difference between draft-ietf-tsvwg-rfc4960-bis-08 and draft-ietf-tsvwg-rfc4960-bis-10. Your changes address my issues. Thanks.

By scanning through the diff, I found a few more issues.

Section 2.3.
Repeating "which can be" in the definition of Path Maximum Transmission Unit (PMTU).

Section 7.2.1.
I would write IPv4 or IPv6 (lower case v) instead of IPV4 or IPV6.

Section 11.
OLD: However, all SCTP implementation
NEW: However, all SCTP implementations

Section 14.2.
OLD: Receive Buffer:  A buffer to store received user data which is not yet delivered to the upper layer.
NEW: Receive Buffer:  A buffer to store received user data which are not yet delivered to the upper layer.

Throughout the document.
I often see two whitespaces instead of one after a period that ends a sentence. A text search for ".  " finds examples.

Association PMTU
Section 6 contains this text:
   1)  When converting user messages into DATA chunks, an endpoint will
       fragment user messages into multiple DATA chunks such that the
       resulting SCTP packet size does not need to exceed the current
       association PMTU.
Does the "will" mean MUST, SHOULD or MAY?
It looks like this is the first time the term association PMTU is used. However, the term is only defined in Section 6.9. Does the term deserves an entry in Section 2.3. Key Terms?

Timo

> On 8. Feb 2021, at 00:14, Michael Tuexen <[e-mail]> wrote:
> 
> Hi Timo,
> 
> thank you very much for the detailed review.
> See my comments in-line.
> 
> Best regards
> Michael
> 
>> On 31. Dec 2020, at 11:02, Timo Völker <[e-mail]> wrote:
>> 
>> Hi,
>> 
>> Here is my review for the RFC4960bis draft (draft-ietf-tsvwg-rfc4960-bis-08).
>> 
>> My list starts with minor issues (typos, etc.) followed by comments about some terms.
>> 
>> 
>> *Typos, Wording*
>> 
>> Section 3.1
>> 
>> Verification Tag:
>> OLD: The receiver of this packet uses the Verification Tag to validate the sender of this SCTP packet. On transmit, the value of this Verification Tag MUST be set to...
>> NEW: The receiver of an SCTP packet uses the Verification Tag to validate the sender of this packet. On transmit, the value of the Verification Tag MUST be set to...
> Fixed.
>> 
>> Checksum:
>> OLD: This field contains the checksum of this SCTP packet.
>> NEW: This field contains the checksum of the SCTP packet.
> Fixed.
>> 
>> 
>> Section 3.2
>> 
>> Chunk Type encoding
>> OLD: ...and report the unrecognized chunk in an 'Unrecognized Chunk Type'.
>> NEW: ...and report the unrecognized chunk in an ERROR chunk using the 'Unrecognized Chunk Type' cause of error.
> Fixed by using
> and report the unrecognized chunk in an ERROR chunk using the 'Unrecognized Chunk Type' error cause.
> This is consistent with other places in the text. Also fixed the other similar text in the same table.
>> 
>> Text about padding in a chunk repeats. First described in the description for Chunk Length field and then directly below the description of the chunk fields. I suggest to remove it in the description of the Chunk Length field, but to keep the relevant part for the field.
>> OLD: The Chunk Length field does not count any chunk padding. Chunks (including Type, Length, and Value fields) are padded...
>> NEW: The value in the Chunk Length field does not include any chunk padding. However, it does include padding of any variable-length parameter except the last parameter in the chunk.
> Fixed.
>> 
>> 
>> Section 3.2.1
>> 
>> Start the enumeration with the word Parameter.
>> OLD: The total length of a parameter (including Type, Parameter Length, and Value fields)...
>> NEW: The total length of a parameter (including Parameter Type, Length, and Value fields)...
> Changed to using Parameter for all three.
>> 
>> The text about Parameters begins like any chunk may include parameters. If this generic form was used by intention, I suggest to rewrite the part below Table 3, which refers to specific chunks.
>> OLD: Please note that in all four cases, an INIT ACK or COOKIE ECHO chunk is sent.
>> NEW: Please note that, when an INIT or INIT ACK chunk is received, in all four cases, an INIT ACK or COOKIE ECHO chunk is sent in response, respectively.
> Fixed.
>> 
>> This continues in Section 3.2.2.
>> OLD: If the receiver of an INIT ACK chunk detects unrecognized parameters and has to report them according to Section 3.2.1, it SHOULD bundle the ERROR chunk containing the 'Unrecognized Parameters' error cause with the COOKIE ECHO chunk sent in response to the INIT ACK chunk.
>> NEW: If the receiver of any other chunk (e.g., INIT ACK) detects unrecognized parameters and has to report them according to Section 3.2.1, it SHOULD bundle the ERROR chunk containing the 'Unrecognized Parameters' error cause with a chunk sent in response (e.g., COOKIE ECHO).
> Fixed.
>> 
>> 
>> Section 3.3.2
>> 
>> Table 5 and 6 lists all specified parameters for the INIT chunk, including the Host Name Address parameter. The text before the tables "The INIT chunk contains the following parameters" does not match to the text after the tables "An INIT chunk MUST NOT contain the Host Name Address parameter".
>> OLD: The INIT chunk contains the following parameters.
>> NEW: The following parameters are specified for the INIT chunk.
>> OLD: Host Name Address Optional 11
>> NEW: Host Name Address Deprecated 11
> Fixed.
>> 
>> I find the text about known parameters that are not optional parameters difficult to understand, since based on the tables, I assumed not optional means mandatory.
>> OLD: If an INIT chunk is received with known parameters that are not optional parameters of the INIT chunk,
>> NEW: If an INIT chunk is received with parameters that are specified for the INIT chunk,
> Changed to
> If an INIT chunk is received with all mandatory parameters that are
> specified for the INIT chunk, then the receiver SHOULD process the INIT chunk
> and send back an INIT ACK.
>> 
>> 
>> Section 3.3.3
>> 
>> It seems like the position of tables 7 and 8 should be directly after this paragraph "The parameter part of INIT ACK is formatted similarly to the INIT chunk. It uses two extra variable parameters: The State Cookie and the Unrecognized Parameter."
> Cleaned up to be consistent with the description of the INIT chunk.
>> 
>> The description for the Advertised Receiver Window Credit in the INIT chunk contains an additional half sentence.
>> OLD: ...SHOULD NOT be lessened (i.e., dedicated buffers taken away from this association).
>> NEW: ...SHOULD NOT be lessened (i.e., dedicated buffers taken away from this association); however, an endpoint MAY change the value of a_rwnd it sends in SACK chunks.
> Fixed.
>> 
>> Similar to my suggestion for the Section 3.3.2
>> OLD: If an INIT ACK chunk is received with known parameters that are not optional parameters of the INIT ACK chunk,
>> NEW: If an INIT ACK chunk is received with parameters that are not specified for the INIT ACK chunk,
> Changed to:
> If an INIT ACK chunk is received with all mandatory parameters that are
> specified for the INIT ACK chunk, then the receiver SHOULD process the
> INIT ACK chunk and send back a COOKIE ECHO chunk.
>> 
>> 
>> Section 3.3.3.1.2
>> 
>> Type seems to me the better word here.
>> OLD: ...an unrecognized parameter that has a value that indicates it SHOULD be reported to the sender.
>> NEW: ...an unrecognized parameter that has a type that indicates it SHOULD be reported to the sender.
> Fixed.
>> 
>> 
>> Section 3.3.7
>> 
>> OLD: ...but they MUST be placed before the ABORT in the SCTP packet or they will be ignored by the receiver.
>> NEW: ...but they MUST be placed before the ABORT in the SCTP packet, otherwise they will be ignored by the receiver.
> Fixed.
>> 
>> 
>> Section 4
>> 
>> State Diagram
>> * Use send and start, instead of snd and strt.
> Fixed
>> * Add a space after "(B)", "(C)" and "(D)"
> Fixed
>> * There is an arrowhead missing on the line between SHUTDOWN-RECEIVED and SHUTDOWN-ACK-SENT.
> Fixed
>> * The position of "(B)rcv SHUTDOWN" looks like it belongs to the arrow that is pointing to CLOSED.
> Fixed
>> 
>> 
>> Section 5.1.6
>> 
>> In Figure 4
>> OLD: DATA [TSN=initial TSN_A
>> NEW: DATA [TSN=init TSN_A
>> OLD: DATA [TSN=init TSN_Z
>> NEW: DATA [TSN=init TSN_Z,
> Fixed.
>> 
>> 
>> Section 6.4
>> 
>> Remove space.
>> OLD: When a receiver of a duplicate DATA chunk sends a SACK to a multi- homed endpoint...
>> NEW: When a receiver of a duplicate DATA chunk sends a SACK to a multi-homed endpoint...
> Fixed.
>> 
>> 
>> Section 6.7
>> 
>> The text reads like the sender should treat all data chunks not acknowledged in an SACK as missing. I assume the highest acknowledged TSN should take into account (as Section 7.2.4 describes).
>> OLD: 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.
>> NEW: All DATA chunks with TSNs not included in the Gap Ack Blocks which are smaller than the highest acknowledged TSN reported by a SACK MUST be treated as "missing" by the sending endpoint.
> Fixed.
>> 
>> 
>> Section 8.4
>> 
>> It seems an "Otherwise" is missing and a "process" should be removed.
>> OLD: 3) ..., indicating that the Verification Tag is not reflected.
>> NEW: 3) ..., indicating that the Verification Tag is not reflected. Otherwise,
> Fixed.
>> OLD: 4) If the packet contains a COOKIE ECHO in the first chunk, process it MUST be processed as described in Section 5.1.
>> NEW: 4) If the packet contains a COOKIE ECHO in the first chunk, it MUST be processed as described in Section 5.1.
> Fixed.
>> 
>> 
>> Section 11
>> 
>> Typo
>> OLD: However, all SCTPs aer expected to provide a...
>> NEW: However, all SCTPs are expected to provide a...
> Fixed.
>> 
>> 
>> Section 11.1.10
>> 
>> OLD: 11.1.10. Request HeartBeat
>> NEW: 11.1.10. Request HeartBeat
>> OLD: Instructs the local endpoint to perform a HeartBeat on the...
>> NEW: Instructs the local endpoint to perform a heartbeat on the...
> Fixed.
>> 
>> 
>> Section 11.1.14
>> 
>> Typo
>> OLD: payload protocol-id: The 32 bit unsigned integer that was sent to be sent to the peer...
>> NEW: payload protocol-id: The 32 bit unsigned integer that was set to be sent to the peer...
> Fixed.
>> 
>> 
>> Section 14.2
>> 
>> Remove space.
>> OLD: Mapping Array: An array of bits or bytes indicating which out-of- order TSNs have been received (relative to the Last Rcvd TSN). If no gaps exist, i.e., no out-of- order packets...
>> NEW: Mapping Array: An array of bits or bytes indicating which out-of-order TSNs have been received (relative to the Last Rcvd TSN). If no gaps exist, i.e., no out-of-order packets...
> Fixed.
>> 
>> 
>> *Terms, Comprehension*
>> 
>> Far End
>> Only Section 2.5.7 and 9.2 use the term "far end". I assume it is a synonym to, for example, the term "peer endpoint". If so, I suggest to use "peer end" or "peer endpoint" in these sections as well.
> Replaced by peer endpoint.
>> 
>> RECEIVE_UNSENT, RECEIVE_UNACKED
>> I do not understand what a RECEIVE_UNSENT and RECEIVE_UNACKED (Section 11.1.14 and 11.1.15) is used for. A sentence before listening the attributes would be helpful.
> Added for RECEIVE_UNSENT:
> <t>This primitive reads a user message, which has never been sent,
> into the buffer specified by ULP.</t>
> Added for RECEIVE_UNACKED:
> <t>This primitive reads a user message, which has been sent and has not been
> acknowledged by the peer, into the buffer specified by ULP.</t>
>> 
>> Protection of Non-SCTP-Capable Hosts
>> I might misunderstand Section 12.4, but for me it seems all paragraphs except the first one are not about protecting Non-SCTP-Capable Hosts. Does the last three paragraph belong to a Section 12.5, which title is missing?
> No, the methods are intended to protect hosts, which do not support SCTP. This text was already in RFC 4960.
> We can chat about the attacks...
>> 
>> Chunk Bundling Option
>> If I disable an option called chunk bundling, I would assume that SCTP does not bundle chunks at all. Section 2.5.5 describes that chunks are still bundled. Also, I guess this only applies to DATA chunks. A name like "Delay for DATA Chunk Bundling" would make it clearer. (Section 11.1.5 describes the no-bundle flag, which refers to that option).
> I made the description clearer:
> <dt>no-bundle flag:</dt>
> <dd><t>instructs SCTP not to delay the sending of DATA chunks for this user data
> just to allow it to be bundled with other outbound DATA chunks.
> When faced with network congestion, SCTP might still bundle the data, even when
> this flag is present.</t></dd>
>> MTU
>> For me, MTU describes the maximum size of an IP packet. Often, this document uses MTU as the maximum size of an SCTP packet. As the maximum size of an SCTP packet is not always the MTU minus IP header, why not using an own term? For example, Maximum SCTP Packet Size (MSPS). I think, this would help to make the text clearer.
>> For example, Section 6.1 contains
>> "the sender SHOULD create a SACK and bundle it with the outbound DATA chunk, as long as the size of the final SCTP packet does not exceed the current MTU."
>> which seems wrong to me. I think replacing MTU with MSPS is the simplest solution.
> I have replaced MTU with PMTU. The document uses the term PMTU now as the maximum IP payload size, which
> is the maximum SCTP packet size, which can reach the peer without IP-level fragmentation.
>> 
>> Timo
>