[PATCH lksctp-tools] Remove dependency to implementation-specific a_rwnd behavior

[PATCH lksctp-tools] Remove dependency to implementation-specific a_rwnd behavior

[Matija Glavinic Pecotic]

test_timetolive and test_sctp_sendrecvmsq take advantage of the following lksctp
characteristic:

 - SO_RECVBUF being set prior association is established will control initially
   advertised rwnd
 - Modifying SO_RECVBUF later will keep maximum rwnd at the initial value which
   is defined at the association startup

Test cases use this fact in order to initially advertise small rwnd, increase
buffer later, but keeping the initial rwnd as maximum one. However, we may at
later time increase buffer, and implementation may advertise this to the
partner. RFC4960 only forbids decrease of the buffer space:

   Advertised Receiver Window Credit (a_rwnd): 32 bits (unsigned
   integer)

      This value represents the dedicated buffer space, in number of
      bytes, the sender of the INIT has reserved in association with
      this window.  During the life of the association, this buffer
      space 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.

Having in mind that rwnd reflects free space in receiver's buffer,
implementation may advertise new value, and therefore test case should not
rely on above defined behavior. There is no impact on functional behavior of
test cases due to this change.

This change was done in course of '[PATCH v2] net: sctp: Fix a_rwnd/rwnd
management to reflect real state of the receiver's buffer', as with this patch
behavior is changed from described to rwnd reflecting real buffer state.

Signed-off-by: Matija Glavinic Pecotic <matija.glavinic-pecotic.ext@nsn.com>

--- lksctp-tools.orig/src/func_tests/test_timetolive.c
+++ lksctp-tools/src/func_tests/test_timetolive.c
@@ -172,10 +172,8 @@ int main(int argc, char *argv[])
 	 * This code sets the associations RWND very small so we can
 	 * fill it.  It does this by manipulating the rcvbuf as follows:
 	 * 1) Reduce the rcvbuf size on the socket
-	 * 2) create an association so that we advertize rcvbuf/2 as
+	 * 2) create an association so that we advertise rcvbuf/2 as
 	 *    our initial rwnd
-	 * 3) raise the rcvbuf value so that we don't drop data wile 
-	 *    receiving later data
 	 */
 	len = SMALL_RCVBUF;
 	error = setsockopt(sk2, SOL_SOCKET, SO_RCVBUF, &len,
@@ -239,14 +237,6 @@ int main(int argc, char *argv[])
 	sac = (struct sctp_assoc_change *)iov.iov_base;
 	associd1 = sac->sac_assoc_id;
 
-	/* restore the rcvbuffer size for the receiving socket */
-	error = setsockopt(sk2, SOL_SOCKET, SO_RCVBUF, &orig_len,
-			   sizeof(orig_len));
-
-	if (error)
-		tst_brkm(TBROK, tst_exit, "setsockopt(SO_RCVBUF): %s",
-			strerror(errno));
-
         /* Get the first data message which was sent.  */
         inmessage.msg_controllen = sizeof(incmsg);
         error = test_recvmsg(sk2, &inmessage, MSG_WAITALL);
--- lksctp-tools.orig/src/func_tests/test_sctp_sendrecvmsg.c
+++ lksctp-tools/src/func_tests/test_sctp_sendrecvmsg.c
@@ -142,7 +142,6 @@ int main(int argc, char *argv[])
 
 	/*
 	 * Set the RWND small so we can fill it up easily.
-	 * then reset RCVBUF to avoid frame droppage
 	 */
 	len = sizeof(int);
 	error = getsockopt(sk2, SOL_SOCKET, SO_RCVBUF, &oldlen, &len);
@@ -186,14 +185,6 @@ int main(int argc, char *argv[])
 				    SCTP_ASSOC_CHANGE, SCTP_COMM_UP);
 
 
-	/* restore the rcvbuffer size for the receiving socket */
-	error = setsockopt(sk2, SOL_SOCKET, SO_RCVBUF, &oldlen,
-			   sizeof(oldlen));
-
-	if (error)
-		tst_brkm(TBROK, tst_exit, "setsockopt(SO_RCVBUF): %s",
-			 strerror(errno));
-
 	/* Get the communication up message on sk1.  */
 	buflen = REALLY_BIG;
 	msgname_len = sizeof(msgname);

Re: [PATCH lksctp-tools] Remove dependency to implementation-specific a_rwnd behavior

[Daniel Borkmann]

