Re: [PATCH net] sctp: avoid livelock while updating retransmit path

[Xin Long]

On Thu, Aug 27, 2026 at 3:50 AM Yiqi Sun <sunyiqixm@xxxxxxxxx> wrote:
>
> sctp_assoc_update_retran_path() walks the association transport list from
> the current retransmit path's successor and stops once it reaches the
> current retransmit path again. However, the loop skips transports in
> SCTP_UNCONFIRMED state before checking for the wraparound condition.
>
> This makes the loop non-terminating when the association contains only
> UNCONFIRMED transports at that point and asoc->peer.retran_path is also
> UNCONFIRMED. One way to reach that state is through ASCONF wildcard
> DEL-IP processing after an unconfirmed address is selected as the
> primary transport. sctp_assoc_del_nonprimary_peers() then removes the
> other transports one by one; when removing the current retran_path,
> sctp_assoc_rm_peer() calls sctp_assoc_update_retran_path() before
> unlinking it. If the remaining candidate and the current retran_path are
> both UNCONFIRMED, the loop repeatedly continues before it can observe
> that it has completed a full pass.
>
> The same reproducer that exercised the bug fixed by commit 9b2854f86f0b
> ("sctp: don't free the ASCONF's own transport in DEL-IP processing") can
> still trigger this CPU stall after that fix is applied. With the UAF
> prevented, the ASCONF processing no longer dereferences the freed
> transport, but it can still reach the retransmit-path update described
> above and spin in the all-UNCONFIRMED case.
Please share the PoC with maintainers.

>
> Fix this by remembering whether the current transport is the original
> retran_path, still considering it as a candidate when it is not
> UNCONFIRMED, and then breaking after the candidate logic. This preserves
> the existing fallback semantics while making the full-pass termination
> independent of the transport state.
>
> Also restore the NULL guard around the retran_path assignment. In the
> all-UNCONFIRMED case there is no eligible replacement transport, and
> installing NULL would leave later retransmit-path users and the debug
> print with a NULL path.
>
> Fixes: 4c47af4d5eb2 ("net: sctp: rework multihoming retransmission path selection to rfc4960")
> Signed-off-by: Yiqi Sun <sunyiqixm@xxxxxxxxx>
> ---
>  net/sctp/associola.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index c0512c827d0f..6f19eb0b01e2 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -1272,6 +1272,7 @@ void sctp_assoc_update_retran_path(struct sctp_association *asoc)
>  {
>         struct sctp_transport *trans = asoc->peer.retran_path;
>         struct sctp_transport *trans_next = NULL;
> +       bool last = false;
>
>         /* We're done as we only have the one and only path. */
>         if (asoc->peer.transport_count == 1)
> @@ -1289,18 +1290,20 @@ void sctp_assoc_update_retran_path(struct sctp_association *asoc)
>                 /* Manually skip the head element. */
>                 if (&trans->transports == &asoc->peer.transport_addr_list)
>                         continue;
> -               if (trans->state == SCTP_UNCONFIRMED)
> -                       continue;
> -               trans_next = sctp_trans_elect_best(trans, trans_next);
> -               /* Active is good enough for immediate return. */
> -               if (trans_next->state == SCTP_ACTIVE)
> -                       break;
> +               last = trans == asoc->peer.retran_path;
> +               if (trans->state != SCTP_UNCONFIRMED) {
> +                       trans_next = sctp_trans_elect_best(trans, trans_next);
> +                       /* Active is good enough for immediate return. */
> +                       if (trans_next->state == SCTP_ACTIVE)
> +                               break;
> +               }
>                 /* We've reached the end, time to update path. */
> -               if (trans == asoc->peer.retran_path)
> +               if (last)
>                         break;
After removing the continue, I think you can keep using
if (trans == asoc->peer.retran_path) here without 'last' needed.

Thanks.

>         }
>
> -       asoc->peer.retran_path = trans_next;
> +       if (trans_next)
> +               asoc->peer.retran_path = trans_next;
>
>         pr_debug("%s: association:%p updated new path to addr:%pISpc\n",
>                  __func__, asoc, &asoc->peer.retran_path->ipaddr.sa);
> --
> 2.34.1
>