Last Call Review of draft-ietf-tsvwg-rfc4960-bis-15
review-ietf-tsvwg-rfc4960-bis-15-artart-lc-lear-2021-10-04-00

This review does not delve into security considerations, nor does it
delve deeply into things that weren't changed across version (to the
best I could determine).  There is at least one major exception to
that below.


Major

Section 3.2

Table 2 discusses processing of unknown chunks.  This sort of
packet-level processing for unknown chunks strikes me as outdated.
Why isn't SCTP using some sort of extension negotiation?  I mean,
Telnet did that.  If you're going to update this spec, at least
consider trying to do away with this aspect.  I also think it lends
itself to silent protocol failures, and would fly in the face of
draft-iab-use-it-or-lose-it.


Similarly, consider the text in this note:

      Note: A robust implementation is expected to accept the chunk
      whether or not the final padding has been included in the Chunk
      Length.

I understand that you are attempting to adhere to Postel's Law, but
you should discuss whether that is appropriate in this circumstance.
Is this a protocol error?  If so, you should consider failing.

I'm not saying you MUST do away with this, but the WG should discuss.

This applies equally to Section 3.2.2.


Section 6.1:

   B)  At any given time, the sender MUST NOT transmit new data to a
       given transport address if it has cwnd + (PMDCS - 1) or more
       bytes of data outstanding to that transport address.  If data is
       available, the sender SHOULD exceed cwnd by up to (PMDCS - 1)
       bytes on a new data transmission if the flightsize does not
       currently reach cwnd.  The breach of cwnd MUST constitute one
       packet only.

Can you explain the logic of the change?  People are now going to
start seeing packets that exceed cwnd, and that is likely to confuse
existing receiving endpoints.  If you're going to leave this text in,
I would go into detail somewhere as to when this should happen and
why.  It just seems to violate the principle of least astonishment.

Section 7.2

It seems to me that much of this section could use some pseudo-code to
describe proper behavior.

The TCB

I think somewhere it would be useful to show the minimal TCB.

ALL of Section 10, but especially this text:

   These procedures differ from [RFC1122] and from its requirements for
   processing of port-unreachable messages and the requirements that an
   implementation MUST abort associations in response to a "protocol
   unreachable" message.

Is this or any text in this section intended to modify any behavior
specified in RFC 4443/STD89?  I'm specifically thinking of Sections
3.1 and 3.2.

Minor

It would be good to do a quick rewrite of the abstract to make it more
concise.

Section 2.3

Make clearer the relationship between PMDCS and PMTU with a calculation.

PMDCS = PMTU - SCTP header
PMTU = PMDCS + SCTP header

Right?

In Section 2.5.5 I would suggest "Each chunk MUST contain..."  After
all, what else could they contain?  "Might" seems inappropriate.

The next paragraph:

   During times of congestion, an SCTP implementation MAY still perform
   bundling even if the user has requested that SCTP not bundle.  The
   user's disabling of bundling only affects SCTP implementations that
   might delay a small period of time before transmission (to attempt to
   encourage bundling).  When the user layer disables bundling, this
   small delay is prohibited but not bundling that is performed during
   congestion or retransmission.

This needs rewording.  For one thing, the last sentence is not
grammatically correct.  Second, we do not use "prohibited".  Please
use active tense with normative words like "MUST [NOT]".  More
importantly, the text seems contradictory and is difficult to interpret.

2.5.6

   The CRC32c checksum can be set
   by the sender of each SCTP packet to provide additional protection
   against data corruption in the network.  The receiver of an SCTP
   packet with an invalid CRC32c checksum silently discards the packet.

I would prefer that you use normative language here.  The CRC32c
MAY/MUST/SHOULD be.. and then then The receiver "MUST silently
discard".  Also, it seems good at this stage to add what an unset
value is (0?), so that the receiver knows when NOT to discard.

Section 3:

   Multiple chunks can be bundled into one SCTP packet as long as the
   size, except for the INIT, INIT ACK, and SHUTDOWN COMPLETE chunks.

Again, here you SHOULD be using normative language.
s/can/[MAY/MUST/SHOULD].  Also, I would reword this slightly as
follows:

   INIT, INIT ACK and SHUTDOWN COMPLETE chunks MUST NOT be bundled
   into one SCTP packet.  All other chunks MAY be bundled up to the
   size indicated by the PMTU.  See section 6.10 for more details on
   chunk bundling.

   
Section 3.2.1

     Chunk Parameter Length: 16 bits (unsigned integer)

      The Parameter Length field contains the size of the parameter in
      bytes, including the Parameter Type, Parameter Length, and
      Parameter Value fields.  Thus, a parameter with a zero-length
      Parameter Value field would have a Parameter Length field of 4.
      The Parameter Length does not include any padding bytes.

