Re: [PATCH net v3] sctp: carry peer capabilities across an INIT collision

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 · Medium: 0 (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).

- [High] sctp_assoc_update() (net/sctp/associola.c:1114) now copies
  new->peer.intl_capable into the surviving association but never re-…
- [High] sctp_assoc_update() now enables peer.asconf_capable
  (net/sctp/associola.c:1112) and peer.reconf_capable (associola.c:1113)…

Pre-existing issues:
- [Medium] The collision merge still does not carry
  asoc->peer.adaptation_ind.

--

sctp: carry peer capabilities across an INIT collision

sctp_assoc_update() folds a temporary association into an existing one when
an INIT collision is resolved, and today it only copies asoc->c, peer.rwnd,
peer.sack_needed, peer.auth_capable and peer.i.  This patch extends the copy
block with prsctp_capable, ecn_capable, asconf_capable, reconf_capable,
intl_capable, ipv4_address and ipv6_address so the surviving association
reflects what the peer advertised in the colliding INIT.

A few questions about the state that is derived from those bits but is not
refreshed by the merge.

> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 4521be3bd85aa..0bd0a662b5463 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -1107,6 +1107,13 @@ int sctp_assoc_update(struct sctp_association *asoc,
>  	asoc->peer.rwnd = new->peer.rwnd;
>  	asoc->peer.sack_needed = new->peer.sack_needed;
>  	asoc->peer.auth_capable = new->peer.auth_capable;
> +	asoc->peer.prsctp_capable = new->peer.prsctp_capable;
> +	asoc->peer.ecn_capable = new->peer.ecn_capable;
> +	asoc->peer.asconf_capable = new->peer.asconf_capable;
> +	asoc->peer.reconf_capable = new->peer.reconf_capable;

[Severity: High]

Turning these two bits on lets the surviving association accept inbound
ASCONF and RE-CONFIG, but the receive-side sequence counters those features
compare against are still the survivor's own.

Both are derived from the peer's Initial TSN in sctp_process_init(), which in
a collision runs on the temporary association only:

net/sctp/sm_make_chunk.c:sctp_process_init() {
	...
	asoc->strreset_inseq = asoc->peer.i.initial_tsn;
	...
	asoc->peer.addip_serial = asoc->peer.i.initial_tsn - 1;
}

sctp_assoc_update() copies asoc->peer.i but leaves peer.addip_serial and
strreset_inseq untouched, so a COOKIE_WAIT survivor keeps zero for both.

Can the peer's ASCONF then never be processed?  sctp_sf_do_asconf() now
clears the capability gate and reaches:

net/sctp/sm_statefuns.c:sctp_sf_do_asconf() {
	...
	if (serial == asoc->peer.addip_serial + 1) {
	...
	} else if (serial < asoc->peer.addip_serial + 1) {
	...
	} else {
		/* discarded */
	}
}

With the peer's serial equal to its Initial TSN and peer.addip_serial at 0,
neither arm matches and the chunk lands in the final else, so no ASCONF-ACK
is sent.  Does the peer then retransmit until its T4 limit and give up on
ASCONF entirely?

The RE-CONFIG side has the same shape.  sctp_process_strreset_outreq(), and
equally the inreq/tsnreq/addstrm_out/addstrm_in handlers, validate against
the stale strreset_inseq:

net/sctp/stream.c:sctp_process_strreset_outreq() {
	...
	if (TSN_lt(asoc->strreset_inseq, request_seq) ||
	    TSN_lt(request_seq, asoc->strreset_inseq - 2)) {
		result = SCTP_STRRESET_ERR_BAD_SEQNO;
		goto err;
	}
}

The testing notes describe ASCONF and RE-CONFIG appearing on the wire, which
exercises the outbound counters (addip_serial and strreset_outseq, both set
locally from asoc->c.initial_tsn).  Was the inbound direction tested too?

Should the merge also carry strreset_inseq and peer.addip_serial, or
re-derive them from new->peer.i.initial_tsn the way sctp_process_init() does?

> +	asoc->peer.intl_capable = new->peer.intl_capable;

[Severity: High]

This bit selects the chunk operation set, and the merge copies the bit
without re-selecting it.  The only place asoc->stream.si is derived from it
is sctp_stream_interleave_init(), reached from sctp_stream_init():

net/sctp/stream_interleave.c:sctp_stream_interleave_init() {
	...
	stream->si = asoc->peer.intl_capable ? &sctp_stream_interleave_1
					     : &sctp_stream_interleave_0;
}

In a collision that ran on the temporary association.  Neither branch of
sctp_assoc_update() refreshes it on the survivor: sctp_stream_update() copies
out/in/outcnt/incnt and leaves stream->si alone, sctp_stream_clear() only
zeroes mid/mid_uo, and the COOKIE_ECHOED case calls neither.  asoc->frag_point
is not recomputed either, although normal negotiation pairs the two:

net/sctp/sm_make_chunk.c:sctp_process_init() {
	...
	if (sctp_stream_init(&asoc->stream, ...))
		goto clean_up;

	/* Update frag_point when stream_interleave may get changed. */
	sctp_assoc_update_frag_point(asoc);
}

So after the merge peer.intl_capable is 1 while stream.si is still
&sctp_stream_interleave_0.  The INIT-ACK extension list is built from
peer.intl_capable, so the peer is told I-DATA is in use.  What happens when
it sends one?

net/sctp/sm_statefuns.c:sctp_eat_data() {
	...
	if (!asoc->stream.si->validate_data(chunk))
		return SCTP_IERROR_PROTO_VIOLATION;
}

sctp_validate_data() returns false for anything that is not SCTP_CID_DATA,
and the callers turn SCTP_IERROR_PROTO_VIOLATION into
sctp_sf_abort_violation().  Can a peer that legitimately follows the
negotiated extension abort the association this way?

There is a second effect in the send path, which branches directly on the
bit rather than on stream.si:

net/sctp/stream_sched.c:sctp_sched_dequeue_done() {
	...
	if (!list_is_last(&ch->frag_list, &ch->msg->chunks) &&
	    !q->asoc->peer.intl_capable) {
		/* pin stream.out_curr */
	}
}

With the bit set but plain DATA chunks being dequeued, stream.out_curr is no
longer pinned mid-message.  Can the PRIO/RR schedulers then interleave
fragments of different streams on the wire?  Before this patch the survivor
kept intl_capable at 0 and the pin was taken.

The reverse case looks reachable too: a restart where the new INIT no longer
advertises I-DATA now clears the bit while stream.si stays
&sctp_stream_interleave_1, leaving I-DATA and I-FORWARD-TSN installed for a
peer that did not negotiate them.

Would calling sctp_stream_interleave_init() and sctp_assoc_update_frag_point()
on the surviving association after the copy address both directions?

> +	asoc->peer.ipv4_address = new->peer.ipv4_address;
> +	asoc->peer.ipv6_address = new->peer.ipv6_address;
>  	asoc->peer.i = new->peer.i;

[Severity: Medium]

This is a pre-existing issue and not introduced by this patch, but since the
copy block is being extended: peer.adaptation_ind is still not carried.

sctp_process_param() records the peer's Adaptation Layer Indication on the
association it is processing, which in a collision is the temporary one:

net/sctp/sm_make_chunk.c:sctp_process_param() {
	...
	case SCTP_PARAM_ADAPTATION_LAYER_IND:
		asoc->peer.adaptation_ind = ntohl(param.aind->adaptation_ind);
}

and the completion path reads the survivor's copy right after the merge:

net/sctp/sm_statefuns.c:sctp_sf_do_dupcook_b() {
	...
	if (asoc->peer.adaptation_ind)
		sctp_add_cmd_sf(commands, SCTP_CMD_ADAPTATION_IND, SCTP_NULL());
}

For a COOKIE_WAIT survivor the field is still 0, so is the
SCTP_ADAPTATION_INDICATION notification dropped even though the peer asked
for it, and on a restart is an older non-zero value reported instead of the
new one?  asoc->c = new->c does update c.adaptation_ind, but nothing copies
that into peer.adaptation_ind after the merge.

Note this field is a separate __u32 rather than part of the bitfield block
the changelog calls "the rest of the block", so it is outside what the patch
sets out to fix.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921135733.577647-1-wbriggs%40cellusys.com