On 02/09/2014 08:11 AM, Matija Glavinic Pecotic wrote:
> test_timetolive and test_sctp_sendrecvmsq take advantage of the following lksctp
> characteristic:
>
>   - SO_RECVBUF being set prior association is established will control initially
>     advertised rwnd
>   - Modifying SO_RECVBUF later will keep maximum rwnd at the initial value which
>     is defined at the association startup
>
> Test cases use this fact in order to initially advertise small rwnd, increase
> buffer later, but keeping the initial rwnd as maximum one. However, we may at
> later time increase buffer, and implementation may advertise this to the
> partner. RFC4960 only forbids decrease of the buffer space:
>
>     Advertised Receiver Window Credit (a_rwnd): 32 bits (unsigned
>     integer)
>
>        This value represents the dedicated buffer space, in number of
>        bytes, the sender of the INIT has reserved in association with
>        this window.  During the life of the association, this buffer
>        space 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.
>
> Having in mind that rwnd reflects free space in receiver's buffer,
> implementation may advertise new value, and therefore test case should not
> rely on above defined behavior. There is no impact on functional behavior of
> test cases due to this change.
>
> This change was done in course of '[PATCH v2] net: sctp: Fix a_rwnd/rwnd
> management to reflect real state of the receiver's buffer', as with this patch
> behavior is changed from described to rwnd reflecting real buffer state.

Before applying this, does that break user space? I had a test run of
test_timetolive of the latest net tree and without this patch here,
test_timetolive is stuck forever; with this patch, it works again.
Before your kernel change, the test suite passed.

So you're saying that the test case is simply implemented wrong by
"violating" the RFC in the fact that it keeps/resets the old small
init rwnd along. Hence, these hunks should be removed as it "SHOULD
NOT" be lessened.

> Signed-off-by: Matija Glavinic Pecotic <matija.glavinic-pecotic.ext@nsn.com>

Re: [PATCH lksctp-tools] Remove dependency to implementation-specific a_rwnd behavior

[Matija Glavinic Pecotic]

Hello,

On 02/17/2014 02:22 PM, ext Daniel Borkmann wrote:
> On 02/09/2014 08:11 AM, Matija Glavinic Pecotic wrote:
>> test_timetolive and test_sctp_sendrecvmsq take advantage of the following lksctp
>> characteristic:
>>
>>   - SO_RECVBUF being set prior association is established will control initially
>>     advertised rwnd
>>   - Modifying SO_RECVBUF later will keep maximum rwnd at the initial value which
>>     is defined at the association startup
>>
>> Test cases use this fact in order to initially advertise small rwnd, increase
>> buffer later, but keeping the initial rwnd as maximum one. However, we may at
>> later time increase buffer, and implementation may advertise this to the
>> partner. RFC4960 only forbids decrease of the buffer space:
>>
>>     Advertised Receiver Window Credit (a_rwnd): 32 bits (unsigned
>>     integer)
>>
>>        This value represents the dedicated buffer space, in number of
>>        bytes, the sender of the INIT has reserved in association with
>>        this window.  During the life of the association, this buffer
>>        space 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.
>>
>> Having in mind that rwnd reflects free space in receiver's buffer,
>> implementation may advertise new value, and therefore test case should not
>> rely on above defined behavior. There is no impact on functional behavior of
>> test cases due to this change.
>>
>> This change was done in course of '[PATCH v2] net: sctp: Fix a_rwnd/rwnd
>> management to reflect real state of the receiver's buffer', as with this patch
>> behavior is changed from described to rwnd reflecting real buffer state.
> 
> Before applying this, does that break user space? I had a test run of
> test_timetolive of the latest net tree and without this patch here,
> test_timetolive is stuck forever; with this patch, it works again.
> Before your kernel change, the test suite passed.

this is correct observation

> So you're saying that the test case is simply implemented wrong by
> "violating" the RFC in the fact that it keeps/resets the old small
> init rwnd along. Hence, these hunks should be removed as it "SHOULD
> NOT" be lessened.

Here is discussed "it may be changed", and in this particular case, it may grow. What test case relies is that initially set rwnd will never change, regardless of the socket buffer change. However, with proposed fair advertisement, if SO_RECVBUF is changed, new buffer size will be reported to the sender in form of increased/decreased maximum rwnd. This makes tc hang, fillmessage being created with size of increased buffer (much more then REALLY_BIG, at least on my machine where rmem_defualt is 160k) and not the original (SMALL_RCVBUF). This will deplete receiver, hang sender on next message, and since everything is done in same thread, receiver never gets opportunity to read.

maximum (and minimum?) rwnd might be clamped to deal with this particular scenario, but I would still propose to go for completely fair advertisement.

>> Signed-off-by: Matija Glavinic Pecotic <matija.glavinic-pecotic.ext@nsn.com>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH lksctp-tools] Remove dependency to implementation-specific a_rwnd behavior

[Daniel Borkmann]

On 02/09/2014 08:11 AM, Matija Glavinic Pecotic wrote:
> test_timetolive and test_sctp_sendrecvmsq take advantage of the following lksctp
> characteristic:
>
...
> This change was done in course of '[PATCH v2] net: sctp: Fix a_rwnd/rwnd
> management to reflect real state of the receiver's buffer', as with this patch
> behavior is changed from described to rwnd reflecting real buffer state.
>
> Signed-off-by: Matija Glavinic Pecotic <matija.glavinic-pecotic.ext@nsn.com>

Applied.