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

[Xin Long]

On Thu, Oct 1, 2026 at 3:05 AM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> 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?
>
This looks a legit one.

Since asoc->peer.asconf_capable and asoc->peer.reconf_capable are updated,
we should address it in the same patch.

So please follow the report above to update addip_serial and strreset_outseq in
the ESTABLISHED branch.

Thanks.

pw-bot: cr