Re: [PATCH net v4] 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: 0 · Medium: 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).

- [Medium] On a peer restart (sctp_sf_do_dupcook_a -> sctp_assoc_update,
  ESTABLISHED branch), the patch re-derives only the inbound counters…
- [Low] The commit message says the change only affects INIT collisions
  (Table 2 action B), but it also changes behaviour when the peer…

--

sctp: carry peer capabilities across an INIT collision

When an INIT collision is resolved, this patch makes sctp_assoc_update()
also copy the peer's prsctp, ecn, asconf and reconf capability bits, the
ipv4/ipv6 address flags and adaptation_ind from the temporary
association. It also re-derives the inbound ASCONF and RE-CONFIG sequence
counters (peer.addip_serial and strreset_inseq) from the copied
peer.i.initial_tsn.

> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 4521be3bd85a..77ac155fdb70 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -1107,8 +1107,19 @@ 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;
> +	asoc->peer.ipv4_address = new->peer.ipv4_address;
> +	asoc->peer.ipv6_address = new->peer.ipv6_address;
> +	asoc->peer.adaptation_ind = new->peer.adaptation_ind;
>  	asoc->peer.i = new->peer.i;

[Severity: Low]
The commit message describes this as an INIT collision fix, but doesn't
the same code also run on a peer restart?

sctp_assoc_update() has one caller, sctp_sf_do_assoc_update(). That is
called from sctp_sf_do_dupcook_b() for the collision case, and also from
sctp_sf_do_dupcook_a() for the peer restart case:

net/sctp/sm_statefuns.c:sctp_sf_do_dupcook_a() {
    ...
	/* Update the content of current association. */
	if (sctp_sf_do_assoc_update((struct sctp_association *)asoc, new_asoc, commands))
		goto nomem;
    ...
}

These new assignments, and the counter resets below, have no state
check. On every peer restart, a live association in ESTABLISHED,
SHUTDOWN_PENDING or a similar state will now:

  - replace its prsctp, ecn, asconf and reconf capabilities
  - replace its ipv4_address and ipv6_address flags
  - replace peer.adaptation_ind
  - reset peer.addip_serial and strreset_inseq

The restart behaviour may well be correct, but the commit message doesn't
mention it, and the testing only covers a COOKIE_WAIT collision. With
Fixes: 1da177e4c3f4, this goes to all stable trees.

Could the commit message describe the restart path too, and could that
path be tested?

>  
> +	/* Re-derive sequence counters from the peer's Initial TSN */
> +	asoc->peer.addip_serial = asoc->peer.i.initial_tsn - 1;
> +	asoc->strreset_inseq = asoc->peer.i.initial_tsn;
> +

[Severity: Medium]
On the peer restart path (sctp_sf_do_dupcook_a()->sctp_assoc_update(),
state >= SCTP_STATE_ESTABLISHED), only the inbound counters are
re-derived here. Shouldn't the outbound counters asoc->addip_serial and
asoc->strreset_outseq be updated as well?

On restart, sctp_tietags_populate() reuses our Initial TSN:

	new_asoc->c.initial_tsn         = asoc->c.initial_tsn;

and sctp_unpack_cookie() bases the temporary association's outbound
counters on it:

	retval->next_tsn = retval->c.initial_tsn;
	retval->ctsn_ack_point = retval->next_tsn - 1;
	retval->addip_serial = retval->c.initial_tsn;
	retval->strreset_outseq = retval->c.initial_tsn;

The restarted peer runs sctp_process_init() on our INIT-ACK, so it now
expects serials starting from our initial_tsn. The ESTABLISHED branch of
sctp_assoc_update() copies next_tsn, ctsn_ack_point and
adv_peer_ack_point from new. It does not copy new->addip_serial or
new->strreset_outseq.

If N ASCONFs were sent before the restart, sctp_make_asconf() builds the
next one with:

	asconf.serial = htonl(asoc->addip_serial++);

and that serial is initial_tsn + N. The peer's sctp_sf_do_asconf() then
discards it silently, because it is above peer.addip_serial + 1:

	} else {
		return SCTP_DISPOSITION_DISCARD;

Would T4 then keep retransmitting this ASCONF, with every later ASCONF
queued behind it, until sctp_sf_t4_timer_expire() aborts the association
at max_retrans? RE-CONFIG requests would carry a stale strreset_outseq in
the same way, and the peer would answer SCTP_STRRESET_ERR_BAD_SEQNO.

Most of this mismatch predates the patch for associations that already
had asconf_capable or reconf_capable set. However, the patch adds a new
trigger. sctp_cmd_process_operr() clears asconf_capable after an ASCONF
has already used a serial:

	asoc->peer.asconf_capable = 0;

The copy above now sets it back to 1 when the restarted peer advertises
ASCONF. Before this patch, such an association never sent another ASCONF.
Now it sends one with a stale serial.

The COOKIE_WAIT collision case doesn't appear to be affected, because the
local outbound counters have not moved yet at that point.

Should the ESTABLISHED branch take addip_serial and strreset_outseq from
new, next to next_tsn?

>  	if (!sctp_tsnmap_init(&asoc->peer.tsn_map, SCTP_TSN_MAP_INITIAL,
>  			      asoc->peer.i.initial_tsn, GFP_ATOMIC))
>  		return -ENOMEM;

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