First, I don't understand why you added the word "Parameter" in the
4th line.  Second, is the name of field Paramater Length or Chunk
Parameter Length?  Please improve for clarity.

Directly below:

   The total length of a parameter (including Parameter Type, Parameter
   Length, and Parameter Value fields) MUST be a multiple of 4 bytes.
   If the length of the parameter is not a multiple of 4 bytes, the
   sender pads the parameter at the end (i.e., after the Parameter Value
   field) with all zero bytes.  The length of the padding is not
   included in the Parameter Length field.  A sender MUST NOT pad with
   more than 3 bytes.  The receiver MUST ignore the padding bytes.

Why is this in the definition of Chunk Parameter Value?  Doesn't it
belong in Chunk Paramter Length?

Section 3.3.1

  Res:

Please indicate the change here that these bits are reserved.
Changing packet field names to accommodate ASCII art should be done
with care, as other documentation (like books) may reference them.

Section 3.3.2:

   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.  The receiver of the INIT chunk
   MAY bundle an ERROR chunk with the COOKIE ACK chunk later.  However,
   restrictive implementations MAY send back an ABORT chunk in response
   to the INIT chunk.

I don't understand the circumstances under which all but the first
sentence apply.  Consider elaborating.  Same for 3.3.3.

Address type:

What should a receiver do if they see a Host Name Address parameter
(Host name = 11)?  Specify behavior.

Section 3.3.4


   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.

and then later in that same section:
     
      Gap Ack Blocks SHOULD be isolated.  This means that
      the DATA chunks with TSNs equal to (Cumulative TSN Ack + Gap Ack
      Block Start - 1) and (Cumulative TSN Ack + Gap Ack Block End + 1)
      have not been received.

Not ok.  You are stating a defintion in two different ways.  Pick one,
and don't repeat the definition.

Section 6.8:

   Any hardware implementation SHOULD permit alternative verification of
   the CRC in software.

What value is this statement, and why do you care?  Does it help
interoperability?  Is this a matter of algorithm agility?  If so, I'll
cleam there's a lot more work needing to be done.

Section 6.9:

   Once a user message is fragmented, it cannot be re-fragmented

s/cannot/MUST NOT/?


Section 7 1st paragraph:

   For some applications, it might be likely that adequate resources
   will be allocated to SCTP traffic to ensure prompt delivery of
   time-critical data -- thus, it would appear to be unlikely, during
   normal operations, that transmissions encounter severe congestion
   conditions.  However,...

The entire 1st paragraph could be replaced with one sentence:

   To manage congestion, the mechanisms and algorithms in this section
   are to be employed.

The rest is just waffle, and best left for researchers to
characterize.

In Section 8.2:

   When an outstanding TSN is acknowledged or a HEARTBEAT chunk sent to
   that address is acknowledged with a HEARTBEAT ACK chunk, the endpoint
   SHOULD clear the error counter of the destination transport address
   to which the DATA chunk was last sent (or HEARTBEAT chunk was sent)
   and SHOULD also report to the upper layer when an inactive
   destination address is marked as active.

Do you have an example of how this notification should occur if the
upper layer is the application?



Nits

A bit too aggressive with may->might.  In Section 2.3, the Bundling
definition probably should be "can" instead of "might".  "Might"
introduces some element of uncertainty.

Section 3.2, Chunk length

s/, if the Chunk/If the Chunk/

Section 3.3.1 U bit

s/,/Therefore,/

Oddly, same with Payload Protocol Identifier...

s/,/,therefore/


Section 5.4

Redundant:

   However, it is possible that a
   misbehaving peer might supply addresses that it does not own. 

Try:

However, a misbehaving peer might supply addresses that it does not
   own.

Section 6.9:

   If an implementation that supports fragmentation makes available to
   its upper layer a mechanism to turn off fragmentation, it might do
   so.

Well, yes ;-)  I would suggest the following rewrite:

  An implementation MAY make available its upper layer a mechanism to
  turn off (disable?) fragmentation.  When in such a state, it MUST
  behave as an implementation that does not support fragmentation.

Further down in Fragmentation:
  The sender SHOULD choose the size of the DATA chunks is
  smaller than or equal to the AMDCS.

s/chunks is/chunks that is/


Section 7.2:

   When doing accounting for a DATA chunk related to one of these
   variables

I suggest the following:

   When calculating one of these variables...

Let's leave accounting out of this ;-)

Section 10:

If you're not going to reference ICMP1-9, there's no point in labeling
them as such, and you should just use a normal list.