[PATCH net 0/5] sctp: a bunch of fixes for stream reconfig

[PATCH net 0/5] sctp: a bunch of fixes for stream reconfig

[Xin Long]

This patchset is to make stream reset and asoc reset work more correctly
for stream reconfig.

Thank to Marcelo making them very clear.

Xin Long (5):
  sctp: use sizeof(__u16) for each stream number length instead of magic
    number
  sctp: only allow the out stream reset when the stream outq is empty
  sctp: only allow the asoc reset when the asoc outq is empty
  sctp: avoid flushing unsent queue when doing asoc reset
  sctp: set sender next_tsn for the old result with ctsn_ack_point plus
    1

 net/sctp/stream.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 65 insertions(+), 12 deletions(-)

-- 
2.1.0

[PATCH net 0/5] sctp: a bunch of fixes for stream reconfig

[Xin Long]

This patchset is to make stream reset and asoc reset work more correctly
for stream reconfig.

Thank to Marcelo making them very clear.

Xin Long (5):
  sctp: use sizeof(__u16) for each stream number length instead of magic
    number
  sctp: only allow the out stream reset when the stream outq is empty
  sctp: only allow the asoc reset when the asoc outq is empty
  sctp: avoid flushing unsent queue when doing asoc reset
  sctp: set sender next_tsn for the old result with ctsn_ack_point plus
    1

 net/sctp/stream.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 65 insertions(+), 12 deletions(-)

-- 
2.1.0

[PATCH net 1/5] sctp: use sizeof(__u16) for each stream number length instead of magic number

[Xin Long]

Now in stream reconf part there are still some places using magic
number 2 for each stream number length. To make it more readable,
this patch is to replace them with sizeof(__u16).

Reported-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/stream.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/sctp/stream.c b/net/sctp/stream.c
index a11db21..09c797a 100644
--- a/net/sctp/stream.c
+++ b/net/sctp/stream.c
@@ -563,7 +563,7 @@ struct sctp_chunk *sctp_process_strreset_outreq(
 		flags = SCTP_STREAM_RESET_INCOMING_SSN;
 	}
 
