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

[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

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 1/4] sctp: purge abandoned I-DATA fragments by stream and MID, not by TSN

[Tim Budding]

On Sun, 25 Oct 2026 12:08:47 +0000, netdev-bot+sashiko@kernel.org wrote:
> > +		if (event->stream < sid)
> > +			continue;
>
> [Severity: Low]
> Each call to sctp_intl_reasm_flush_mid() starts at the head of the queue.

[...]

Agreed. Walking each queue once per I-FORWARD-TSN would be a separate
change. I could send that to net-next.

> > -	/* 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?

[...]

> 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?

Correct, and the commit message is wrong to say such fragments stay
until the association is freed. An unordered fragment of an abandoned
message that first arrives after the last I-FORWARD-TSN naming that
message, with a TSN above the cumulative TSN, can't be reassembled. It
keeps its rwnd and receive buffer charge until a later unordered skip
on that stream with the same or a higher MID, a reset, or teardown. At
the tail of reasm_uo it also stops reneging, as any fragment below the
cumulative TSN already can.

One late fragment is enough if nothing after it has arrived: the
receiver then delays its SACK for the I-FORWARD-TSN, the fragment fills
the gap within that delay, and the sender never names the message
again. With the last fragment of an abandoned three-fragment message
arriving in that window, that fragment stays queued (RX_QUEUE 1232). A
duplicate can't do it, as the TSN map drops the second copy.

Before this patch a later I-FORWARD-TSN covering its TSN would have
freed it, so if another message is abandoned later, this patch keeps
it longer. Without such a later I-FORWARD-TSN, this patch frees more
than before: in the same test, the TSN-based purge leaves the
message's received fragments above the New Cumulative TSN queued
(RX_QUEUE 2464 without the delay), and this patch frees all but the
late one.

For an ordered message the same late fragment aborts the association
instead: its MID is below the next expected one, so
sctp_validate_idata() fails. That happens on net today as well. I
could look at it separately.

Freeing fragments once the cumulative TSN moves past them doesn't work
with interleaving: a live unordered message can have fragments below
the cumulative TSN, and freeing those is the loss this patch fixes.
Dropping late fragments on arrival needs new per-stream state, the
highest skipped unordered MID, which must be cleared on resets and must
not go stale when MIDs wrap. I'd rather do that in a separate patch
than grow this fix (question at the end). Either way, v2 will spell out
the cost in the commit message.

> >  	/* 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?

[...]

> 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?

Yes. At this commit an I-FORWARD-TSN whose New Cumulative TSN is at or
above the highest TSN received still takes that branch, and
sctp_intl_abort_pd() frees every queued fragment, including those of a
message still being received on another stream: the same loss this
patch fixes below the leading edge. If that message is ordered and in
partial delivery, its MID is also skipped, and the association is
aborted when its next fragment arrives. With this patch alone the
ordered and unordered tests pass and leading_edge fails. The later
patch fixes it. v2 adds a leading_edge case without partial delivery,
which shows the loss directly.

I kept the two apart because they are different mistakes in
sctp_report_iftsn(): the TSN-based purge, and treating ftsn ==
max_tsn_seen as a reset. But they share the Fixes: tag and the
symptom, so folding them is reasonable too. The later patch needs
patch 2 and the new pdapi_seq patch (see the reply to 3/4), and
neither of those needs this one, so folding would mean those two
first, then this patch and the later one as a single patch (question
at the end). If they stay separate, this commit message will say in v2
that the leading-edge case is only fixed by "sctp: don't treat an
I-FORWARD-TSN as an association reset". All three fixes are Cc: stable
and apply cleanly, in order, to the v5.10 through v6.18 release tags.

> >  			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?

Yes, and that is the next patch in this series, "sctp: abort unordered
partial delivery when its message is skipped". At this commit no abort
notification is sent and the next unordered message on the stream is
delivered as the rest of the skipped one. The uo_pd test fails at this
commit and passes from the next patch on.

Xin, Marcelo, two questions for v2:

1. Fold "sctp: don't treat an I-FORWARD-TSN as an association reset"
   into this patch, or keep it separate?
2. Should dropping late unordered fragments be part of this series, or
   a follow-up to net?

Unless you prefer otherwise, v2 keeps them separate and leaves the drop
to a follow-up.

pw-bot: cr

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

[Tim Budding]

On Sun, 25 Oct 2026 12:08:48 +0000, netdev-bot+sashiko@kernel.org wrote:
> [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?

[...]

> 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()?

Thanks, this is a real bug. sctp_intl_skip() has always reported the
skip entry's MID, which is the largest MID skipped on that stream, so
for an I-FORWARD-TSN below the leading edge pdapi_seq could already be
wrong when more than one message on the stream was skipped. For
unordered streams it was wrong whenever the abort was reported at all,
as the old check only fired when the skip MID was larger than the one
being delivered. This patch removes the path that happened to report
the right MID at the leading edge, so it now shows up there too.

v2 will add a patch before this one that does that: report sin->mid_uo
/ sin->mid in the notification and keep the skip MID for the flush and
sctp_mid_skip(). The selftest will check pdapi_seq for an I-FORWARD-TSN
below the leading edge that skips two messages on the stream being
partially delivered.

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

[Xin Long]

On Mon, Oct 26, 2026 at 1:27 AM Tim Budding <tim.budding@gmail.com> wrote:
>
> On Sun, 25 Oct 2026 12:08:47 +0000, netdev-bot+sashiko@kernel.org wrote:
> > > +           if (event->stream < sid)
> > > +                   continue;
> >
> > [Severity: Low]
> > Each call to sctp_intl_reasm_flush_mid() starts at the head of the queue.
>
> [...]
>
> Agreed. Walking each queue once per I-FORWARD-TSN would be a separate
> change. I could send that to net-next.
>
> > > -   /* 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?
>
> [...]
>
> > 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?
>
> Correct, and the commit message is wrong to say such fragments stay
> until the association is freed. An unordered fragment of an abandoned
> message that first arrives after the last I-FORWARD-TSN naming that
> message, with a TSN above the cumulative TSN, can't be reassembled. It
> keeps its rwnd and receive buffer charge until a later unordered skip
> on that stream with the same or a higher MID, a reset, or teardown. At
> the tail of reasm_uo it also stops reneging, as any fragment below the
> cumulative TSN already can.
>
> One late fragment is enough if nothing after it has arrived: the
> receiver then delays its SACK for the I-FORWARD-TSN, the fragment fills
> the gap within that delay, and the sender never names the message
> again. With the last fragment of an abandoned three-fragment message
> arriving in that window, that fragment stays queued (RX_QUEUE 1232). A
> duplicate can't do it, as the TSN map drops the second copy.
>
> Before this patch a later I-FORWARD-TSN covering its TSN would have
> freed it, so if another message is abandoned later, this patch keeps
> it longer. Without such a later I-FORWARD-TSN, this patch frees more
> than before: in the same test, the TSN-based purge leaves the
> message's received fragments above the New Cumulative TSN queued
> (RX_QUEUE 2464 without the delay), and this patch frees all but the
> late one.
>
> For an ordered message the same late fragment aborts the association
> instead: its MID is below the next expected one, so
> sctp_validate_idata() fails. That happens on net today as well. I
> could look at it separately.
>
> Freeing fragments once the cumulative TSN moves past them doesn't work
> with interleaving: a live unordered message can have fragments below
> the cumulative TSN, and freeing those is the loss this patch fixes.
> Dropping late fragments on arrival needs new per-stream state, the
> highest skipped unordered MID, which must be cleared on resets and must
> not go stale when MIDs wrap. I'd rather do that in a separate patch
> than grow this fix (question at the end). Either way, v2 will spell out
> the cost in the commit message.
>
> > >     /* 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?
>
> [...]
>
> > 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?
>
> Yes. At this commit an I-FORWARD-TSN whose New Cumulative TSN is at or
> above the highest TSN received still takes that branch, and
> sctp_intl_abort_pd() frees every queued fragment, including those of a
> message still being received on another stream: the same loss this
> patch fixes below the leading edge. If that message is ordered and in
> partial delivery, its MID is also skipped, and the association is
> aborted when its next fragment arrives. With this patch alone the
> ordered and unordered tests pass and leading_edge fails. The later
> patch fixes it. v2 adds a leading_edge case without partial delivery,
> which shows the loss directly.
>
> I kept the two apart because they are different mistakes in
> sctp_report_iftsn(): the TSN-based purge, and treating ftsn ==
> max_tsn_seen as a reset. But they share the Fixes: tag and the
> symptom, so folding them is reasonable too. The later patch needs
> patch 2 and the new pdapi_seq patch (see the reply to 3/4), and
> neither of those needs this one, so folding would mean those two
> first, then this patch and the later one as a single patch (question
> at the end). If they stay separate, this commit message will say in v2
> that the leading-edge case is only fixed by "sctp: don't treat an
> I-FORWARD-TSN as an association reset". All three fixes are Cc: stable
> and apply cleanly, in order, to the v5.10 through v6.18 release tags.
>
> > >                     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?
>
> Yes, and that is the next patch in this series, "sctp: abort unordered
> partial delivery when its message is skipped". At this commit no abort
> notification is sent and the next unordered message on the stream is
> delivered as the rest of the skipped one. The uo_pd test fails at this
> commit and passes from the next patch on.
>
> Xin, Marcelo, two questions for v2:
>
> 1. Fold "sctp: don't treat an I-FORWARD-TSN as an association reset"
>    into this patch, or keep it separate?
> 2. Should dropping late unordered fragments be part of this series, or
>    a follow-up to net?
>
Please try to fold it into this patch, and leave the drop to a follow-up.

Thanks.

> Unless you prefer otherwise, v2 keeps them separate and leaves the drop
> to a follow-up.
>
> pw-bot: cr