[PATCH net 0/4] sctp: fix I-FORWARD-TSN handling with user message interleaving

[PATCH net 0/4] sctp: fix I-FORWARD-TSN handling with user message interleaving

[Tim Budding]

With user message interleaving (RFC 8260), fragments of messages on
different streams can be interleaved in TSN space. When a PR-SCTP
message is abandoned, the sender reports it in an I-FORWARD-TSN, which
carries a New Cumulative TSN and the stream, U bit and MID of the
skipped messages. The receiver still uses the New Cumulative TSN as if
it delimited messages, which loses or corrupts unrelated messages:

  TSN  stream  MID
    0    0      0    B   64KB reliable message on stream 0
  ...
    5    0      0
    6    1      0    BE  PR-SCTP message on stream 1, lost, abandoned
    7    0      0
  ...
  I-FORWARD-TSN new_cum=6 skip=[sid=1 mid=0]

The receiver frees the stream 0 fragments with TSNs 0-5 together with
the abandoned message. They have already been acked and are not
retransmitted, so the stream 0 message is never delivered and the
stream stays blocked.

Patch 1 frees reassembly fragments per I-FORWARD-TSN skip entry (stream,
U bit and MID) instead of by TSN. Patch 2 aborts an unordered partial
delivery when the message being delivered is the skipped one, instead
of merging it with the next unordered message on the stream. Patch 3
stops treating an I-FORWARD-TSN as an association reset when its New
Cumulative TSN is the highest TSN seen, which aborted partial delivery
on all streams and could abort the association. Patch 2 comes first
because the code that patch 3 removes also aborted that unordered
partial delivery in the leading edge case.

Patch 4 adds a selftest. Its ordered, unordered, leading_edge and uo_pd
tests fail without the corresponding fix and pass with it; its reset
test checks that an SSN/TSN association reset still works.

Tested with the new selftest, sctp_vrf.sh and the lksctp-tools
functional tests, also on a kernel with KASAN, lockdep and kmemleak
enabled. W=1 and sparse show no new warnings.

The first bug showed up as message loss in real use, and I found its
cause by AI-assisted code inspection. An AI assistant found the other
two while I was fixing it. The assistant also helped write the patches,
the selftest and these messages; I have reviewed and tested all of it.

Tim Budding (4):
  sctp: purge abandoned I-DATA fragments by stream and MID, not by TSN
  sctp: abort unordered partial delivery when its message is skipped
  sctp: don't treat an I-FORWARD-TSN as an association reset
  selftests: net: add SCTP I-FORWARD-TSN tests with message interleaving

 net/sctp/stream.c                           |   2 +
 net/sctp/stream_interleave.c                |  34 +-
 tools/testing/selftests/net/.gitignore      |   1 +
 tools/testing/selftests/net/Makefile        |   2 +
 tools/testing/selftests/net/sctp_ifwdtsn.c  | 538 ++++++++++++++++++++
 tools/testing/selftests/net/sctp_ifwdtsn.sh | 141 +++++
 6 files changed, 696 insertions(+), 22 deletions(-)
 create mode 100644 tools/testing/selftests/net/sctp_ifwdtsn.c
 create mode 100755 tools/testing/selftests/net/sctp_ifwdtsn.sh


base-commit: 7076f684d5132e6a6145e1da2fa74771ecd052e5
-- 
2.34.1

[PATCH net 1/4] sctp: purge abandoned I-DATA fragments by stream and MID, not by TSN

[Tim Budding]

When an I-FORWARD-TSN is received, sctp_report_iftsn() calls
sctp_intl_reasm_flushtsn(), which frees every fragment in reasm and
reasm_uo with a TSN at or below the New Cumulative TSN. That is only
correct for DATA chunks, where the fragments of a message use
consecutive TSNs. With I-DATA, fragments of messages on different
streams can be interleaved, and RFC 8260 section 2.2.3 says the receiver
"MUST NOT make any assumption about the TSN assignments of the sender".

So when a PR-SCTP message is abandoned while a large message on another
stream is still being received, the fragments of the large message below
the abandoned TSN are freed as well. They have already been acked and
are not retransmitted, so the large message is never delivered, its
remaining fragments stay queued until a later I-FORWARD-TSN covers them
or the association goes away, and on an ordered stream every later
message is blocked behind it. Also, received fragments of the abandoned
message above the New Cumulative TSN are only freed if a later
I-FORWARD-TSN happens to cover them.

The skip entries of the I-FORWARD-TSN already identify the abandoned
messages by stream, U bit and MID. Free the fragments from
sctp_intl_skip() based on those instead. An entry carries the largest
skipped MID for its stream and U bit (RFC 8260 section 2.3.1), and a
sender fragments only one message per stream at a time (section 2.2.2),
so all fragments on that stream with a MID at or below it can be freed.
The reassembly queues are sorted by stream and MID, so the walk stops
once it is past them. Fragments that no skip entry names, such as a late
copy of an abandoned fragment, are no longer freed by an unrelated
I-FORWARD-TSN; they stay until the association is freed. The association
reset paths that also call sctp_report_iftsn() are not affected: they
pass the highest TSN seen, so sctp_intl_abort_pd() still flushes the
whole ulpq for them.

Observed in real use as unexpected message loss when sending messages of
mixed sizes over one association; the cause was found by AI-assisted
code inspection. Reproduced on an unmodified kernel with the ordered and
unordered tests of the sctp_ifwdtsn.sh selftest added later in this
series, which pass with this fix.

Fixes: 47b20a88566f ("sctp: implement report_ftsn for sctp_stream_interleave")
Cc: stable@vger.kernel.org
Signed-off-by: Tim Budding <tim.budding@gmail.com>
---
 net/sctp/stream_interleave.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/net/sctp/stream_interleave.c b/net/sctp/stream_interleave.c
index 840f24045ae2..f8c818a5533c 100644
--- a/net/sctp/stream_interleave.c
+++ b/net/sctp/stream_interleave.c
@@ -1203,28 +1203,21 @@ static void sctp_report_fwdtsn(struct sctp_ulpq *ulpq, __u32 ftsn)
 	sctp_ulpq_abort_pd(ulpq, GFP_ATOMIC);
 }
 