-	nums = (ntohs(param.p->length) - sizeof(*outreq)) / 2;
+	nums = (ntohs(param.p->length) - sizeof(*outreq)) / sizeof(__u16);
 	if (nums) {
 		str_p = outreq->list_of_streams;
 		for (i = 0; i < nums; i++) {
@@ -627,7 +627,7 @@ struct sctp_chunk *sctp_process_strreset_inreq(
 		goto out;
 	}
 
-	nums = (ntohs(param.p->length) - sizeof(*inreq)) / 2;
+	nums = (ntohs(param.p->length) - sizeof(*inreq)) / sizeof(__u16);
 	str_p = inreq->list_of_streams;
 	for (i = 0; i < nums; i++) {
 		if (ntohs(str_p[i]) >= stream->outcnt) {
@@ -927,7 +927,8 @@ struct sctp_chunk *sctp_process_strreset_resp(
 
 		outreq = (struct sctp_strreset_outreq *)req;
 		str_p = outreq->list_of_streams;
-		nums = (ntohs(outreq->param_hdr.length) - sizeof(*outreq)) / 2;
+		nums = (ntohs(outreq->param_hdr.length) - sizeof(*outreq)) /
+		       sizeof(__u16);
 
 		if (result = SCTP_STRRESET_PERFORMED) {
 			if (nums) {
@@ -956,7 +957,8 @@ struct sctp_chunk *sctp_process_strreset_resp(
 
 		inreq = (struct sctp_strreset_inreq *)req;
 		str_p = inreq->list_of_streams;
-		nums = (ntohs(inreq->param_hdr.length) - sizeof(*inreq)) / 2;
+		nums = (ntohs(inreq->param_hdr.length) - sizeof(*inreq)) /
+		       sizeof(__u16);
 
 		*evp = sctp_ulpevent_make_stream_reset_event(asoc, flags,
 			nums, str_p, GFP_ATOMIC);
-- 
2.1.0

[PATCH net 1/5] sctp: use sizeof(__u16) for each stream number length instead of magic number

[Xin Long]

Now in stream reconf part there are still some places using magic
number 2 for each stream number length. To make it more readable,
this patch is to replace them with sizeof(__u16).

Reported-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/stream.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/sctp/stream.c b/net/sctp/stream.c
index a11db21..09c797a 100644
--- a/net/sctp/stream.c
+++ b/net/sctp/stream.c
@@ -563,7 +563,7 @@ struct sctp_chunk *sctp_process_strreset_outreq(
 		flags = SCTP_STREAM_RESET_INCOMING_SSN;
 	}
 
-	nums = (ntohs(param.p->length) - sizeof(*outreq)) / 2;
+	nums = (ntohs(param.p->length) - sizeof(*outreq)) / sizeof(__u16);
 	if (nums) {
 		str_p = outreq->list_of_streams;
 		for (i = 0; i < nums; i++) {
@@ -627,7 +627,7 @@ struct sctp_chunk *sctp_process_strreset_inreq(
 		goto out;
 	}
 
-	nums = (ntohs(param.p->length) - sizeof(*inreq)) / 2;
+	nums = (ntohs(param.p->length) - sizeof(*inreq)) / sizeof(__u16);
 	str_p = inreq->list_of_streams;
 	for (i = 0; i < nums; i++) {
 		if (ntohs(str_p[i]) >= stream->outcnt) {
@@ -927,7 +927,8 @@ struct sctp_chunk *sctp_process_strreset_resp(
 
 		outreq = (struct sctp_strreset_outreq *)req;
 		str_p = outreq->list_of_streams;
-		nums = (ntohs(outreq->param_hdr.length) - sizeof(*outreq)) / 2;
+		nums = (ntohs(outreq->param_hdr.length) - sizeof(*outreq)) /
+		       sizeof(__u16);
 
 		if (result == SCTP_STRRESET_PERFORMED) {
 			if (nums) {
@@ -956,7 +957,8 @@ struct sctp_chunk *sctp_process_strreset_resp(
 
 		inreq = (struct sctp_strreset_inreq *)req;
 		str_p = inreq->list_of_streams;
-		nums = (ntohs(inreq->param_hdr.length) - sizeof(*inreq)) / 2;
+		nums = (ntohs(inreq->param_hdr.length) - sizeof(*inreq)) /
+		       sizeof(__u16);
 
 		*evp = sctp_ulpevent_make_stream_reset_event(asoc, flags,
 			nums, str_p, GFP_ATOMIC);
-- 
2.1.0

[PATCH net 2/5] sctp: only allow the out stream reset when the stream outq is empty

[Xin Long]

Now the out stream reset in sctp stream reconf could be done even if
the stream outq is not empty. It means that users can not be sure
since which msg the new ssn will be used.

To make this more synchronous, it shouldn't allow to do out stream
reset until these chunks in unsent outq all are sent out.

This patch checks the corresponding stream outqs when sending and
processing the request . If any of them has unsent chunks in outq,
it will return -EAGAIN instead or send SCTP_STRRESET_IN_PROGRESS
back to the sender.

Fixes: 7f9d68ac944e ("sctp: implement sender-side procedures for SSN Reset Request Parameter")
Suggested-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/stream.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/net/sctp/stream.c b/net/sctp/stream.c
index 09c797a..b209037 100644
--- a/net/sctp/stream.c
+++ b/net/sctp/stream.c
@@ -254,6 +254,30 @@ static int sctp_send_reconf(struct sctp_association *asoc,
 	return retval;
 }
 
+static bool sctp_stream_outq_is_empty(struct sctp_stream *stream,
+				      __u16 str_nums, __be16 *str_list)
+{
+	struct sctp_association *asoc;
+	__u16 i;
+
+	asoc = container_of(stream, struct sctp_association, stream);
+	if (!asoc->outqueue.out_qlen)
+		return true;
+
+	if (!str_nums)
+		return false;
+
+	for (i = 0; i < str_nums; i++) {
+		__u16 sid = ntohs(str_list[i]);
+
+		if (stream->out[sid].ext &&
+		    !list_empty(&stream->out[sid].ext->outq))
+			return false;
+	}
+
+	return true;
+}
+
 int sctp_send_reset_streams(struct sctp_association *asoc,
 			    struct sctp_reset_streams *params)
 {
@@ -317,6 +341,11 @@ int sctp_send_reset_streams(struct sctp_association *asoc,
 	for (i = 0; i < str_nums; i++)
 		nstr_list[i] = htons(str_list[i]);
 
+	if (out && !sctp_stream_outq_is_empty(stream, str_nums, nstr_list)) {
+		retval = -EAGAIN;
+		goto out;
+	}
+
 	chunk = sctp_make_strreset_req(asoc, str_nums, nstr_list, out, in);
 
 	kfree(nstr_list);
@@ -636,6 +665,12 @@ struct sctp_chunk *sctp_process_strreset_inreq(
 		}
 	}
 
+	if (!sctp_stream_outq_is_empty(stream, nums, str_p)) {
+		result = SCTP_STRRESET_IN_PROGRESS;
+		asoc->strreset_inseq--;
+		goto err;
+	}
+
 	chunk = sctp_make_strreset_req(asoc, nums, str_p, 1, 0);
 	if (!chunk)
 		goto out;
-- 
2.1.0

[PATCH net 2/5] sctp: only allow the out stream reset when the stream outq is empty

[Xin Long]

Now the out stream reset in sctp stream reconf could be done even if
the stream outq is not empty. It means that users can not be sure
since which msg the new ssn will be used.

To make this more synchronous, it shouldn't allow to do out stream
reset until these chunks in unsent outq all are sent out.

This patch checks the corresponding stream outqs when sending and
processing the request . If any of them has unsent chunks in outq,
it will return -EAGAIN instead or send SCTP_STRRESET_IN_PROGRESS
back to the sender.

Fixes: 7f9d68ac944e ("sctp: implement sender-side procedures for SSN Reset Request Parameter")
Suggested-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/stream.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/net/sctp/stream.c b/net/sctp/stream.c
index 09c797a..b209037 100644
--- a/net/sctp/stream.c
+++ b/net/sctp/stream.c
@@ -254,6 +254,30 @@ static int sctp_send_reconf(struct sctp_association *asoc,
 	return retval;
 }
 
+static bool sctp_stream_outq_is_empty(struct sctp_stream *stream,
+				      __u16 str_nums, __be16 *str_list)
+{
+	struct sctp_association *asoc;
+	__u16 i;
+
+	asoc = container_of(stream, struct sctp_association, stream);
+	if (!asoc->outqueue.out_qlen)
+		return true;
+
+	if (!str_nums)
+		return false;
+
+	for (i = 0; i < str_nums; i++) {
+		__u16 sid = ntohs(str_list[i]);
+
+		if (stream->out[sid].ext &&
+		    !list_empty(&stream->out[sid].ext->outq))
+			return false;
+	}
+
+	return true;
+}
+
 int sctp_send_reset_streams(struct sctp_association *asoc,
 			    struct sctp_reset_streams *params)
 {
@@ -317,6 +341,11 @@ int sctp_send_reset_streams(struct sctp_association *asoc,
 	for (i = 0; i < str_nums; i++)
 		nstr_list[i] = htons(str_list[i]);
 
+	if (out && !sctp_stream_outq_is_empty(stream, str_nums, nstr_list)) {
+		retval = -EAGAIN;
+		goto out;
+	}
+
 	chunk = sctp_make_strreset_req(asoc, str_nums, nstr_list, out, in);
 
 	kfree(nstr_list);
@@ -636,6 +665,12 @@ struct sctp_chunk *sctp_process_strreset_inreq(
 		}
 	}
 
+	if (!sctp_stream_outq_is_empty(stream, nums, str_p)) {
+		result = SCTP_STRRESET_IN_PROGRESS;
+		asoc->strreset_inseq--;
+		goto err;
+	}
+
 	chunk = sctp_make_strreset_req(asoc, nums, str_p, 1, 0);
 	if (!chunk)
 		goto out;
-- 
2.1.0

[PATCH net 3/5] sctp: only allow the asoc reset when the asoc outq is empty

[Xin Long]

As it says in rfc6525#section5.1.4, before sending the request,

   C2:  The sender has either no outstanding TSNs or considers all
        outstanding TSNs abandoned.

Prior to this patch, it tried to consider all outstanding TSNs abandoned
by dropping all chunks in all outqs with sctp_outq_free (even including
sacked, retransmit and transmitted queues) when doing this reset, which
is too aggressive.

To make it work gently, this patch will only allow the asoc reset when
the sender has no outstanding TSNs by checking if unsent, transmitted
and retransmit are all empty with sctp_outq_is_empty before sending
and processing the request.

Fixes: 692787cef651 ("sctp: implement receiver-side procedures for the SSN/TSN Reset Request Parameter")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/stream.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/sctp/stream.c b/net/sctp/stream.c
index b209037..f3b7d27 100644
--- a/net/sctp/stream.c
+++ b/net/sctp/stream.c
@@ -406,6 +406,9 @@ int sctp_send_reset_assoc(struct sctp_association *asoc)
 	if (asoc->strreset_outstanding)
 		return -EINPROGRESS;
 
+	if (!sctp_outq_is_empty(&asoc->outqueue))
+		return -EAGAIN;
+
 	chunk = sctp_make_strreset_tsnreq(asoc);
 	if (!chunk)
 		return -ENOMEM;
@@ -728,6 +731,12 @@ struct sctp_chunk *sctp_process_strreset_tsnreq(
 		}
 		goto err;
 	}
+
+	if (!sctp_outq_is_empty(&asoc->outqueue)) {
+		result = SCTP_STRRESET_IN_PROGRESS;
+		goto err;
+	}
+
 	asoc->strreset_inseq++;
 
 	if (!(asoc->strreset_enable & SCTP_ENABLE_RESET_ASSOC_REQ))
-- 
2.1.0

[PATCH net 3/5] sctp: only allow the asoc reset when the asoc outq is empty

[Xin Long]

As it says in rfc6525#section5.1.4, before sending the request,

   C2:  The sender has either no outstanding TSNs or considers all
        outstanding TSNs abandoned.

Prior to this patch, it tried to consider all outstanding TSNs abandoned
by dropping all chunks in all outqs with sctp_outq_free (even including
sacked, retransmit and transmitted queues) when doing this reset, which
is too aggressive.

To make it work gently, this patch will only allow the asoc reset when
the sender has no outstanding TSNs by checking if unsent, transmitted
and retransmit are all empty with sctp_outq_is_empty before sending
and processing the request.

Fixes: 692787cef651 ("sctp: implement receiver-side procedures for the SSN/TSN Reset Request Parameter")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/stream.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/sctp/stream.c b/net/sctp/stream.c
index b209037..f3b7d27 100644
--- a/net/sctp/stream.c
+++ b/net/sctp/stream.c
@@ -406,6 +406,9 @@ int sctp_send_reset_assoc(struct sctp_association *asoc)
 	if (asoc->strreset_outstanding)
 		return -EINPROGRESS;
 
+	if (!sctp_outq_is_empty(&asoc->outqueue))
+		return -EAGAIN;
+
 	chunk = sctp_make_strreset_tsnreq(asoc);
 	if (!chunk)
 		return -ENOMEM;
@@ -728,6 +731,12 @@ struct sctp_chunk *sctp_process_strreset_tsnreq(
 		}
 		goto err;
 	}
+
+	if (!sctp_outq_is_empty(&asoc->outqueue)) {
+		result = SCTP_STRRESET_IN_PROGRESS;
+		goto err;
+	}
+
 	asoc->strreset_inseq++;
 
 	if (!(asoc->strreset_enable & SCTP_ENABLE_RESET_ASSOC_REQ))
-- 
2.1.0

[PATCH net 4/5] sctp: avoid flushing unsent queue when doing asoc reset

[Xin Long]

Now when doing asoc reset, it cleans up sacked and abandoned queues
by calling sctp_outq_free where it also cleans up unsent, retransmit
and transmitted queues.

It's safe for the sender of response, as these 3 queues are empty at
that time. But when the receiver of response is doing the reset, the
users may already enqueue some chunks into unsent during the time
waiting the response, and these chunks should not be flushed.

To void the chunks in it would be removed, it moves the queue into a
temp list, then gets it back after sctp_outq_free is done.

The patch also fixes some incorrect comments in
sctp_process_strreset_tsnreq.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/stream.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/net/sctp/stream.c b/net/sctp/stream.c
index f3b7d27..9dd5bfe 100644
--- a/net/sctp/stream.c
+++ b/net/sctp/stream.c
@@ -747,9 +747,10 @@ struct sctp_chunk *sctp_process_strreset_tsnreq(
 		goto out;
 	}
 
-	/* G3: The same processing as though a SACK chunk with no gap report
-	 *     and a cumulative TSN ACK of the Sender's Next TSN minus 1 were
-	 *     received MUST be performed.
+	/* G4: The same processing as though a FWD-TSN chunk (as defined in
+	 *     [RFC3758]) with all streams affected and a new cumulative TSN
+	 *     ACK of the Receiver's Next TSN minus 1 were received MUST be
+	 *     performed.
 	 */
 	max_tsn_seen = sctp_tsnmap_get_max_tsn_seen(&asoc->peer.tsn_map);
 	sctp_ulpq_reasm_flushtsn(&asoc->ulpq, max_tsn_seen);
@@ -764,10 +765,9 @@ struct sctp_chunk *sctp_process_strreset_tsnreq(
 	sctp_tsnmap_init(&asoc->peer.tsn_map, SCTP_TSN_MAP_INITIAL,
 			 init_tsn, GFP_ATOMIC);
 
-	/* G4: The same processing as though a FWD-TSN chunk (as defined in
-	 *     [RFC3758]) with all streams affected and a new cumulative TSN
-	 *     ACK of the Receiver's Next TSN minus 1 were received MUST be
-	 *     performed.
+	/* G3: The same processing as though a SACK chunk with no gap report
+	 *     and a cumulative TSN ACK of the Sender's Next TSN minus 1 were
+	 *     received MUST be performed.
 	 */
 	sctp_outq_free(&asoc->outqueue);
 
@@ -1021,6 +1021,7 @@ struct sctp_chunk *sctp_process_strreset_resp(
 		if (result = SCTP_STRRESET_PERFORMED) {
 			__u32 mtsn = sctp_tsnmap_get_max_tsn_seen(
 						&asoc->peer.tsn_map);
+			LIST_HEAD(temp);
 
 			sctp_ulpq_reasm_flushtsn(&asoc->ulpq, mtsn);
 			sctp_ulpq_abort_pd(&asoc->ulpq, GFP_ATOMIC);
@@ -1029,7 +1030,13 @@ struct sctp_chunk *sctp_process_strreset_resp(
 					 SCTP_TSN_MAP_INITIAL,
 					 stsn, GFP_ATOMIC);
 
+			/* Clean up sacked and abandoned queues only. As the
+			 * out_chunk_list may not be empty, splice it to temp,
+			 * then get it back after sctp_outq_free is done.
+			 */
+			list_splice_init(&asoc->outqueue.out_chunk_list, &temp);
 			sctp_outq_free(&asoc->outqueue);
+			list_splice_init(&temp, &asoc->outqueue.out_chunk_list);
 
 			asoc->next_tsn = rtsn;
 			asoc->ctsn_ack_point = asoc->next_tsn - 1;
-- 
2.1.0

[PATCH net 4/5] sctp: avoid flushing unsent queue when doing asoc reset

[Xin Long]

Now when doing asoc reset, it cleans up sacked and abandoned queues
by calling sctp_outq_free where it also cleans up unsent, retransmit
and transmitted queues.

It's safe for the sender of response, as these 3 queues are empty at
that time. But when the receiver of response is doing the reset, the
users may already enqueue some chunks into unsent during the time
waiting the response, and these chunks should not be flushed.

To void the chunks in it would be removed, it moves the queue into a
temp list, then gets it back after sctp_outq_free is done.

The patch also fixes some incorrect comments in
sctp_process_strreset_tsnreq.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/stream.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/net/sctp/stream.c b/net/sctp/stream.c
index f3b7d27..9dd5bfe 100644
--- a/net/sctp/stream.c
+++ b/net/sctp/stream.c
@@ -747,9 +747,10 @@ struct sctp_chunk *sctp_process_strreset_tsnreq(
 		goto out;
 	}
 
-	/* G3: The same processing as though a SACK chunk with no gap report
-	 *     and a cumulative TSN ACK of the Sender's Next TSN minus 1 were
-	 *     received MUST be performed.
+	/* G4: The same processing as though a FWD-TSN chunk (as defined in
+	 *     [RFC3758]) with all streams affected and a new cumulative TSN
+	 *     ACK of the Receiver's Next TSN minus 1 were received MUST be
+	 *     performed.
 	 */
 	max_tsn_seen = sctp_tsnmap_get_max_tsn_seen(&asoc->peer.tsn_map);
 	sctp_ulpq_reasm_flushtsn(&asoc->ulpq, max_tsn_seen);
@@ -764,10 +765,9 @@ struct sctp_chunk *sctp_process_strreset_tsnreq(
 	sctp_tsnmap_init(&asoc->peer.tsn_map, SCTP_TSN_MAP_INITIAL,
 			 init_tsn, GFP_ATOMIC);
 
-	/* G4: The same processing as though a FWD-TSN chunk (as defined in
-	 *     [RFC3758]) with all streams affected and a new cumulative TSN
-	 *     ACK of the Receiver's Next TSN minus 1 were received MUST be
-	 *     performed.
+	/* G3: The same processing as though a SACK chunk with no gap report
+	 *     and a cumulative TSN ACK of the Sender's Next TSN minus 1 were
+	 *     received MUST be performed.
 	 */
 	sctp_outq_free(&asoc->outqueue);
 
@@ -1021,6 +1021,7 @@ struct sctp_chunk *sctp_process_strreset_resp(
 		if (result == SCTP_STRRESET_PERFORMED) {
 			__u32 mtsn = sctp_tsnmap_get_max_tsn_seen(
 						&asoc->peer.tsn_map);
+			LIST_HEAD(temp);
 
 			sctp_ulpq_reasm_flushtsn(&asoc->ulpq, mtsn);
 			sctp_ulpq_abort_pd(&asoc->ulpq, GFP_ATOMIC);
@@ -1029,7 +1030,13 @@ struct sctp_chunk *sctp_process_strreset_resp(
 					 SCTP_TSN_MAP_INITIAL,
 					 stsn, GFP_ATOMIC);
 
+			/* Clean up sacked and abandoned queues only. As the
+			 * out_chunk_list may not be empty, splice it to temp,
+			 * then get it back after sctp_outq_free is done.
+			 */
+			list_splice_init(&asoc->outqueue.out_chunk_list, &temp);
 			sctp_outq_free(&asoc->outqueue);
+			list_splice_init(&temp, &asoc->outqueue.out_chunk_list);
 
 			asoc->next_tsn = rtsn;
 			asoc->ctsn_ack_point = asoc->next_tsn - 1;
-- 
2.1.0

[PATCH net 5/5] sctp: set sender next_tsn for the old result with ctsn_ack_point plus 1

[Xin Long]

When doing asoc reset, if the sender of the response has already sent some
chunk and increased asoc->next_tsn before the duplicate request comes, the
response will use the old result with an incorrect sender next_tsn.

Better than asoc->next_tsn, asoc->ctsn_ack_point can't be changed after
the sender of the response has performed the asoc reset and before the
peer has confirmed it, and it's value is still asoc->next_tsn original
value minus 1.

This patch sets sender next_tsn for the old result with ctsn_ack_point
plus 1 when processing the duplicate request, to make sure the sender
next_tsn value peer gets will be always right.

Fixes: 692787cef651 ("sctp: implement receiver-side procedures for the SSN/TSN Reset Request Parameter")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/stream.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sctp/stream.c b/net/sctp/stream.c
index 9dd5bfe..a20145b 100644
--- a/net/sctp/stream.c
+++ b/net/sctp/stream.c
@@ -725,7 +725,7 @@ struct sctp_chunk *sctp_process_strreset_tsnreq(
 		i = asoc->strreset_inseq - request_seq - 1;
 		result = asoc->strreset_result[i];
 		if (result = SCTP_STRRESET_PERFORMED) {
-			next_tsn = asoc->next_tsn;
+			next_tsn = asoc->ctsn_ack_point + 1;
 			init_tsn  				sctp_tsnmap_get_ctsn(&asoc->peer.tsn_map) + 1;
 		}
-- 
2.1.0

[PATCH net 5/5] sctp: set sender next_tsn for the old result with ctsn_ack_point plus 1

[Xin Long]

When doing asoc reset, if the sender of the response has already sent some
chunk and increased asoc->next_tsn before the duplicate request comes, the
response will use the old result with an incorrect sender next_tsn.

Better than asoc->next_tsn, asoc->ctsn_ack_point can't be changed after
the sender of the response has performed the asoc reset and before the
peer has confirmed it, and it's value is still asoc->next_tsn original
value minus 1.

This patch sets sender next_tsn for the old result with ctsn_ack_point
plus 1 when processing the duplicate request, to make sure the sender
next_tsn value peer gets will be always right.

Fixes: 692787cef651 ("sctp: implement receiver-side procedures for the SSN/TSN Reset Request Parameter")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/stream.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sctp/stream.c b/net/sctp/stream.c
index 9dd5bfe..a20145b 100644
--- a/net/sctp/stream.c
+++ b/net/sctp/stream.c
@@ -725,7 +725,7 @@ struct sctp_chunk *sctp_process_strreset_tsnreq(
 		i = asoc->strreset_inseq - request_seq - 1;
 		result = asoc->strreset_result[i];
 		if (result == SCTP_STRRESET_PERFORMED) {
-			next_tsn = asoc->next_tsn;
+			next_tsn = asoc->ctsn_ack_point + 1;
 			init_tsn =
 				sctp_tsnmap_get_ctsn(&asoc->peer.tsn_map) + 1;
 		}
-- 
2.1.0

Re: [PATCH net 5/5] sctp: set sender next_tsn for the old result with ctsn_ack_point plus 1

[Marcelo Ricardo Leitner]

On Sat, Nov 25, 2017 at 09:05:36PM +0800, Xin Long wrote:
> When doing asoc reset, if the sender of the response has already sent some
> chunk and increased asoc->next_tsn before the duplicate request comes, the
> response will use the old result with an incorrect sender next_tsn.
> 
> Better than asoc->next_tsn, asoc->ctsn_ack_point can't be changed after
> the sender of the response has performed the asoc reset and before the
> peer has confirmed it, and it's value is still asoc->next_tsn original
> value minus 1.
> 
> This patch sets sender next_tsn for the old result with ctsn_ack_point
> plus 1 when processing the duplicate request, to make sure the sender
> next_tsn value peer gets will be always right.
> 
> Fixes: 692787cef651 ("sctp: implement receiver-side procedures for the SSN/TSN Reset Request Parameter")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> ---
>  net/sctp/stream.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> index 9dd5bfe..a20145b 100644
> --- a/net/sctp/stream.c
> +++ b/net/sctp/stream.c
> @@ -725,7 +725,7 @@ struct sctp_chunk *sctp_process_strreset_tsnreq(
>  		i = asoc->strreset_inseq - request_seq - 1;
>  		result = asoc->strreset_result[i];
>  		if (result = SCTP_STRRESET_PERFORMED) {
> -			next_tsn = asoc->next_tsn;
> +			next_tsn = asoc->ctsn_ack_point + 1;
>  			init_tsn >  				sctp_tsnmap_get_ctsn(&asoc->peer.tsn_map) + 1;
>  		}
> -- 
> 2.1.0
> 
> --
> 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 net 5/5] sctp: set sender next_tsn for the old result with ctsn_ack_point plus 1

[Marcelo Ricardo Leitner]

On Sat, Nov 25, 2017 at 09:05:36PM +0800, Xin Long wrote:
> When doing asoc reset, if the sender of the response has already sent some
> chunk and increased asoc->next_tsn before the duplicate request comes, the
> response will use the old result with an incorrect sender next_tsn.
> 
> Better than asoc->next_tsn, asoc->ctsn_ack_point can't be changed after
> the sender of the response has performed the asoc reset and before the
> peer has confirmed it, and it's value is still asoc->next_tsn original
> value minus 1.
> 
> This patch sets sender next_tsn for the old result with ctsn_ack_point
> plus 1 when processing the duplicate request, to make sure the sender
> next_tsn value peer gets will be always right.
> 
> Fixes: 692787cef651 ("sctp: implement receiver-side procedures for the SSN/TSN Reset Request Parameter")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> ---
>  net/sctp/stream.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> index 9dd5bfe..a20145b 100644
> --- a/net/sctp/stream.c
> +++ b/net/sctp/stream.c
> @@ -725,7 +725,7 @@ struct sctp_chunk *sctp_process_strreset_tsnreq(
>  		i = asoc->strreset_inseq - request_seq - 1;
>  		result = asoc->strreset_result[i];
>  		if (result == SCTP_STRRESET_PERFORMED) {
> -			next_tsn = asoc->next_tsn;
> +			next_tsn = asoc->ctsn_ack_point + 1;
>  			init_tsn =
>  				sctp_tsnmap_get_ctsn(&asoc->peer.tsn_map) + 1;
>  		}
> -- 
> 2.1.0
> 
> --
> 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 net 4/5] sctp: avoid flushing unsent queue when doing asoc reset

[Marcelo Ricardo Leitner]

On Sat, Nov 25, 2017 at 09:05:35PM +0800, Xin Long wrote:
> Now when doing asoc reset, it cleans up sacked and abandoned queues
> by calling sctp_outq_free where it also cleans up unsent, retransmit
> and transmitted queues.
> 
> It's safe for the sender of response, as these 3 queues are empty at
> that time. But when the receiver of response is doing the reset, the
> users may already enqueue some chunks into unsent during the time
> waiting the response, and these chunks should not be flushed.
> 
> To void the chunks in it would be removed, it moves the queue into a
> temp list, then gets it back after sctp_outq_free is done.
> 
> The patch also fixes some incorrect comments in
> sctp_process_strreset_tsnreq.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> ---
>  net/sctp/stream.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> index f3b7d27..9dd5bfe 100644
> --- a/net/sctp/stream.c
> +++ b/net/sctp/stream.c
> @@ -747,9 +747,10 @@ struct sctp_chunk *sctp_process_strreset_tsnreq(
>  		goto out;
>  	}
>  
> -	/* G3: The same processing as though a SACK chunk with no gap report
> -	 *     and a cumulative TSN ACK of the Sender's Next TSN minus 1 were
> -	 *     received MUST be performed.
> +	/* G4: The same processing as though a FWD-TSN chunk (as defined in
> +	 *     [RFC3758]) with all streams affected and a new cumulative TSN
> +	 *     ACK of the Receiver's Next TSN minus 1 were received MUST be
> +	 *     performed.
>  	 */
>  	max_tsn_seen = sctp_tsnmap_get_max_tsn_seen(&asoc->peer.tsn_map);
>  	sctp_ulpq_reasm_flushtsn(&asoc->ulpq, max_tsn_seen);
> @@ -764,10 +765,9 @@ struct sctp_chunk *sctp_process_strreset_tsnreq(
>  	sctp_tsnmap_init(&asoc->peer.tsn_map, SCTP_TSN_MAP_INITIAL,
>  			 init_tsn, GFP_ATOMIC);
>  
> -	/* G4: The same processing as though a FWD-TSN chunk (as defined in
> -	 *     [RFC3758]) with all streams affected and a new cumulative TSN
> -	 *     ACK of the Receiver's Next TSN minus 1 were received MUST be
> -	 *     performed.
> +	/* G3: The same processing as though a SACK chunk with no gap report
> +	 *     and a cumulative TSN ACK of the Sender's Next TSN minus 1 were
> +	 *     received MUST be performed.
>  	 */
>  	sctp_outq_free(&asoc->outqueue);
>  
> @@ -1021,6 +1021,7 @@ struct sctp_chunk *sctp_process_strreset_resp(
>  		if (result = SCTP_STRRESET_PERFORMED) {
>  			__u32 mtsn = sctp_tsnmap_get_max_tsn_seen(
>  						&asoc->peer.tsn_map);
> +			LIST_HEAD(temp);
>  
>  			sctp_ulpq_reasm_flushtsn(&asoc->ulpq, mtsn);
>  			sctp_ulpq_abort_pd(&asoc->ulpq, GFP_ATOMIC);
> @@ -1029,7 +1030,13 @@ struct sctp_chunk *sctp_process_strreset_resp(
>  					 SCTP_TSN_MAP_INITIAL,
>  					 stsn, GFP_ATOMIC);
>  
> +			/* Clean up sacked and abandoned queues only. As the
> +			 * out_chunk_list may not be empty, splice it to temp,
> +			 * then get it back after sctp_outq_free is done.
> +			 */
> +			list_splice_init(&asoc->outqueue.out_chunk_list, &temp);
>  			sctp_outq_free(&asoc->outqueue);
> +			list_splice_init(&temp, &asoc->outqueue.out_chunk_list);
>  
>  			asoc->next_tsn = rtsn;
>  			asoc->ctsn_ack_point = asoc->next_tsn - 1;
> -- 
> 2.1.0
> 
> --
> 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 net 4/5] sctp: avoid flushing unsent queue when doing asoc reset

[Marcelo Ricardo Leitner]

On Sat, Nov 25, 2017 at 09:05:35PM +0800, Xin Long wrote:
> Now when doing asoc reset, it cleans up sacked and abandoned queues
> by calling sctp_outq_free where it also cleans up unsent, retransmit
> and transmitted queues.
> 
> It's safe for the sender of response, as these 3 queues are empty at
> that time. But when the receiver of response is doing the reset, the
> users may already enqueue some chunks into unsent during the time
> waiting the response, and these chunks should not be flushed.
> 
> To void the chunks in it would be removed, it moves the queue into a
> temp list, then gets it back after sctp_outq_free is done.
> 
> The patch also fixes some incorrect comments in
> sctp_process_strreset_tsnreq.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> ---
>  net/sctp/stream.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> index f3b7d27..9dd5bfe 100644
> --- a/net/sctp/stream.c
> +++ b/net/sctp/stream.c
> @@ -747,9 +747,10 @@ struct sctp_chunk *sctp_process_strreset_tsnreq(
>  		goto out;
>  	}
>  
> -	/* G3: The same processing as though a SACK chunk with no gap report
> -	 *     and a cumulative TSN ACK of the Sender's Next TSN minus 1 were
> -	 *     received MUST be performed.
> +	/* G4: The same processing as though a FWD-TSN chunk (as defined in
> +	 *     [RFC3758]) with all streams affected and a new cumulative TSN
> +	 *     ACK of the Receiver's Next TSN minus 1 were received MUST be
> +	 *     performed.
>  	 */
>  	max_tsn_seen = sctp_tsnmap_get_max_tsn_seen(&asoc->peer.tsn_map);
>  	sctp_ulpq_reasm_flushtsn(&asoc->ulpq, max_tsn_seen);
> @@ -764,10 +765,9 @@ struct sctp_chunk *sctp_process_strreset_tsnreq(
>  	sctp_tsnmap_init(&asoc->peer.tsn_map, SCTP_TSN_MAP_INITIAL,
>  			 init_tsn, GFP_ATOMIC);
>  
> -	/* G4: The same processing as though a FWD-TSN chunk (as defined in
> -	 *     [RFC3758]) with all streams affected and a new cumulative TSN
> -	 *     ACK of the Receiver's Next TSN minus 1 were received MUST be
> -	 *     performed.
> +	/* G3: The same processing as though a SACK chunk with no gap report
> +	 *     and a cumulative TSN ACK of the Sender's Next TSN minus 1 were
> +	 *     received MUST be performed.
>  	 */
>  	sctp_outq_free(&asoc->outqueue);
>  
> @@ -1021,6 +1021,7 @@ struct sctp_chunk *sctp_process_strreset_resp(
>  		if (result == SCTP_STRRESET_PERFORMED) {
>  			__u32 mtsn = sctp_tsnmap_get_max_tsn_seen(
>  						&asoc->peer.tsn_map);
> +			LIST_HEAD(temp);
>  
>  			sctp_ulpq_reasm_flushtsn(&asoc->ulpq, mtsn);
>  			sctp_ulpq_abort_pd(&asoc->ulpq, GFP_ATOMIC);
> @@ -1029,7 +1030,13 @@ struct sctp_chunk *sctp_process_strreset_resp(
>  					 SCTP_TSN_MAP_INITIAL,
>  					 stsn, GFP_ATOMIC);
>  
> +			/* Clean up sacked and abandoned queues only. As the
> +			 * out_chunk_list may not be empty, splice it to temp,
> +			 * then get it back after sctp_outq_free is done.
> +			 */
> +			list_splice_init(&asoc->outqueue.out_chunk_list, &temp);
>  			sctp_outq_free(&asoc->outqueue);
> +			list_splice_init(&temp, &asoc->outqueue.out_chunk_list);
>  
>  			asoc->next_tsn = rtsn;
>  			asoc->ctsn_ack_point = asoc->next_tsn - 1;
> -- 
> 2.1.0
> 
> --
> 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 net 3/5] sctp: only allow the asoc reset when the asoc outq is empty

[David Laight]

From: Xin Long
> Sent: 25 November 2017 13:06
> 
> As it says in rfc6525#section5.1.4, before sending the request,
> 
>    C2:  The sender has either no outstanding TSNs or considers all
>         outstanding TSNs abandoned.
> 
> Prior to this patch, it tried to consider all outstanding TSNs abandoned
> by dropping all chunks in all outqs with sctp_outq_free (even including
> sacked, retransmit and transmitted queues) when doing this reset, which
> is too aggressive.
> 
> To make it work gently, this patch will only allow the asoc reset when
> the sender has no outstanding TSNs by checking if unsent, transmitted
> and retransmit are all empty with sctp_outq_is_empty before sending
> and processing the request.

Doesn't that rather defeat the point of a reset?

	David

Re: [PATCH net 3/5] sctp: only allow the asoc reset when the asoc outq is empty

[Xin Long]

On Mon, Nov 27, 2017 at 6:06 PM, David Laight <David.Laight@aculab.com> wrote:
> From: Xin Long
>> Sent: 25 November 2017 13:06
>>
>> As it says in rfc6525#section5.1.4, before sending the request,
>>
>>    C2:  The sender has either no outstanding TSNs or considers all
>>         outstanding TSNs abandoned.
>>
>> Prior to this patch, it tried to consider all outstanding TSNs abandoned
>> by dropping all chunks in all outqs with sctp_outq_free (even including
>> sacked, retransmit and transmitted queues) when doing this reset, which
>> is too aggressive.
>>
>> To make it work gently, this patch will only allow the asoc reset when
>> the sender has no outstanding TSNs by checking if unsent, transmitted
>> and retransmit are all empty with sctp_outq_is_empty before sending
>> and processing the request.
>
> Doesn't that rather defeat the point of a reset?
No, reset here means to reset the next tsn/ssn, not with the cost of
losing outstanding data, though RFC6525 allows to consider them
abandoned.

Re: [PATCH net 3/5] sctp: only allow the asoc reset when the asoc outq is empty

[Xin Long]

On Mon, Nov 27, 2017 at 6:06 PM, David Laight <David.Laight@aculab.com> wrote:
> From: Xin Long
>> Sent: 25 November 2017 13:06
>>
>> As it says in rfc6525#section5.1.4, before sending the request,
>>
>>    C2:  The sender has either no outstanding TSNs or considers all
>>         outstanding TSNs abandoned.
>>
>> Prior to this patch, it tried to consider all outstanding TSNs abandoned
>> by dropping all chunks in all outqs with sctp_outq_free (even including
>> sacked, retransmit and transmitted queues) when doing this reset, which
>> is too aggressive.
>>
>> To make it work gently, this patch will only allow the asoc reset when
>> the sender has no outstanding TSNs by checking if unsent, transmitted
>> and retransmit are all empty with sctp_outq_is_empty before sending
>> and processing the request.
>
> Doesn't that rather defeat the point of a reset?
No, reset here means to reset the next tsn/ssn, not with the cost of
losing outstanding data, though RFC6525 allows to consider them
abandoned.

Re: [PATCH net 3/5] sctp: only allow the asoc reset when the asoc outq is empty

[Marcelo Ricardo Leitner]

On Sat, Nov 25, 2017 at 09:05:34PM +0800, Xin Long wrote:
> As it says in rfc6525#section5.1.4, before sending the request,
> 
>    C2:  The sender has either no outstanding TSNs or considers all
>         outstanding TSNs abandoned.
> 
> Prior to this patch, it tried to consider all outstanding TSNs abandoned
> by dropping all chunks in all outqs with sctp_outq_free (even including
> sacked, retransmit and transmitted queues) when doing this reset, which
> is too aggressive.
> 
> To make it work gently, this patch will only allow the asoc reset when
> the sender has no outstanding TSNs by checking if unsent, transmitted
> and retransmit are all empty with sctp_outq_is_empty before sending
> and processing the request.
> 
> Fixes: 692787cef651 ("sctp: implement receiver-side procedures for the SSN/TSN Reset Request Parameter")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> ---
>  net/sctp/stream.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> index b209037..f3b7d27 100644
> --- a/net/sctp/stream.c
> +++ b/net/sctp/stream.c
> @@ -406,6 +406,9 @@ int sctp_send_reset_assoc(struct sctp_association *asoc)
>  	if (asoc->strreset_outstanding)
>  		return -EINPROGRESS;
>  
> +	if (!sctp_outq_is_empty(&asoc->outqueue))
> +		return -EAGAIN;
> +
>  	chunk = sctp_make_strreset_tsnreq(asoc);
>  	if (!chunk)
>  		return -ENOMEM;
> @@ -728,6 +731,12 @@ struct sctp_chunk *sctp_process_strreset_tsnreq(
>  		}
>  		goto err;
>  	}
> +
> +	if (!sctp_outq_is_empty(&asoc->outqueue)) {
> +		result = SCTP_STRRESET_IN_PROGRESS;
> +		goto err;
> +	}
> +
>  	asoc->strreset_inseq++;
>  
>  	if (!(asoc->strreset_enable & SCTP_ENABLE_RESET_ASSOC_REQ))
> -- 
> 2.1.0
> 
> --
> 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 net 3/5] sctp: only allow the asoc reset when the asoc outq is empty

[Marcelo Ricardo Leitner]

On Sat, Nov 25, 2017 at 09:05:34PM +0800, Xin Long wrote:
> As it says in rfc6525#section5.1.4, before sending the request,
> 
>    C2:  The sender has either no outstanding TSNs or considers all
>         outstanding TSNs abandoned.
> 
> Prior to this patch, it tried to consider all outstanding TSNs abandoned
> by dropping all chunks in all outqs with sctp_outq_free (even including
> sacked, retransmit and transmitted queues) when doing this reset, which
> is too aggressive.
> 
> To make it work gently, this patch will only allow the asoc reset when
> the sender has no outstanding TSNs by checking if unsent, transmitted
> and retransmit are all empty with sctp_outq_is_empty before sending
> and processing the request.
> 
> Fixes: 692787cef651 ("sctp: implement receiver-side procedures for the SSN/TSN Reset Request Parameter")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> ---
>  net/sctp/stream.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> index b209037..f3b7d27 100644
> --- a/net/sctp/stream.c
> +++ b/net/sctp/stream.c
> @@ -406,6 +406,9 @@ int sctp_send_reset_assoc(struct sctp_association *asoc)
>  	if (asoc->strreset_outstanding)
>  		return -EINPROGRESS;
>  
> +	if (!sctp_outq_is_empty(&asoc->outqueue))
> +		return -EAGAIN;
> +
>  	chunk = sctp_make_strreset_tsnreq(asoc);
>  	if (!chunk)
>  		return -ENOMEM;
> @@ -728,6 +731,12 @@ struct sctp_chunk *sctp_process_strreset_tsnreq(
>  		}
>  		goto err;
>  	}
> +
> +	if (!sctp_outq_is_empty(&asoc->outqueue)) {
> +		result = SCTP_STRRESET_IN_PROGRESS;
> +		goto err;
> +	}
> +
>  	asoc->strreset_inseq++;
>  
>  	if (!(asoc->strreset_enable & SCTP_ENABLE_RESET_ASSOC_REQ))
> -- 
> 2.1.0
> 
> --
> 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 net 2/5] sctp: only allow the out stream reset when the stream outq is empty

[Marcelo Ricardo Leitner]

On Sat, Nov 25, 2017 at 09:05:33PM +0800, Xin Long wrote:
> Now the out stream reset in sctp stream reconf could be done even if
> the stream outq is not empty. It means that users can not be sure
> since which msg the new ssn will be used.
> 
> To make this more synchronous, it shouldn't allow to do out stream
> reset until these chunks in unsent outq all are sent out.
> 
> This patch checks the corresponding stream outqs when sending and
> processing the request . If any of them has unsent chunks in outq,
> it will return -EAGAIN instead or send SCTP_STRRESET_IN_PROGRESS
> back to the sender.
> 
> Fixes: 7f9d68ac944e ("sctp: implement sender-side procedures for SSN Reset Request Parameter")
> Suggested-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> ---
>  net/sctp/stream.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> index 09c797a..b209037 100644
> --- a/net/sctp/stream.c
> +++ b/net/sctp/stream.c
> @@ -254,6 +254,30 @@ static int sctp_send_reconf(struct sctp_association *asoc,
>  	return retval;
>  }
>  
> +static bool sctp_stream_outq_is_empty(struct sctp_stream *stream,
> +				      __u16 str_nums, __be16 *str_list)
> +{
> +	struct sctp_association *asoc;
> +	__u16 i;
> +
> +	asoc = container_of(stream, struct sctp_association, stream);
> +	if (!asoc->outqueue.out_qlen)
> +		return true;
> +
> +	if (!str_nums)
> +		return false;
> +
> +	for (i = 0; i < str_nums; i++) {
> +		__u16 sid = ntohs(str_list[i]);
> +
> +		if (stream->out[sid].ext &&
> +		    !list_empty(&stream->out[sid].ext->outq))
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>  int sctp_send_reset_streams(struct sctp_association *asoc,
>  			    struct sctp_reset_streams *params)
>  {
> @@ -317,6 +341,11 @@ int sctp_send_reset_streams(struct sctp_association *asoc,
>  	for (i = 0; i < str_nums; i++)
>  		nstr_list[i] = htons(str_list[i]);
>  
> +	if (out && !sctp_stream_outq_is_empty(stream, str_nums, nstr_list)) {
> +		retval = -EAGAIN;
> +		goto out;
> +	}
> +
>  	chunk = sctp_make_strreset_req(asoc, str_nums, nstr_list, out, in);
>  
>  	kfree(nstr_list);
> @@ -636,6 +665,12 @@ struct sctp_chunk *sctp_process_strreset_inreq(
>  		}
>  	}
>  
> +	if (!sctp_stream_outq_is_empty(stream, nums, str_p)) {
> +		result = SCTP_STRRESET_IN_PROGRESS;
> +		asoc->strreset_inseq--;
> +		goto err;
> +	}
> +
>  	chunk = sctp_make_strreset_req(asoc, nums, str_p, 1, 0);
>  	if (!chunk)
>  		goto out;
> -- 
> 2.1.0
> 
> --
> 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 net 2/5] sctp: only allow the out stream reset when the stream outq is empty

[Marcelo Ricardo Leitner]

On Sat, Nov 25, 2017 at 09:05:33PM +0800, Xin Long wrote:
> Now the out stream reset in sctp stream reconf could be done even if
> the stream outq is not empty. It means that users can not be sure
> since which msg the new ssn will be used.
> 
> To make this more synchronous, it shouldn't allow to do out stream
> reset until these chunks in unsent outq all are sent out.
> 
> This patch checks the corresponding stream outqs when sending and
> processing the request . If any of them has unsent chunks in outq,
> it will return -EAGAIN instead or send SCTP_STRRESET_IN_PROGRESS
> back to the sender.
> 
> Fixes: 7f9d68ac944e ("sctp: implement sender-side procedures for SSN Reset Request Parameter")
> Suggested-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> ---
>  net/sctp/stream.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> index 09c797a..b209037 100644
> --- a/net/sctp/stream.c
> +++ b/net/sctp/stream.c
> @@ -254,6 +254,30 @@ static int sctp_send_reconf(struct sctp_association *asoc,
>  	return retval;
>  }
>  
> +static bool sctp_stream_outq_is_empty(struct sctp_stream *stream,
> +				      __u16 str_nums, __be16 *str_list)
> +{
> +	struct sctp_association *asoc;
> +	__u16 i;
> +
> +	asoc = container_of(stream, struct sctp_association, stream);
> +	if (!asoc->outqueue.out_qlen)
> +		return true;
> +
> +	if (!str_nums)
> +		return false;
> +
> +	for (i = 0; i < str_nums; i++) {
> +		__u16 sid = ntohs(str_list[i]);
> +
> +		if (stream->out[sid].ext &&
> +		    !list_empty(&stream->out[sid].ext->outq))
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>  int sctp_send_reset_streams(struct sctp_association *asoc,
>  			    struct sctp_reset_streams *params)
>  {
> @@ -317,6 +341,11 @@ int sctp_send_reset_streams(struct sctp_association *asoc,
>  	for (i = 0; i < str_nums; i++)
>  		nstr_list[i] = htons(str_list[i]);
>  
> +	if (out && !sctp_stream_outq_is_empty(stream, str_nums, nstr_list)) {
> +		retval = -EAGAIN;
> +		goto out;
> +	}
> +
>  	chunk = sctp_make_strreset_req(asoc, str_nums, nstr_list, out, in);
>  
>  	kfree(nstr_list);
> @@ -636,6 +665,12 @@ struct sctp_chunk *sctp_process_strreset_inreq(
>  		}
>  	}
>  
> +	if (!sctp_stream_outq_is_empty(stream, nums, str_p)) {
> +		result = SCTP_STRRESET_IN_PROGRESS;
> +		asoc->strreset_inseq--;
> +		goto err;
> +	}
> +
>  	chunk = sctp_make_strreset_req(asoc, nums, str_p, 1, 0);
>  	if (!chunk)
>  		goto out;
> -- 
> 2.1.0
> 
> --
> 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 net 1/5] sctp: use sizeof(__u16) for each stream number length instead of magic number

[Marcelo Ricardo Leitner]

On Sat, Nov 25, 2017 at 09:05:32PM +0800, Xin Long wrote:
> Now in stream reconf part there are still some places using magic
> number 2 for each stream number length. To make it more readable,
> this patch is to replace them with sizeof(__u16).
> 
> Reported-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> ---
>  net/sctp/stream.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> index a11db21..09c797a 100644
> --- a/net/sctp/stream.c
> +++ b/net/sctp/stream.c
> @@ -563,7 +563,7 @@ struct sctp_chunk *sctp_process_strreset_outreq(
>  		flags = SCTP_STREAM_RESET_INCOMING_SSN;
>  	}
>  
> -	nums = (ntohs(param.p->length) - sizeof(*outreq)) / 2;
> +	nums = (ntohs(param.p->length) - sizeof(*outreq)) / sizeof(__u16);
>  	if (nums) {
>  		str_p = outreq->list_of_streams;
>  		for (i = 0; i < nums; i++) {
> @@ -627,7 +627,7 @@ struct sctp_chunk *sctp_process_strreset_inreq(
>  		goto out;
>  	}
>  
> -	nums = (ntohs(param.p->length) - sizeof(*inreq)) / 2;
> +	nums = (ntohs(param.p->length) - sizeof(*inreq)) / sizeof(__u16);
>  	str_p = inreq->list_of_streams;
>  	for (i = 0; i < nums; i++) {
>  		if (ntohs(str_p[i]) >= stream->outcnt) {
> @@ -927,7 +927,8 @@ struct sctp_chunk *sctp_process_strreset_resp(
>  
>  		outreq = (struct sctp_strreset_outreq *)req;
>  		str_p = outreq->list_of_streams;
> -		nums = (ntohs(outreq->param_hdr.length) - sizeof(*outreq)) / 2;
> +		nums = (ntohs(outreq->param_hdr.length) - sizeof(*outreq)) /
> +		       sizeof(__u16);
>  
>  		if (result = SCTP_STRRESET_PERFORMED) {
>  			if (nums) {
> @@ -956,7 +957,8 @@ struct sctp_chunk *sctp_process_strreset_resp(
>  
>  		inreq = (struct sctp_strreset_inreq *)req;
>  		str_p = inreq->list_of_streams;
> -		nums = (ntohs(inreq->param_hdr.length) - sizeof(*inreq)) / 2;
> +		nums = (ntohs(inreq->param_hdr.length) - sizeof(*inreq)) /
> +		       sizeof(__u16);
>  
>  		*evp = sctp_ulpevent_make_stream_reset_event(asoc, flags,
>  			nums, str_p, GFP_ATOMIC);
> -- 
> 2.1.0
> 
> --
> 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 net 1/5] sctp: use sizeof(__u16) for each stream number length instead of magic number

[Marcelo Ricardo Leitner]

On Sat, Nov 25, 2017 at 09:05:32PM +0800, Xin Long wrote:
> Now in stream reconf part there are still some places using magic
> number 2 for each stream number length. To make it more readable,
> this patch is to replace them with sizeof(__u16).
> 
> Reported-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> ---
>  net/sctp/stream.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> index a11db21..09c797a 100644
> --- a/net/sctp/stream.c
> +++ b/net/sctp/stream.c
> @@ -563,7 +563,7 @@ struct sctp_chunk *sctp_process_strreset_outreq(
>  		flags = SCTP_STREAM_RESET_INCOMING_SSN;
>  	}
>  
> -	nums = (ntohs(param.p->length) - sizeof(*outreq)) / 2;
> +	nums = (ntohs(param.p->length) - sizeof(*outreq)) / sizeof(__u16);
>  	if (nums) {
>  		str_p = outreq->list_of_streams;
>  		for (i = 0; i < nums; i++) {
> @@ -627,7 +627,7 @@ struct sctp_chunk *sctp_process_strreset_inreq(
>  		goto out;
>  	}
>  
> -	nums = (ntohs(param.p->length) - sizeof(*inreq)) / 2;
> +	nums = (ntohs(param.p->length) - sizeof(*inreq)) / sizeof(__u16);
>  	str_p = inreq->list_of_streams;
>  	for (i = 0; i < nums; i++) {
>  		if (ntohs(str_p[i]) >= stream->outcnt) {
> @@ -927,7 +927,8 @@ struct sctp_chunk *sctp_process_strreset_resp(
>  
>  		outreq = (struct sctp_strreset_outreq *)req;
>  		str_p = outreq->list_of_streams;
> -		nums = (ntohs(outreq->param_hdr.length) - sizeof(*outreq)) / 2;
> +		nums = (ntohs(outreq->param_hdr.length) - sizeof(*outreq)) /
> +		       sizeof(__u16);
>  
>  		if (result == SCTP_STRRESET_PERFORMED) {
>  			if (nums) {
> @@ -956,7 +957,8 @@ struct sctp_chunk *sctp_process_strreset_resp(
>  
>  		inreq = (struct sctp_strreset_inreq *)req;
>  		str_p = inreq->list_of_streams;
> -		nums = (ntohs(inreq->param_hdr.length) - sizeof(*inreq)) / 2;
> +		nums = (ntohs(inreq->param_hdr.length) - sizeof(*inreq)) /
> +		       sizeof(__u16);
>  
>  		*evp = sctp_ulpevent_make_stream_reset_event(asoc, flags,
>  			nums, str_p, GFP_ATOMIC);
> -- 
> 2.1.0
> 
> --
> 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 net 0/5] sctp: a bunch of fixes for stream reconfig

[Neil Horman]

On Sat, Nov 25, 2017 at 09:05:31PM +0800, Xin Long wrote:
> This patchset is to make stream reset and asoc reset work more correctly
> for stream reconfig.
> 
> Thank to Marcelo making them very clear.
> 
> Xin Long (5):
>   sctp: use sizeof(__u16) for each stream number length instead of magic
>     number
>   sctp: only allow the out stream reset when the stream outq is empty
>   sctp: only allow the asoc reset when the asoc outq is empty
>   sctp: avoid flushing unsent queue when doing asoc reset
>   sctp: set sender next_tsn for the old result with ctsn_ack_point plus
>     1
> 
>  net/sctp/stream.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 65 insertions(+), 12 deletions(-)
> 
> -- 
> 2.1.0
> 
> 
Series
Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCH net 0/5] sctp: a bunch of fixes for stream reconfig

[Neil Horman]

On Sat, Nov 25, 2017 at 09:05:31PM +0800, Xin Long wrote:
> This patchset is to make stream reset and asoc reset work more correctly
> for stream reconfig.
> 
> Thank to Marcelo making them very clear.
> 
> Xin Long (5):
>   sctp: use sizeof(__u16) for each stream number length instead of magic
>     number
>   sctp: only allow the out stream reset when the stream outq is empty
>   sctp: only allow the asoc reset when the asoc outq is empty
>   sctp: avoid flushing unsent queue when doing asoc reset
>   sctp: set sender next_tsn for the old result with ctsn_ack_point plus
>     1
> 
>  net/sctp/stream.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 65 insertions(+), 12 deletions(-)
> 
> -- 
> 2.1.0
> 
> 
Series
Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCH net 0/5] sctp: a bunch of fixes for stream reconfig

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Sat, 25 Nov 2017 21:05:31 +0800

> This patchset is to make stream reset and asoc reset work more correctly
> for stream reconfig.
> 
> Thank to Marcelo making them very clear.

Series applied, thanks Xin.

Re: [PATCH net 0/5] sctp: a bunch of fixes for stream reconfig

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Sat, 25 Nov 2017 21:05:31 +0800

> This patchset is to make stream reset and asoc reset work more correctly
> for stream reconfig.
> 
> Thank to Marcelo making them very clear.

Series applied, thanks Xin.