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

[Tim Budding <tim.budding@gmail.com>]

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