Eliot: thanks a lot for this thoughtful review! You have made many good points, and helped improve the document.
And thank you to the authors for answering this review, and giving pointers to where discussion has happened. I do agree with Eliot that I would have liked to see some stricter “proper behavior”
defined, but I understand that this is a bis doc, and doing so would create problems for existing implementations, so I am happy with the answer “this has been considered”.
I have balloted No Objection.
Francesca
From: last-call <last-call-bounces@xxxxxxxx> on behalf of Eliot Lear via Datatracker <noreply@xxxxxxxx>
Date: Monday, 4 October 2021 at 16:08
To: art@xxxxxxxx <art@xxxxxxxx>
Cc: last-call@xxxxxxxx <last-call@xxxxxxxx>, tsvwg@xxxxxxxx <tsvwg@xxxxxxxx>, draft-ietf-tsvwg-rfc4960-bis.all@xxxxxxxx <draft-ietf-tsvwg-rfc4960-bis.all@xxxxxxxx>
Subject: [Last-Call] Artart last call review of draft-ietf-tsvwg-rfc4960-bis-15
Reviewer: Eliot Lear
Review result: Almost Ready
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.
--
last-call mailing list
last-call@xxxxxxxx
https://www.ietf.org/mailman/listinfo/last-call
|
