Re: [tsvwg] WGLC for draft-ietf-tsvwg-sctp-ndata as PS, closes 22 Dec 2016

[Michael Tuexen <[e-mail]>]

> On 19 Dec 2016, at 17:16, Proshin, Maksim <[e-mail]> wrote:
> 
> Hi,
> 
> Thanks for the document! Please find below some comments. Only major comments are included while editorial/minor comments have been sent directly to Michael.
Hi Maxim,

thanks a lot for the review. I addresses the minor comments and responded to your mail.
See my comments in-line.

Best regards
Michael
> 
> 1. Section 1.1: "DATA chunks are not
>   permitted when I-DATA support has been negotiated."
> What exactly does it mean for implementation (both sending and receiving sides)? I think to avoid any problems with the sending side it should be "MUST NOT" in the sentence. For the receiving side I suppose it means stop processing of the packet and silently discard all further chunks in the packet, right? I think this should be clearly stated. It could also be good to clearly state that after association
I think it is clearly stated in 2.2.1:

If I-DATA support has been negotiated for an association, I-DATA chunks MUST be used for all user messages and DATA-chunks MUST NOT be used. If I-DATA support has not been negotiated for an association, DATA chunks MUST be used for all user messages and I-DATA chunks MUST NOT be used.

So if these rules are violated, I think sending an ABORT with protocol violation might be appropriate.
> restart which was with DATA previously but became with I-DATA after the restart, SCTP still may receive packets with DATA and they should be silently discarded due to VTag mismatch following the rules of Section 8.5. from RFC4960 (I suppose this is important to avoid any "protocol violation" reaction). 
Sure. But this is normal restart behaviour.
> 
> 2. Section 2.1: 
> The only differences between the I-DATA chunk in Figure 3 and the
>   DATA chunk defined in [RFC4960] and [RFC7053] is the addition of the
>   new Message Identifier (MID) and Fragment Sequence Number (FSN) and
>   the removal of the Stream Sequence Number (SSN).
> The B bit now has a different (extended) purpose and also specifies if PPID or FSN is used. I think it should be stated.
OK. I added:
The Payload Protocol Identifier (PPID) and the FSN are stored at the
same location of the packet using the B-bit to determine which value is
stored at the location.
> 
> 3. I suppose the draft assumes that SCTP still MAY (i.e. not SHOULD/MUST) support fragmentation even with I-DATA and MUST (i.e. not MAY/SHOULD) support re-assembling. If so, I suggest to clearly state it.
Yes, this hasn't changed. I think we don't need to repeat this point. Especially since I have not see
an implementation not supporting fragmentation...
> 
> 4. Is RFC6096 impacted by new chunks?
No. It only deals with old chunks. When defining a new chunk, you have to define the corresponding flags
registry as we do in this document.
> 
> 5. Section 2.3.1: Support for the I-FORWARD-TSN chunk is negotiated during the SCTP
>   association setup via the Supported Extensions Parameter as defined
>   in [RFC5061].  Only if both end points support the I-DATA chunk and
>   the I-FORWARD-TSN chunk, the partial reliability extension can be
>   used in combination with user message interleaving.
> Not sure how implementation should behave if either of these chunks is unsupported. I think it should be clearly stated.
> Namely if I-DATA is supported but not I-FORWARD-TSN, I believe the partial reliability functionality should be completely off, i.e. not fallback to FORWARD-TSN. 
> If I-DATA is not supported but I-FORWARD-TSN is supported (strange implementation but still according to the draft as it does not prohibit the opposite), it seems both functionalities message interleaving and partial reliability should be off, i.e. there is no way to fallback to FORWARD-TSN, right?  
Right.
> Should the draft state something like "If I-DATA chunk is not used, I-FORWARD-TSN chunk MUST NOT be used."?
There is already text there:

The FORWARD-TSN chunk MUST be used in combination with the DATA chunk
and MUST NOT be used in combination with the I-DATA chunk.  The I-
FORWARD-TSN chunk MUST be used in combination with the I-DATA chunk
and MUST NOT be used in combination with the DATA chunk.
> 
> 6. Section 4.1: 
> MID is 32 bits while SSN is 16 bits so why a new SCTP Receive Information Structure cannot be introduced instead of using the existing one?
One could add such a structure, but is it needed? I don't see a use-case...
> If the new SCTP Receive Information Structure is introduced then a new message receive function should also be introduced (instead of recv_msg() ). Is that the main problem?
recvmsg() is generic. It doesn't need to be changed. You would add another CMSG.
> Of course, it would also be a problem if the interleaving functionality requires enabling only on the sender side (see the next comment).
No, it is required to be enabled on both sides.
> 
> 
> 7. Section 4.3.1:
>   User message interleaving is disabled per default.
> I'm not sure about it. First of all I think it should be clearly stated what does it mean for the sender and for the receiver.
> If the intention was that I-DATA is included to INIT and INIT ACK only when the user enabled this functionality then I think this is not a good way because then it impacts not only the sender but also the receiver. 
> In other words, if the feature is disabled by default on both sides while SCTP on both sides support it then it is not enough to just enable it on one side, it requires enabling it on the other side.  
Exactly. I think this is stated in the first sentence of 2.2.1.
> 
> 7. Section 4.3.1:  Why don't specify exactly value 1 for the enabled feature? Otherwise, in the future it will be difficult to use another value of this parameter for a different purpose as it will be non-backward compatible (old application and new SCTP may work improperly).
Since it is the standard way of encoding a flag in an int...
> 
> 8. Section 5 does not cover I-FORWARD-TSN.
Good point. I added corresponding text.
> 
> 9. Section 6: In case of DTLS over SCTP support of the message interleaving functionality may increase the risk of receiving messages with new epoch after ChangeCipherSpec message and before Finished message.
Possibly. But implementations have to deal with it anyway. So what is the point?
> 
> 
> BR, Maksim
> 
>> Am 08.12.2016 um 08:36 schrieb [e-mail]:
>> 
>> 
>> This mail announces a TSVWG Working Group Last Call (WGLC) on:
>> 
>>            Stream Schedulers and User Message Interleaving forSCTP
>>            draft-ietf-tsvwg-sctp-ndata
>> https://datatracker.ietf.org/doc/draft-ietf-tsvwg-sctp-ndata
>> 
>> This WGLC will last for 2 weeks weeks.
>> 
>> Comments should be sent to the [e-mail] list, although purely
>> editorial comments may be sent directly to the authors. Please cc: the
>> WG chairs at [e-mail]  if you would like the chairs to
>> track such editorial comments as part of the WGLC process.
>> 
>> No IPR disclosures have been submitted directly on
>> draft-ietf-tsvwg-sctp-ndata.
>> 
>> Thanks,
>> Gorry
>> (TSVWG Co-Chair)