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

From: Michael Tuexen <[email-masked]>
Date: Mon, 13 Mar 2017 08:21:08 +0100

> On 19 Dec 2016, at 17:16, Proshin, Maksim <[email-masked]> 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 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). 
> 
> 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.
>  
> 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.
> 
> 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?  
> Should the draft state something like "If I-DATA chunk is not used, I-FORWARD-TSN chunk MUST NOT be used."?
> 
> 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?
> 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). 
> 
> 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 [email-masked]:
>> 
>> 
>> 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 [email-masked] list, although purely
>> editorial comments may be sent directly to the authors. Please cc: the
>> WG chairs at [email-masked]  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)

---

From: Michael Tuexen <[email-masked]>
Date: Mon, 13 Mar 2017 08:23:01 +0100

Still incomplete, I hit the send button by accident. Will follow up when the response is complete.

Sorry for the noise.

Best regards
Michael
Hi Maxim,

Best regards
Michael

---

From: Michael Tuexen <[email-masked]>
Date: Mon, 13 Mar 2017 11:59:34 +0100

Hi Maxim,

Best regards
Michael
> 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.
> 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 st…
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...
> 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 inter…
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.
> 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...
> 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.

---

From: "Proshin, Maksim" <[email-masked]>
Date: Tue, 21 Mar 2017 06:16:30 +0000

Hi Michael,

Basically I'm ok with all your answers and updates. 
I don't have any major comments but I have a few proposals below inline (by [Maksim] tag).

BR, Maksim

-----Original Message-----
From: Michael Tuexen [mailto:[email-masked]] 
Sent: Monday, March 13, 2017 14:00
To: Proshin, Maksim <[email-masked]>
Cc: [email-masked]
Subject: Re: [tsvwg] WGLC for draft-ietf-tsvwg-sctp-ndata as PS, closes 22 Dec 2016

Hi Maxim,

Best regards
Michael
> 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

[Maksim] Ok, I agree that Section 2.2.1 clearly explains it for the sender. Can the same be explained for the receiver?

So if these rules are violated, I think sending an ABORT with protocol violation might be appropriate.
[Maksim] Ok. Can it be clearly stated (e.g. in 2.2.1)? I mean it would be good to state that such case should be treated as a protocol violation. I also wonder that it should be mentioned that the restart case is different and should not be treated as a violation. 

> 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 ru…
Sure. But this is normal restart behavior.
[Maksim] The reason I've asked it is because the statement is that this is "permitted" while in the restart case it is still possible to receive DATA while I-DATA was already negotiated.
> 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 st…
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.
Right.

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.
One could add such a structure, but is it needed? I don't see a use-case...
[Maksim] Isn't the drawback of the current approach is that 16 bits are lost?
Exactly. I think this is stated in the first sentence of 2.2.1.
[Maksim] I think it is not clearly stated in the document. "Supported" does not really mean "enabled". I would clearly state that this functionality should be enabled on both sides and only in this case it could be successfully negotiated. Especially it is important to state for the receiver side because Section 2.2.1 has the following statement "A sender MUST NOT use the I-DATA chunk unless the user has requested its use (e.g. via the socket API, see Section 4.3)." 
Possibly. But implementations have to deal with it anyway. So what is the point?
[Maksim] Mainly my point was that it could be clarified so that future SCTP implementations of this draft/RFC supporting DTLS could consider it. 
>> editorial comments may be sent directly to the authors. Please cc: 
>> the WG chairs at [email-masked]  if you would like the chairs 
>> to track such editorial comments as part of the WGLC process.
>>

---

From: Michael Tuexen <[email-masked]>
Date: Mon, 27 Mar 2017 22:47:09 +0200

> On 21 Mar 2017, at 07:16, Proshin, Maksim <[email-masked]> wrote:
> 
> Hi Michael,
> 
Hi Maksim,

see my comments in-line. I've removed issues which are resolved.

Best regards
Michael

---

From: Michael Tuexen <[email-masked]>
Date: Mon, 27 Mar 2017 22:48:30 +0200

It went out too early... Sorry for the noise.

Best regards
Michael
Hi Maksim,

Best regards
Michael