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

[netdev-bot+sashiko@kernel.org]

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