-static void sctp_intl_reasm_flushtsn(struct sctp_ulpq *ulpq, __u32 ftsn)
+static void sctp_intl_reasm_flush_mid(struct sk_buff_head *reasm, __u16 sid,
+				      __u32 mid)
 {
 	struct sk_buff *pos, *tmp;
 
-	skb_queue_walk_safe(&ulpq->reasm, pos, tmp) {
+	skb_queue_walk_safe(reasm, pos, tmp) {
 		struct sctp_ulpevent *event = sctp_skb2event(pos);
-		__u32 tsn = event->tsn;
 
-		if (TSN_lte(tsn, ftsn)) {
-			__skb_unlink(pos, &ulpq->reasm);
-			sctp_ulpevent_free(event);
-		}
-	}
-
-	skb_queue_walk_safe(&ulpq->reasm_uo, pos, tmp) {
-		struct sctp_ulpevent *event = sctp_skb2event(pos);
-		__u32 tsn = event->tsn;
+		if (event->stream < sid)
+			continue;
+		if (event->stream > sid || MID_lt(mid, event->mid))
+			break;
 
-		if (TSN_lte(tsn, ftsn)) {
-			__skb_unlink(pos, &ulpq->reasm_uo);
-			sctp_ulpevent_free(event);
-		}
+		__skb_unlink(pos, reasm);
+		sctp_ulpevent_free(event);
 	}
 }
 
@@ -1232,8 +1225,6 @@ static void sctp_report_iftsn(struct sctp_ulpq *ulpq, __u32 ftsn)
 {
 	/* Move the Cumulattive TSN Ack ahead. */
 	sctp_tsnmap_skip(&ulpq->asoc->peer.tsn_map, ftsn);
-	/* purge the fragmentation queue */
-	sctp_intl_reasm_flushtsn(ulpq, ftsn);
 	/* abort only when it's for all data */
 	if (ftsn == sctp_tsnmap_get_max_tsn_seen(&ulpq->asoc->peer.tsn_map))
 		sctp_intl_abort_pd(ulpq, GFP_ATOMIC);
@@ -1260,6 +1251,7 @@ static void sctp_intl_skip(struct sctp_ulpq *ulpq, __u16 sid, __u32 mid,
 			sctp_intl_stream_abort_pd(ulpq, sid, mid, 0x1,
 						  GFP_ATOMIC);
 		}
+		sctp_intl_reasm_flush_mid(&ulpq->reasm_uo, sid, mid);
 		return;
 	}
 
@@ -1271,6 +1263,7 @@ static void sctp_intl_skip(struct sctp_ulpq *ulpq, __u16 sid, __u32 mid,
 		sctp_intl_stream_abort_pd(ulpq, sid, mid, 0x0, GFP_ATOMIC);
 	}
 
+	sctp_intl_reasm_flush_mid(&ulpq->reasm, sid, mid);
 	sctp_mid_skip(stream, in, sid, mid);
 
 	sctp_intl_reap_ordered(ulpq, sid);
-- 
2.34.1

[PATCH net 2/4] sctp: abort unordered partial delivery when its message is skipped

[Tim Budding]

When an I-FORWARD-TSN skips unordered messages on a stream,
sctp_intl_skip() aborts the stream's unordered partial delivery only if
the MID being delivered is lower than the skipped MID. If the skipped
message is the one being partially delivered, partial delivery is left
in progress: the application never gets SCTP_PARTIAL_DELIVERY_ABORTED,
and the next unordered message on that stream is delivered with MSG_EOR
as if it were the rest of the abandoned message, so the application
sees the two messages merged into one. Until a later unordered message
on that stream is abandoned, no other unordered partial delivery can
start there either.

Abort the partial delivery when the MID being delivered is at or below
the skipped MID, as the ordered case already does.

Found by an AI assistant while working on the previous patch. Reproduced
on an unmodified kernel with the uo_pd test of the sctp_ifwdtsn.sh
selftest added later in this series, which passes with this fix.

Fixes: de60fe910543 ("sctp: implement handle_ftsn for sctp_stream_interleave")
Cc: stable@vger.kernel.org
Signed-off-by: Tim Budding <tim.budding@gmail.com>
---
 net/sctp/stream_interleave.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sctp/stream_interleave.c b/net/sctp/stream_interleave.c
index f8c818a5533c..77fe0db5ff77 100644
--- a/net/sctp/stream_interleave.c
+++ b/net/sctp/stream_interleave.c
@@ -1246,7 +1246,7 @@ static void sctp_intl_skip(struct sctp_ulpq *ulpq, __u16 sid, __u32 mid,
 	struct sctp_stream *stream  = &ulpq->asoc->stream;
 
 	if (flags & SCTP_FTSN_U_BIT) {
-		if (sin->pd_mode_uo && MID_lt(sin->mid_uo, mid)) {
+		if (sin->pd_mode_uo && !MID_lt(mid, sin->mid_uo)) {
 			sin->pd_mode_uo = 0;
 			sctp_intl_stream_abort_pd(ulpq, sid, mid, 0x1,
 						  GFP_ATOMIC);
-- 
2.34.1

[PATCH net 3/4] sctp: don't treat an I-FORWARD-TSN as an association reset

[Tim Budding]

sctp_report_iftsn() handles both a received I-FORWARD-TSN and the SSN/TSN
reset request and response of RFC 6525, where all queued data has to be
discarded. It tells them apart by checking whether the New Cumulative
TSN equals the highest TSN seen, and if so calls sctp_intl_abort_pd(),
which aborts partial delivery on every stream, skips the MIDs being
partially delivered and flushes the whole ulpq.

However, sctp_tsnmap_skip() runs first and raises the highest TSN seen
to the New Cumulative TSN, so the check is also true for any
I-FORWARD-TSN that arrives before data with a higher TSN, e.g. when the
abandoned message was the last one sent, or after a burst of loss. All
partially received messages on all streams are then dropped. A stream
that was in partial delivery also has its MID skipped, so the next
fragment of that message fails sctp_validate_idata() as stale, and the
association is aborted with a protocol violation.

Drop the check from sctp_report_iftsn() and have the reset paths call
abort_pd() explicitly. For DATA chunks this changes nothing, as
sctp_report_fwdtsn() already aborts partial delivery and
sctp_ulpq_abort_pd() returns early when none is in progress. For I-DATA
the reset paths still discard everything as before.

This relies on the previous patch, which makes sctp_intl_skip() abort an
unordered partial delivery of the skipped message; the check removed
here used to do that at the leading edge.

Found by an AI assistant while working on the first patch in this
series. Reproduced on an unmodified kernel with the leading_edge test of
the sctp_ifwdtsn.sh selftest added later in this series, which passes
with this fix. Its reset test checks that an SSN/TSN association reset
still works with interleaving.

Fixes: 47b20a88566f ("sctp: implement report_ftsn for sctp_stream_interleave")
Cc: stable@vger.kernel.org
Signed-off-by: Tim Budding <tim.budding@gmail.com>
---
 net/sctp/stream.c            | 2 ++
 net/sctp/stream_interleave.c | 3 ---
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/sctp/stream.c b/net/sctp/stream.c
index e4354d58f20b..d5e71a2027f5 100644
--- a/net/sctp/stream.c
+++ b/net/sctp/stream.c
@@ -749,6 +749,7 @@ struct sctp_chunk *sctp_process_strreset_tsnreq(
 	 */
 	max_tsn_seen = sctp_tsnmap_get_max_tsn_seen(&asoc->peer.tsn_map);
 	asoc->stream.si->report_ftsn(&asoc->ulpq, max_tsn_seen);
+	asoc->stream.si->abort_pd(&asoc->ulpq, GFP_ATOMIC);
 
 	/* G1: Compute an appropriate value for the Receiver's Next TSN -- the
 	 *     TSN that the peer should use to send the next DATA chunk.  The
@@ -1028,6 +1029,7 @@ struct sctp_chunk *sctp_process_strreset_resp(
 			LIST_HEAD(temp);
 
 			asoc->stream.si->report_ftsn(&asoc->ulpq, mtsn);
+			asoc->stream.si->abort_pd(&asoc->ulpq, GFP_ATOMIC);
 
 			sctp_tsnmap_init(&asoc->peer.tsn_map,
 					 SCTP_TSN_MAP_INITIAL,
diff --git a/net/sctp/stream_interleave.c b/net/sctp/stream_interleave.c
index 77fe0db5ff77..f49b534ff78f 100644
--- a/net/sctp/stream_interleave.c
+++ b/net/sctp/stream_interleave.c
@@ -1225,9 +1225,6 @@ static void sctp_report_iftsn(struct sctp_ulpq *ulpq, __u32 ftsn)
 {
 	/* Move the Cumulattive TSN Ack ahead. */
 	sctp_tsnmap_skip(&ulpq->asoc->peer.tsn_map, ftsn);
-	/* abort only when it's for all data */
-	if (ftsn == sctp_tsnmap_get_max_tsn_seen(&ulpq->asoc->peer.tsn_map))
-		sctp_intl_abort_pd(ulpq, GFP_ATOMIC);
 }
 
 static void sctp_handle_fwdtsn(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk)
-- 
2.34.1

[PATCH net 4/4] selftests: net: add SCTP I-FORWARD-TSN tests with message interleaving

[Tim Budding]

Add tests for the receiver side of I-FORWARD-TSN with user message
interleaving (RFC 8260) and partial reliability. The first four tests
interleave a 64KB reliable message on stream 0 with a PR-SCTP message on
stream 1, which an nft rule drops so that the sender abandons it. The
tests are:

- ordered, unordered: the large message, ordered and unordered, must be
  delivered intact, and the abandoned message's received fragments must
  be freed.
- leading_edge: with the large message in partial delivery, drop all
  data after the abandoned message until the I-FORWARD-TSN arrives; this
  must not be treated as an association reset.
- uo_pd: abandon an unordered message during its partial delivery; the
  partial delivery must be aborted and the next unordered message on the
  stream must be delivered on its own.
- reset: an SSN/TSN association reset with interleaving completes and
  leaves a working association.

The PR-SCTP message is abandoned after one retransmission (RTX policy),
which keeps the tests stable on slow or loaded machines.

Signed-off-by: Tim Budding <tim.budding@gmail.com>
---
 tools/testing/selftests/net/.gitignore      |   1 +
 tools/testing/selftests/net/Makefile        |   2 +
 tools/testing/selftests/net/sctp_ifwdtsn.c  | 538 ++++++++++++++++++++
 tools/testing/selftests/net/sctp_ifwdtsn.sh | 141 +++++
 4 files changed, 682 insertions(+)
 create mode 100644 tools/testing/selftests/net/sctp_ifwdtsn.c
 create mode 100755 tools/testing/selftests/net/sctp_ifwdtsn.sh

diff --git a/tools/testing/selftests/net/.gitignore b/tools/testing/selftests/net/.gitignore
index 21412510ac26..ca451bfeda75 100644
--- a/tools/testing/selftests/net/.gitignore
+++ b/tools/testing/selftests/net/.gitignore
@@ -34,6 +34,7 @@ reuseport_bpf_numa
 reuseport_dualstack
 rxtimestamp
 sctp_hello
+sctp_ifwdtsn
 sk_bind_sendto_listen
 sk_connect_zero_addr
 sk_so_peek_off
diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index df215e1a6262..58ca9fc23979 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -90,6 +90,7 @@ TEST_PROGS := \
 	run_afpackettests \
 	run_netsocktests \
 	rxtimestamp.sh \
+	sctp_ifwdtsn.sh \
 	sctp_vrf.sh \
 	skf_net_off.sh \
 	srv6_encap_lookup_l3vpn_test.sh \
@@ -164,6 +165,7 @@ TEST_GEN_FILES := \
 	reuseport_addr_any \
 	rxtimestamp \
 	sctp_hello \
+	sctp_ifwdtsn \
 	skf_net_off \
 	so_netns_cookie \
 	so_rcv_listener \
diff --git a/tools/testing/selftests/net/sctp_ifwdtsn.c b/tools/testing/selftests/net/sctp_ifwdtsn.c
new file mode 100644
index 000000000000..15346aca10ef
--- /dev/null
+++ b/tools/testing/selftests/net/sctp_ifwdtsn.c
@@ -0,0 +1,538 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Helper for sctp_ifwdtsn.sh: I-FORWARD-TSN handling with SCTP user message
+ * interleaving (RFC 8260) and partial reliability (RFC 3758).
+ *
+ * Runs both endpoints of one association: the server socket is created in
+ * the current network namespace, the client socket in the namespace given
+ * on the command line. The caller sets up the link and any nft rule that
+ * drops chunks. Messages sent by the client:
+ *
+ *   big:    64KB on stream 0, reliable, fragmented into ~66 I-DATA chunks
+ *   small:  on stream 1, PR-SCTP "RTX" policy allowing one retransmission;
+ *           the nft rule drops (part of) it, so it gets abandoned
+ *   marker: short ordered message on stream 0
+ *
+ * Usage: sctp_ifwdtsn <client netns> <test>
+ * Exit status: 0 pass, 1 fail, 4 skip.
+ */
+#include <errno.h>
+#include <fcntl.h>
+#include <poll.h>
+#include <sched.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <time.h>
+#include <unistd.h>
+#include <arpa/inet.h>
+#include <netinet/in.h>
+#include <sys/socket.h>
+#include <linux/sctp.h>
+
+#include "kselftest.h"
+
+#ifndef SOL_SCTP
+#define SOL_SCTP	132
+#endif
+
+#define SRV_ADDR	"10.0.0.1"
+#define SRV_PORT	5000
+#define BIG_LEN		(64 * 1024)
+#define BUF_LEN		(BIG_LEN + 1024)
+#define MAXSEG		1000
+#define TIMEOUT_MS	5000
+#define MARKER		"marker"
+#define UO_MARKER	"uo-marker"
+
+struct test {
+	const char *name;
+	bool big_uo;		/* send big unordered */
+	int small_len;		/* 0: don't send the small message */
+	bool small_uo;		/* send small unordered */
+	int pd_point;		/* server SCTP_PARTIAL_DELIVERY_POINT */
+	bool expect_pd_abort;	/* PD on stream 1 must be aborted */
+	bool uo_marker;		/* then send an unordered marker on 1 */
+	bool reset;		/* SSN/TSN association reset after big */
+};
+
+static const struct test tests[] = {
+	{ .name = "ordered", .small_len = 3000 },
+	{ .name = "unordered", .big_uo = true, .small_len = 3000 },
+	{ .name = "leading_edge", .small_len = 1000, .pd_point = 2000 },
+	{ .name = "uo_pd", .small_len = 20000, .small_uo = true,
+	  .pd_point = 2000, .expect_pd_abort = true, .uo_marker = true },
+	{ .name = "reset", .reset = true },
+};
+
+static const struct test *t;
+
+static void die(const char *what)
+{
+	fprintf(stderr, "%s: %s\n", what, strerror(errno));
+	exit(KSFT_FAIL);
+}
+
+static void setopt(int fd, int opt, const void *val, socklen_t len,
+		   const char *name)
+{
+	if (setsockopt(fd, SOL_SCTP, opt, val, len) < 0) {
+		fprintf(stderr, "setsockopt(%s): %s\n", name, strerror(errno));
+		/* Interleaving disabled, or option not supported. */
+		exit(errno == EPERM || errno == ENOPROTOOPT ?
+		     KSFT_SKIP : KSFT_FAIL);
+	}
+}
+
+static void set_assoc_opt(int fd, int opt, unsigned int val, const char *name)
+{
+	struct sctp_assoc_value av = {
+		.assoc_id = SCTP_FUTURE_ASSOC,
+		.assoc_value = val,
+	};
+
+	setopt(fd, opt, &av, sizeof(av), name);
+}
+
+static void subscribe(int fd, int type)
+{
+	struct sctp_event ev = {
+		.se_assoc_id = SCTP_FUTURE_ASSOC,
+		.se_type = type,
+		.se_on = 1,
+	};
+
+	setopt(fd, SCTP_EVENT, &ev, sizeof(ev), "SCTP_EVENT");
+}
+
+static int sctp_socket(void)
+{
+	struct sctp_initmsg im = {
+		.sinit_num_ostreams = 2,
+		.sinit_max_instreams = 2,
+	};
+	struct sctp_paddrparams pp = {
+		.spp_assoc_id = SCTP_FUTURE_ASSOC,
+		.spp_flags = SPP_HB_DISABLE,
+	};
+	struct sctp_rtoinfo rto = {
+		.srto_assoc_id = SCTP_FUTURE_ASSOC,
+		.srto_initial = 300,
+		.srto_min = 300,
+		.srto_max = 1000,
+	};
+	int fd, val;
+
+	fd = socket(AF_INET, SOCK_STREAM, IPPROTO_SCTP);
+	if (fd < 0) {
+		perror("socket");
+		exit(errno == EPROTONOSUPPORT ? KSFT_SKIP : KSFT_FAIL);
+	}
+	val = 1;
+	setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(val));
+
+	setopt(fd, SCTP_INITMSG, &im, sizeof(im), "SCTP_INITMSG");
+	setopt(fd, SCTP_RTOINFO, &rto, sizeof(rto), "SCTP_RTOINFO");
+	/* No HEARTBEATs: the nft rules only look at the first chunk of a
+	 * packet, and a HEARTBEAT bundled in front of DATA would bypass them.
+	 */
+	setopt(fd, SCTP_PEER_ADDR_PARAMS, &pp, sizeof(pp),
+	       "SCTP_PEER_ADDR_PARAMS");
+	set_assoc_opt(fd, SCTP_PR_SUPPORTED, 1, "SCTP_PR_SUPPORTED");
+	val = 2;
+	setopt(fd, SCTP_FRAGMENT_INTERLEAVE, &val, sizeof(val),
+	       "SCTP_FRAGMENT_INTERLEAVE");
+	set_assoc_opt(fd, SCTP_INTERLEAVING_SUPPORTED, 1,
+		      "SCTP_INTERLEAVING_SUPPORTED");
+	if (t->reset) {
+		set_assoc_opt(fd, SCTP_RECONFIG_SUPPORTED, 1,
+			      "SCTP_RECONFIG_SUPPORTED");
+		set_assoc_opt(fd, SCTP_ENABLE_STREAM_RESET,
+			      SCTP_ENABLE_RESET_ASSOC_REQ,
+			      "SCTP_ENABLE_STREAM_RESET");
+		subscribe(fd, SCTP_ASSOC_RESET_EVENT);
+	}
+	val = 1;
+	setopt(fd, SCTP_RECVRCVINFO, &val, sizeof(val), "SCTP_RECVRCVINFO");
+
+	return fd;
+}
+
+static void send_msg(int fd, int sid, bool uo, const void *buf, size_t len,
+		     bool pr)
+{
+	char cbuf[CMSG_SPACE(sizeof(struct sctp_sndinfo)) +
+		  CMSG_SPACE(sizeof(struct sctp_prinfo))] = {};
+	struct iovec iov = { .iov_base = (void *)buf, .iov_len = len };
+	struct msghdr msg = {
+		.msg_iov = &iov,
+		.msg_iovlen = 1,
+		.msg_control = cbuf,
+		.msg_controllen = CMSG_SPACE(sizeof(struct sctp_sndinfo)),
+	};
+	struct sctp_sndinfo *si;
+	struct sctp_prinfo *pi;
+	struct cmsghdr *cm;
+
+	cm = CMSG_FIRSTHDR(&msg);
+	cm->cmsg_level = IPPROTO_SCTP;
+	cm->cmsg_type = SCTP_SNDINFO;
+	cm->cmsg_len = CMSG_LEN(sizeof(*si));
+	si = (struct sctp_sndinfo *)CMSG_DATA(cm);
+	si->snd_sid = sid;
+	si->snd_flags = uo ? SCTP_UNORDERED : 0;
+
+	if (pr) {
+		/* Abandon once the chunk would need a second retransmission:
+		 * only a chunk that is lost again after being retransmitted
+		 * qualifies, so abandonment does not depend on timing.
+		 */
+		msg.msg_controllen += CMSG_SPACE(sizeof(*pi));
+		cm = CMSG_NXTHDR(&msg, cm);
+		cm->cmsg_level = IPPROTO_SCTP;
+		cm->cmsg_type = SCTP_PRINFO;
+		cm->cmsg_len = CMSG_LEN(sizeof(*pi));
+		pi = (struct sctp_prinfo *)CMSG_DATA(cm);
+		pi->pr_policy = SCTP_PR_SCTP_RTX;
+		pi->pr_value = 1;
+	}
+
+	if (sendmsg(fd, &msg, 0) != (ssize_t)len)
+		die("sendmsg");
+}
+
+static unsigned char pattern(size_t i)
+{
+	return (i * 7 + (i >> 8)) & 0xff;
+}
+
+static long long now_ms(void)
+{
+	struct timespec ts;
+
+	clock_gettime(CLOCK_MONOTONIC, &ts);
+	return ts.tv_sec * 1000LL + ts.tv_nsec / 1000000;
+}
+
+/* Wait until the client has abandoned a sent message. */
+static bool wait_abandoned(int fd)
+{
+	long long end = now_ms() + TIMEOUT_MS;
+	struct sctp_prstatus ps;
+	socklen_t len;
+
+	while (now_ms() < end) {
+		memset(&ps, 0, sizeof(ps));
+		ps.sprstat_policy = SCTP_PR_SCTP_RTX;
+		len = sizeof(ps);
+		if (getsockopt(fd, SOL_SCTP, SCTP_PR_ASSOC_STATUS, &ps,
+			       &len) < 0)
+			die("SCTP_PR_ASSOC_STATUS");
+		if (ps.sprstat_abandoned_sent)
+			return true;
+		usleep(10000);
+	}
+	return false;
+}
+
+/* Wait for a notification of @type on @fd, discarding anything else. */
+static bool wait_event(int fd, int type)
+{
+	long long end = now_ms() + TIMEOUT_MS;
+	char buf[256];
+
+	while (now_ms() < end) {
+		struct pollfd pfd = { .fd = fd, .events = POLLIN };
+		struct iovec iov = { buf, sizeof(buf) };
+		struct msghdr msg = { .msg_iov = &iov, .msg_iovlen = 1 };
+		union sctp_notification *sn = (void *)buf;
+
+		if (poll(&pfd, 1, end - now_ms()) <= 0)
+			break;
+		if (recvmsg(fd, &msg, 0) <= 0)
+			break;
+		if ((msg.msg_flags & MSG_NOTIFICATION) &&
+		    sn->sn_header.sn_type == type)
+			return true;
+	}
+	return false;
+}
+
+/* Bytes held by the server association (/proc/net/sctp/assocs RX_QUEUE). */
+static long assoc_rx_queue(void)
+{
+	/* 7 columns, TX_QUEUE, RX_QUEUE, UID, INODE, LPORT */
+	const char *fmt = "%*s %*s %*s %*s %*s %*s %*s %*u %lu %*s %*s %d";
+	unsigned long rxq;
+	char line[512];
+	long ret = -1;
+	FILE *f;
+	int lport;
+
+	f = fopen("/proc/net/sctp/assocs", "r");
+	if (!f)
+		return -1;
+	while (fgets(line, sizeof(line), f)) {
+		if (sscanf(line, fmt, &rxq, &lport) == 2 && lport == SRV_PORT)
+			ret = rxq;
+	}
+	fclose(f);
+	return ret;
+}
+
+struct server_state {
+	unsigned char *buf[2];	/* message being received, per stream */
+	size_t len[2];
+	bool big, marker, uo_marker, reset_event;
+	int pd_aborts[2];
+	int errors;
+};
+
+static void check_msg(struct server_state *s, int sid)
+{
+	unsigned char *m = s->buf[sid];
+	size_t len = s->len[sid];
+	size_t i;
+
+	s->len[sid] = 0;
+	if (sid == 0 && len == sizeof(MARKER) && !memcmp(m, MARKER, len)) {
+		s->marker = true;
+		return;
+	}
+	if (sid == 1 && len == sizeof(UO_MARKER) &&
+	    !memcmp(m, UO_MARKER, len)) {
+		s->uo_marker = true;
+		return;
+	}
+	if (sid == 0 && len == BIG_LEN) {
+		for (i = 0; i < BIG_LEN && m[i] == pattern(i); i++)
+			;
+		if (i == BIG_LEN) {
+			s->big = true;
+			return;
+		}
+		printf("big message corrupted at offset %zu\n", i);
+	} else if (sid == 1 && len > sizeof(UO_MARKER) &&
+		   !memcmp(m + len - sizeof(UO_MARKER), UO_MARKER,
+			   sizeof(UO_MARKER))) {
+		printf("unordered marker appended to the abandoned message\n");
+	} else if (sid == 1 && t->small_len) {
+		printf("abandoned message delivered (%zu bytes)\n", len);
+	} else {
+		printf("unexpected %zu byte message on stream %d\n", len, sid);
+	}
+	s->errors++;
+}
+
+/* Receive until @done(s) is true or the timeout expires. */
+static bool server_recv(int fd, struct server_state *s,
+			bool (*done)(const struct server_state *s))
+{
+	long long end = now_ms() + TIMEOUT_MS;
+	static char cbuf[CMSG_SPACE(sizeof(struct sctp_rcvinfo))];
+	static unsigned char buf[BUF_LEN];
+
+	while (!done(s)) {
+		struct pollfd pfd = { .fd = fd, .events = POLLIN };
+		struct iovec iov = { buf, sizeof(buf) };
+		struct msghdr msg = {
+			.msg_iov = &iov,
+			.msg_iovlen = 1,
+			.msg_control = cbuf,
+			.msg_controllen = sizeof(cbuf),
+		};
+		union sctp_notification *sn = (void *)buf;
+		struct sctp_rcvinfo *ri = NULL;
+		struct cmsghdr *cm;
+		long long left;
+		ssize_t n;
+		int sid;
+
+		left = end - now_ms();
+		if (left <= 0 || poll(&pfd, 1, left) <= 0) {
+			printf("timeout waiting for data\n");
+			return false;
+		}
+		n = recvmsg(fd, &msg, 0);
+		if (n <= 0) {
+			printf("recvmsg: %s\n", n ? strerror(errno) : "EOF");
+			return false;
+		}
+
+		if (msg.msg_flags & MSG_NOTIFICATION) {
+			int type = sn->sn_header.sn_type;
+
+			if (type == SCTP_PARTIAL_DELIVERY_EVENT) {
+				sid = sn->sn_pdapi_event.pdapi_stream;
+				if (sid > 1)
+					sid = 1;
+				s->pd_aborts[sid]++;
+				s->len[sid] = 0;
+			} else if (type == SCTP_ASSOC_RESET_EVENT) {
+				s->reset_event = true;
+			}
+			continue;
+		}
+
+		for (cm = CMSG_FIRSTHDR(&msg); cm; cm = CMSG_NXTHDR(&msg, cm))
+			if (cm->cmsg_level == IPPROTO_SCTP &&
+			    cm->cmsg_type == SCTP_RCVINFO)
+				ri = (struct sctp_rcvinfo *)CMSG_DATA(cm);
+		sid = ri ? ri->rcv_sid : 0;
+		if (sid > 1 || s->len[sid] + n > BUF_LEN) {
+			printf("unexpected data on stream %d\n", sid);
+			return false;
+		}
+		memcpy(s->buf[sid] + s->len[sid], buf, n);
+		s->len[sid] += n;
+		if (msg.msg_flags & MSG_EOR)
+			check_msg(s, sid);
+	}
+	return true;
+}
+
+static bool got_big(const struct server_state *s)
+{
+	return s->big || s->errors;
+}
+
+static bool got_all(const struct server_state *s)
+{
+	return (s->big && s->marker && (!t->uo_marker || s->uo_marker)) ||
+	       s->errors;
+}
+
+static int client_socket(const char *netns)
+{
+	int self, other, fd;
+	char path[256];
+
+	snprintf(path, sizeof(path), "/run/netns/%s", netns);
+	self = open("/proc/self/ns/net", O_RDONLY);
+	other = open(path, O_RDONLY);
+	if (self < 0 || other < 0)
+		die("open netns");
+	if (setns(other, CLONE_NEWNET))
+		die("setns");
+	fd = sctp_socket();
+	if (setns(self, CLONE_NEWNET))
+		die("setns");
+	close(other);
+	close(self);
+	return fd;
+}
+
+int main(int argc, char **argv)
+{
+	struct sockaddr_in sa = {
+		.sin_family = AF_INET,
+		.sin_port = htons(SRV_PORT),
+	};
+	struct server_state s = {};
+	unsigned char *big, *small;
+	int lfd, sfd, cfd, i;
+	sctp_assoc_t id = 0;
+	bool ok;
+	long rxq;
+
+	if (argc != 3) {
+		fprintf(stderr, "usage: %s <client netns> <test>\n", argv[0]);
+		return KSFT_FAIL;
+	}
+	for (i = 0; i < (int)ARRAY_SIZE(tests); i++)
+		if (!strcmp(argv[2], tests[i].name))
+			t = &tests[i];
+	if (!t) {
+		fprintf(stderr, "unknown test %s\n", argv[2]);
+		return KSFT_FAIL;
+	}
+	inet_pton(AF_INET, SRV_ADDR, &sa.sin_addr);
+
+	lfd = sctp_socket();
+	if (t->pd_point) {
+		setopt(lfd, SCTP_PARTIAL_DELIVERY_POINT, &t->pd_point,
+		       sizeof(t->pd_point), "SCTP_PARTIAL_DELIVERY_POINT");
+		subscribe(lfd, SCTP_PARTIAL_DELIVERY_EVENT);
+	}
+	if (bind(lfd, (struct sockaddr *)&sa, sizeof(sa)) || listen(lfd, 1))
+		die("bind/listen");
+
+	cfd = client_socket(argv[1]);
+	set_assoc_opt(cfd, SCTP_MAXSEG, MAXSEG, "SCTP_MAXSEG");
+	/* Round robin, so that the small message is sent between fragments
+	 * of the big one.
+	 */
+	set_assoc_opt(cfd, SCTP_STREAM_SCHEDULER, SCTP_SS_RR,
+		      "SCTP_STREAM_SCHEDULER");
+	if (connect(cfd, (struct sockaddr *)&sa, sizeof(sa)))
+		die("connect");
+	sfd = accept(lfd, NULL, NULL);
+	if (sfd < 0)
+		die("accept");
+
+	big = malloc(BIG_LEN);
+	small = malloc(t->small_len + 1);
+	s.buf[0] = malloc(BUF_LEN);
+	s.buf[1] = malloc(BUF_LEN);
+	if (!big || !small || !s.buf[0] || !s.buf[1])
+		die("malloc");
+	for (i = 0; i < BIG_LEN; i++)
+		big[i] = pattern(i);
+	memset(small, 's', t->small_len);
+
+	send_msg(cfd, 0, t->big_uo, big, BIG_LEN, false);
+	if (t->small_len)
+		send_msg(cfd, 1, t->small_uo, small, t->small_len, true);
+
+	if (t->reset) {
+		ok = server_recv(sfd, &s, got_big);
+		if (ok && setsockopt(sfd, SOL_SCTP, SCTP_RESET_ASSOC, &id,
+				     sizeof(id)))
+			die("SCTP_RESET_ASSOC");
+		/* Only send the marker once the reset is done. */
+		if (ok && !wait_event(cfd, SCTP_ASSOC_RESET_EVENT)) {
+			printf("client: no association reset event\n");
+			return KSFT_FAIL;
+		}
+	}
+	send_msg(cfd, 0, false, MARKER, sizeof(MARKER), false);
+
+	if (t->small_len && !wait_abandoned(cfd)) {
+		printf("client: small message was not abandoned\n");
+		return KSFT_FAIL;
+	}
+	if (t->uo_marker)
+		send_msg(cfd, 1, true, UO_MARKER, sizeof(UO_MARKER), false);
+
+	ok = server_recv(sfd, &s, got_all);
+	rxq = assoc_rx_queue();
+
+	if (!s.big)
+		printf("big message not delivered intact\n");
+	if (!s.marker)
+		printf("marker not delivered\n");
+	if (t->uo_marker && !s.uo_marker)
+		printf("unordered marker not delivered as its own message\n");
+	if (s.pd_aborts[0] || (s.pd_aborts[1] && !t->expect_pd_abort)) {
+		printf("unexpected partial delivery abort\n");
+		ok = false;
+	}
+	if (t->expect_pd_abort && !s.pd_aborts[1]) {
+		printf("partial delivery on stream 1 not aborted\n");
+		ok = false;
+	}
+	if (t->reset && !s.reset_event) {
+		printf("server: no association reset event\n");
+		ok = false;
+	}
+	if (rxq < 0) {
+		printf("association is gone\n");
+		ok = false;
+	} else if (rxq > 0) {
+		printf("%ld bytes left queued on the association\n", rxq);
+		ok = false;
+	}
+
+	return ok && !s.errors ? KSFT_PASS : KSFT_FAIL;
+}
diff --git a/tools/testing/selftests/net/sctp_ifwdtsn.sh b/tools/testing/selftests/net/sctp_ifwdtsn.sh
new file mode 100755
index 000000000000..2bf8d28cee52
--- /dev/null
+++ b/tools/testing/selftests/net/sctp_ifwdtsn.sh
@@ -0,0 +1,141 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# SCTP I-FORWARD-TSN handling with user message interleaving (RFC 8260).
+#
+# TOPO: CLI_NS (veth, 10.0.0.2) <---> (veth, 10.0.0.1) SRV_NS
+#
+# A large reliable message on stream 0 is sent interleaved with a PR-SCTP
+# message on stream 1 that an nft rule in SRV_NS drops, so the sender
+# abandons it and sends an I-FORWARD-TSN. The netem delay keeps the sender
+# window-limited, so the two messages' fragments interleave. Most of the
+# checks are done by sctp_ifwdtsn, see there.
+
+source lib.sh
+
+ALL_TESTS="
+	test_ordered
+	test_unordered
+	test_leading_edge
+	test_uo_pd
+	test_reset
+"
+
+# nft matches on the first chunk of a packet: chunk type at byte 12 of the
+# SCTP header, flags at byte 13, I-DATA stream id at byte 20 and FSN at
+# byte 28. SCTP_MAXSEG limits fragments to 1000 bytes and GSO is limited
+# to the MTU, so no two full-size fragments share a packet.
+NFT_SCTP="meta l4proto sctp"
+NFT_CHUNK_IDATA="@th,96,8 64"
+NFT_CHUNK_IFWDTSN="@th,96,8 194"
+NFT_FLAG_B="@th,104,8 & 0x02 == 0x02"
+NFT_SID1="@th,160,16 1"
+NFT_FSN5="@th,224,32 5"
+
+setup()
+{
+	setup_ns CLI_NS SRV_NS || return
+	defer cleanup_all_ns
+
+	ip -n "$SRV_NS" link add veth0 type veth peer name veth0 \
+		netns "$CLI_NS" || return
+	ip -n "$SRV_NS" addr add 10.0.0.1/24 dev veth0 || return
+	ip -n "$CLI_NS" addr add 10.0.0.2/24 dev veth0 || return
+	for ns in "$SRV_NS" "$CLI_NS"; do
+		ip -n "$ns" link set veth0 gso_max_size 1500 up || return
+		tc -n "$ns" qdisc add dev veth0 root netem delay 50ms || return
+		ip netns exec "$ns" sysctl -wq net.sctp.intl_enable=1 \
+			net.sctp.prsctp_enable=1 net.sctp.reconf_enable=1 ||
+			return
+	done
+}
+
+# $1: nft rules for the server's input hook (may be empty)
+nft_rules()
+{
+	ip netns exec "$SRV_NS" nft -f - <<-EOF
+		table ip sctp_test {
+			set blk {
+				type ipv4_addr
+				flags dynamic
+			}
+			chain input {
+				type filter hook input priority 0;
+				$1
+			}
+		}
+	EOF
+}
+
+# $1: test name for sctp_ifwdtsn, $2: description, $3: nft rules (optional)
+run_helper()
+{
+	if ! setup || { [ -n "$3" ] && ! nft_rules "$3"; }; then
+		log_test_skip "$2"
+		return
+	fi
+	ip netns exec "$SRV_NS" ./sctp_ifwdtsn "$CLI_NS" "$1"
+	RET=$?
+	[ "$RET" -eq "$ksft_skip" ] || [ "$RET" -eq 0 ] || RET=$ksft_fail
+	log_test "$2"
+}
+
+# Fragments of the abandoned message must not take fragments of a live
+# message below the New Cumulative TSN with them, and the abandoned
+# message's own received fragments must be freed.
+test_ordered()
+{
+	run_helper ordered \
+		"abandoned message does not purge ordered message" \
+		"$NFT_SCTP $NFT_CHUNK_IDATA $NFT_SID1 $NFT_FLAG_B drop"
+}
+
+test_unordered()
+{
+	run_helper unordered \
+		"abandoned message does not purge unordered message" \
+		"$NFT_SCTP $NFT_CHUNK_IDATA $NFT_SID1 $NFT_FLAG_B drop"
+}
+
+# Drop the abandoned message and every packet after it until one starting
+# with the I-FORWARD-TSN arrives, so that its New Cumulative TSN is the
+# highest TSN received. This must not abort the partial delivery of the
+# message on stream 0.
+test_leading_edge()
+{
+	run_helper leading_edge \
+		"I-FORWARD-TSN at the highest TSN is not a reset" "
+		$NFT_SCTP $NFT_CHUNK_IFWDTSN delete @blk { ip saddr }
+		$NFT_SCTP $NFT_CHUNK_IDATA $NFT_SID1 add @blk { ip saddr } drop
+		$NFT_SCTP ip saddr @blk drop"
+}
+
+# Abandon an unordered message during its partial delivery: the partial
+# delivery must be aborted, and the next unordered message on the stream
+# must not be appended to it.
+test_uo_pd()
+{
+	run_helper uo_pd "abandoned unordered partial delivery is aborted" \
+		"$NFT_SCTP $NFT_CHUNK_IDATA $NFT_SID1 $NFT_FSN5 drop"
+}
+
+# An SSN/TSN association reset with interleaving completes and leaves a
+# working association.
+test_reset()
+{
+	run_helper reset "association reset with interleaving"
+}
+
+require_command nft
+require_command tc
+modprobe -q sctp
+if [ ! -d /proc/net/sctp ]; then
+	echo "SKIP: SCTP not available"
+	exit "$ksft_skip"
+fi
+
+trap defer_scopes_cleanup EXIT
+
+tests_run
+
+exit "$EXIT_STATUS"
-- 
2.34.1

Re: [PATCH net 1/4] sctp: purge abandoned I-DATA fragments by stream and MID, not by TSN

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 (1) · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] At this commit, sctp_report_iftsn()
  (net/sctp/stream_interleave.c:1224-1231) still contains the branch `if…
- [Medium] Removing the TSN-based purge from sctp_report_iftsn() also
  removes the only non-teardown cleanup for unordered I-DATA fragments…
- [Low] Previously, an I-FORWARD-TSN chunk walked reasm and reasm_uo once
  each (sctp_intl_reasm_flushtsn), and a U-bit skip entry did O(1) work.

Pre-existing issues:
- [Medium] At this commit, the unordered branch of sctp_intl_skip()
  (net/sctp/stream_interleave.c:1249) aborts unordered partial delivery…

--

sctp: purge abandoned I-DATA fragments by stream and MID, not by TSN

This commit removes the TSN-based purge of the I-DATA reassembly queues
from sctp_report_iftsn(). In its place, sctp_intl_skip() frees fragments
on reasm or reasm_uo based on the stream, U bit and MID of each
I-FORWARD-TSN skip entry. The walk stops once it is past the named
stream and MID.

> diff --git a/net/sctp/stream_interleave.c b/net/sctp/stream_interleave.c
> index 840f24045ae2e..f8c818a5533c1 100644
> --- a/net/sctp/stream_interleave.c
> +++ b/net/sctp/stream_interleave.c
> @@ -1203,28 +1203,21 @@ static void sctp_report_fwdtsn(struct sctp_ulpq *ulpq, __u32 ftsn)
>  	sctp_ulpq_abort_pd(ulpq, GFP_ATOMIC);
>  }
>  
> -static void sctp_intl_reasm_flushtsn(struct sctp_ulpq *ulpq, __u32 ftsn)
> +static void sctp_intl_reasm_flush_mid(struct sk_buff_head *reasm, __u16 sid,
> +				      __u32 mid)
>  {
>  	struct sk_buff *pos, *tmp;
>  
> -	skb_queue_walk_safe(&ulpq->reasm, pos, tmp) {
> +	skb_queue_walk_safe(reasm, pos, tmp) {
>  		struct sctp_ulpevent *event = sctp_skb2event(pos);
> -		__u32 tsn = event->tsn;
>  
> -		if (TSN_lte(tsn, ftsn)) {
> -			__skb_unlink(pos, &ulpq->reasm);
> -			sctp_ulpevent_free(event);
> -		}
> -	}
> -
> -	skb_queue_walk_safe(&ulpq->reasm_uo, pos, tmp) {
> -		struct sctp_ulpevent *event = sctp_skb2event(pos);
> -		__u32 tsn = event->tsn;
> +		if (event->stream < sid)
> +			continue;

[Severity: Low]
Each call to sctp_intl_reasm_flush_mid() starts at the head of the queue.
It steps over every fragment of a lower-numbered stream without freeing
anything. sctp_intl_skip() now calls it once for every U-bit skip entry
(on reasm_uo) and once for every ordered entry (on reasm). Before this
patch, an I-FORWARD-TSN walked each queue only once.

sctp_validate_iftsn() only checks the stream bound, so it accepts
duplicate entries:

	sctp_walk_ifwdtsn(skip, chunk)
		if (ntohs(skip->stream) >= incnt)
			return false;

sctp_sf_eat_fwd_tsn() only discards a chunk when sctp_tsnmap_check()
returns < 0, so a chunk with an old or duplicate cumulative TSN is still
processed.

Suppose a peer keeps reasm_uo full of incomplete unordered fragments on
low streams. It then sends I-FORWARD-TSN chunks with thousands of U-bit
entries for a high sid. Could each such chunk cost O(entries x
fragments) list iterations in softirq with the socket lock held?

sctp_intl_reap_ordered() already walks the lobby this way for ordered
entries, so the pattern itself is not new. This patch adds another
instance of it.

[ ... ]

> @@ -1232,8 +1225,6 @@ static void sctp_report_iftsn(struct sctp_ulpq *ulpq, __u32 ftsn)
>  {
>  	/* Move the Cumulattive TSN Ack ahead. */
>  	sctp_tsnmap_skip(&ulpq->asoc->peer.tsn_map, ftsn);
> -	/* purge the fragmentation queue */
> -	sctp_intl_reasm_flushtsn(ulpq, ftsn);

[Severity: Medium]
With the TSN-based purge removed, what frees an unordered fragment of an
abandoned message that arrives after the I-FORWARD-TSN naming that
message has been processed?

sctp_validate_idata() accepts unordered I-DATA without any MID check:

	if (chunk->chunk_hdr->flags & SCTP_DATA_UNORDERED)
		return true;

So a late fragment with a TSN above the New Cumulative TSN is stored in
reasm_uo by sctp_intl_reasm_uo(). Its siblings were already freed by
sctp_intl_reasm_flush_mid(), so it can never be reassembled.

For example:

- Unordered message M on stream S has fragments at TSN 10 and 12. TSN 11
  belongs to another stream.
- The sender abandons M and sends I-FORWARD-TSN(cum=10, {S, U, M}).
- Fragment 12 arrives late, for example on another path.
- TSN 11 arrives, so ctsn moves to 12.
- The sender drops M from q->abandoned and does not name {S, U, M}
  again.

>From then on, the fragment is only freed by a later unordered skip on
stream S with MID >= M, by an association reset, or at teardown. Before
this patch, any later I-FORWARD-TSN covering its TSN would have freed it.

While it stays queued, it keeps its sctp_assoc_rwnd_decrease() charge,
its rmem charges, and the association reference taken in
sctp_ulpevent_receive_data(). sctp_ulpq_renege_list() won't reclaim it
either:

	if (TSN_lte(tsn, sctp_tsnmap_get_ctsn(tsnmap)))
		break;

Renege walks from the tail. If a stuck fragment sits at the tail of
reasm_uo, it also blocks reneging of every other reasm_uo entry.

The commit message says:

  "Fragments that no skip entry names, such as a late copy of an
  abandoned fragment, are no longer freed by an unrelated I-FORWARD-TSN;
  they stay until the association is freed."

Should the commit message also mention the lost rwnd and receive buffer
space? Alternatively, could these fragments be dropped when they arrive,
or when the cumulative TSN moves past them?

>  	/* abort only when it's for all data */
>  	if (ftsn == sctp_tsnmap_get_max_tsn_seen(&ulpq->asoc->peer.tsn_map))
>  		sctp_intl_abort_pd(ulpq, GFP_ATOMIC);

[Severity: High]
Can this branch still flush the whole ulpq for an ordinary I-FORWARD-TSN?

Just above, sctp_tsnmap_skip() raises max_tsn_seen to ftsn whenever
ftsn is higher:

	if (TSN_lt(map->max_tsn_seen, tsn))
		map->max_tsn_seen = tsn;

So the equality holds whenever the New Cumulative TSN is at or above the
highest TSN received. That happens, for example, when the abandoned
message was the last one sent, or when later data is still in flight.

sctp_intl_abort_pd() then aborts partial delivery on every stream. It
also frees everything in reasm, reasm_uo and lobby, whatever the stream
or MID:

	/* intl abort pd happens only when all data needs to be cleaned */
	sctp_ulpq_flush(ulpq);

sctp_sf_eat_fwd_tsn() queues SCTP_CMD_REPORT_FWDTSN before
SCTP_CMD_PROCESS_FWDTSN. So this flush also runs before the new per-MID
handling in sctp_intl_skip().

Wouldn't this still free the already-acked fragments of an in-progress
message on another stream? That is the loss this patch sets out to fix.
The commit message describes this branch only in terms of the reset
paths:

  "The association reset paths that also call sctp_report_iftsn() are
  not affected: they pass the highest TSN seen, so sctp_intl_abort_pd()
  still flushes the whole ulpq for them."

A later patch in this series, "sctp: don't treat an I-FORWARD-TSN as an
association reset", seems to address this. It drops the max_tsn_seen
check from sctp_report_iftsn() and makes the stream reset paths call
si->abort_pd() directly. This patch is tagged for stable on its own.
Should that change be folded in here, or should the two be backported
together?

> @@ -1260,6 +1251,7 @@ static void sctp_intl_skip(struct sctp_ulpq *ulpq, __u16 sid, __u32 mid,
>  			sctp_intl_stream_abort_pd(ulpq, sid, mid, 0x1,
>  						  GFP_ATOMIC);
>  		}
> +		sctp_intl_reasm_flush_mid(&ulpq->reasm_uo, sid, mid);

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. Does the
new flush leave unordered partial delivery stuck when the skipped MID is
the message currently being partially delivered?

The check just above in sctp_intl_skip() uses a strict comparison:

	if (sin->pd_mode_uo && MID_lt(sin->mid_uo, mid)) {

When mid == sin->mid_uo, pd_mode_uo stays set and no
SCTP_PARTIAL_DELIVERY_ABORTED event is queued.
sctp_intl_reasm_flush_mid() then frees all remaining fragments of that
MID, because it only stops on MID_lt(mid, event->mid).

The stream is left in unordered partial delivery with nothing left to
deliver. sctp_intl_retrieve_first_uo() and
sctp_intl_retrieve_reassembled_uo() skip that stream, and the
application never gets the abort notification.

The strict comparison comes from the original code. The next patch in
this series, "sctp: abort unordered partial delivery when its message is
skipped", changes it to !MID_lt(mid, sin->mid_uo).

>  		return;
>  	}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261024120627.837568-1-tim.budding%40gmail.com

Re: [PATCH net 3/4] sctp: don't treat an I-FORWARD-TSN as an association reset

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] After this patch, sctp_intl_skip() is the only code that emits
  SCTP_PARTIAL_DELIVERY_ABORTED when an I-FORWARD-TSN reaches the…

--

sctp: don't treat an I-FORWARD-TSN as an association reset

This commit removes the ftsn == max_tsn_seen check from
sctp_report_iftsn(). That check made every I-FORWARD-TSN reaching the
leading edge abort partial delivery on all streams and flush the ulpq.
The SSN/TSN reset paths in stream.c now call abort_pd() explicitly, and
the leading-edge abort is left to sctp_intl_skip().

> diff --git a/net/sctp/stream_interleave.c b/net/sctp/stream_interleave.c
> index 77fe0db5ff774..f49b534ff78fd 100644
> --- a/net/sctp/stream_interleave.c
> +++ b/net/sctp/stream_interleave.c
> @@ -1225,9 +1225,6 @@ static void sctp_report_iftsn(struct sctp_ulpq *ulpq, __u32 ftsn)
>  {
>  	/* Move the Cumulattive TSN Ack ahead. */
>  	sctp_tsnmap_skip(&ulpq->asoc->peer.tsn_map, ftsn);
> -	/* abort only when it's for all data */
> -	if (ftsn == sctp_tsnmap_get_max_tsn_seen(&ulpq->asoc->peer.tsn_map))
> -		sctp_intl_abort_pd(ulpq, GFP_ATOMIC);
>  }

[Severity: Medium]
With this call gone, sctp_intl_skip() now sends
SCTP_PARTIAL_DELIVERY_ABORTED for an I-FORWARD-TSN at the leading edge.
Is the pdapi_seq it reports still the message that was being partially
delivered?

The removed path, sctp_intl_abort_pd(), reported the in-progress MID:

net/sctp/stream_interleave.c:sctp_intl_abort_pd() {
    ...
		if (sin->pd_mode_uo) {
			sin->pd_mode_uo = 0;

			mid = sin->mid_uo;
			sctp_intl_stream_abort_pd(ulpq, sid, mid, 0x1, gfp);
		}

		if (sin->pd_mode) {
			sin->pd_mode = 0;

			mid = sin->mid;
			sctp_intl_stream_abort_pd(ulpq, sid, mid, 0, gfp);
    ...
}

sctp_intl_skip() passes the skip entry MID instead:

net/sctp/stream_interleave.c:sctp_intl_skip() {
    ...
		if (sin->pd_mode_uo && !MID_lt(mid, sin->mid_uo)) {
			sin->pd_mode_uo = 0;
			sctp_intl_stream_abort_pd(ulpq, sid, mid, 0x1,
						  GFP_ATOMIC);
		}
    ...
	if (sin->pd_mode) {
		sin->pd_mode = 0;
		sctp_intl_stream_abort_pd(ulpq, sid, mid, 0x0, GFP_ATOMIC);
	}
    ...
}

The skip MID is the largest MID skipped on that stream, so it can be
greater than the in-progress MID. sctp_generate_iftsn() merges abandoned
chunks with the same stream and U flag into one entry and keeps the last
MID:

	skip_pos = sctp_get_skip_pos(&ftsn_skip_arr[0], nskips,
				     sid, flags);
	...
	ftsn_skip_arr[skip_pos].mid = mid;

sctp_ulpevent_make_pdapi() then copies this value into the user-visible
field with pd->pdapi_seq = seq.

Here is an example. The receiver is partially delivering ordered MID 3
on stream S. The sender abandons MIDs 3 and 4 on S and sends nothing
after them. The I-FORWARD-TSN then carries a single entry {S, mid=4}.
Before this patch userspace got pdapi_seq=3. Now it gets pdapi_seq=4,
a message that was never partially delivered.

sctp_intl_skip() already reported the skip MID for I-FORWARD-TSNs that
do not reach the leading edge. This patch extends that to the
leading-edge case, which the commit message describes as common. The
code is still the same at the end of the series.

Could sctp_intl_skip() pass sin->mid_uo (unordered) or sin->mid
(ordered) to sctp_intl_stream_abort_pd() for the notification, and
keep using the skip MID for the flush and sctp_mid_skip()?

>  
>  static void sctp_handle_fwdtsn(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk)

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261024120627.837568-1-tim.budding%40gmail.com

Re: [PATCH net 4/4] selftests: net: add SCTP I-FORWARD-TSN tests with message interleaving

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In the ordered and unordered tests, main() in
  tools/testing/selftests/net/sctp_ifwdtsn.c samples the server…
- [Low] The reset case in tools/testing/selftests/net/sctp_ifwdtsn.c (`{
  .name = "reset", .reset = true }` at line 66, the reset branch of…
- [Low] wait_event() in tools/testing/selftests/net/sctp_ifwdtsn.c checks
  the deadline with `while (now_ms() < end)` (line 245), then reads the…

--

selftests: net: add SCTP I-FORWARD-TSN tests with message interleaving

This adds a helper and a script that test the receiver side of
I-FORWARD-TSN with RFC 8260 interleaving and PR-SCTP. Four cases send a
64KB reliable message interleaved with a PR-SCTP message that nft drops
and the sender abandons. A fifth case runs an SSN/TSN association reset.

> diff --git a/tools/testing/selftests/net/sctp_ifwdtsn.c b/tools/testing/selftests/net/sctp_ifwdtsn.c
> new file mode 100644
> index 0000000000000..15346aca10ef0
> --- /dev/null
> +++ b/tools/testing/selftests/net/sctp_ifwdtsn.c
> @@ -0,0 +1,538 @@

[ ... ]

> +/* Wait for a notification of @type on @fd, discarding anything else. */
> +static bool wait_event(int fd, int type)
> +{
> +	long long end = now_ms() + TIMEOUT_MS;
> +	char buf[256];
> +
> +	while (now_ms() < end) {
> +		struct pollfd pfd = { .fd = fd, .events = POLLIN };
> +		struct iovec iov = { buf, sizeof(buf) };
> +		struct msghdr msg = { .msg_iov = &iov, .msg_iovlen = 1 };
> +		union sctp_notification *sn = (void *)buf;
> +
> +		if (poll(&pfd, 1, end - now_ms()) <= 0)

[Severity: Low]
This isn't a bug, but the deadline check and the poll() timeout use two
separate now_ms() samples. If end - now_ms() comes out negative, poll()
blocks with no timeout. The helper would then hang until the kselftest
runner timeout instead of printing "client: no association reset event".

As the code stands this doesn't look reachable. wait_event() is only
called on the client socket in the reset test. That socket only
subscribes to SCTP_ASSOC_RESET_EVENT and gets no user data, so the loop
never runs a second time.

Could this compute the remaining time once and check left <= 0, as
server_recv() does, in case wait_event() is reused later?

> +			break;

[ ... ]

> +	send_msg(cfd, 0, t->big_uo, big, BIG_LEN, false);
> +	if (t->small_len)
> +		send_msg(cfd, 1, t->small_uo, small, t->small_len, true);
> +
> +	if (t->reset) {
> +		ok = server_recv(sfd, &s, got_big);
> +		if (ok && setsockopt(sfd, SOL_SCTP, SCTP_RESET_ASSOC, &id,
> +				     sizeof(id)))
> +			die("SCTP_RESET_ASSOC");

[Severity: Low]
Does the reset case check that queued I-DATA is discarded on reset?

The tests[] entry is:

	{ .name = "reset", .reset = true },

So no PR-SCTP message is sent and no partial delivery point is set.
SCTP_RESET_ASSOC is only issued after server_recv(got_big) has received
the whole 64KB message.

At that point sctp_process_strreset_tsnreq() runs on the client and
sctp_process_strreset_resp() handles PERFORMED on the server. On both
sides the reasm, reasm_uo and lobby queues are empty, and no stream is
in partial delivery. The previous patch in this series adds this call
on both paths:

	asoc->stream.si->abort_pd(&asoc->ulpq, GFP_ATOMIC);

Here it does nothing. The test would still pass if either call were
removed, or if sctp_intl_abort_pd() stopped flushing.

After "sctp: don't treat an I-FORWARD-TSN as an association reset",
sctp_report_iftsn() only calls sctp_tsnmap_skip(). These abort_pd()
calls are now the only place queued I-DATA is discarded on reset. That
patch says "For I-DATA the reset paths still discard everything as
before" and cites this test as its check.

The test does show that MIDs restart at 0, because the marker is
delivered. Could the test leave stale fragments, or a message in partial
delivery, pending on the server when the reset is issued?

[ ... ]

> +	send_msg(cfd, 0, false, MARKER, sizeof(MARKER), false);
> +
> +	if (t->small_len && !wait_abandoned(cfd)) {
> +		printf("client: small message was not abandoned\n");
> +		return KSFT_FAIL;
> +	}
> +	if (t->uo_marker)
> +		send_msg(cfd, 1, true, UO_MARKER, sizeof(UO_MARKER), false);
> +
> +	ok = server_recv(sfd, &s, got_all);
> +	rxq = assoc_rx_queue();

[Severity: Medium]
Can the RX_QUEUE check fail on a correct kernel in the ordered and
unordered tests?

For those tests got_all() only needs s->big && s->marker.
wait_abandoned() only polls the client's local abandoned_sent counter.
sctp_chunk_abandoned() increments that counter when the sender decides
to abandon. At that point the I-FORWARD-TSN still has at least the 50ms
netem delay ahead of it.

Until sctp_intl_skip() runs on the server, the received middle and end
fragments of the small message stay in the reasm queue and count toward
rmem_alloc. If the tail of big and the marker arrive before the
I-FORWARD-TSN, server_recv() returns at once. The test then reports
"... bytes left queued on the association".

With default sysctls nothing explicitly prevents this ordering. The B
fragment of small goes out early, and the client abandons it on the
first SACK after the fast retransmit, long before the tail of big is
sent.

What happens if net.core.wmem_default is lowered? sk_sndbuf can be
large enough for the 64KB sendmsg but smaller than the memory charged
for the ~66 big chunks. Then sendmsg(small) looks like it would block
here:

net/sctp/socket.c:sctp_sendmsg_to_asoc() {
    ...
	if (sctp_wspace(asoc) <= 0 || !sk_wmem_schedule(sk, msg_len)) {
		timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
		err = sctp_wait_for_sndbuf(asoc, transport, &timeo, msg_len);
    ...
}

Small and then the marker would be sent after all of big. The SACK for
the marker supplies the third miss report that triggers the fast
retransmit. So the marker would always reach the server before the
I-FORWARD-TSN.

Should this poll assoc_rx_queue() until it reaches 0, with a timeout?
Or wait for some sign on the server that the I-FORWARD-TSN was
processed? leading_edge and uo_pd already have such a barrier: the nft
rule, and the uo-marker that is sent after abandonment.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261024120627.837568-1-tim.budding%40gmail.com