net/sctp/associola.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
On Thu, Aug 27, 2026 at 3:50 AM, Xin Long wrote:
> After removing the continue, I think you can keep using
> if (trans == asoc->peer.retran_path) here without 'last' needed.
Yes. The v1 'last' variable was redundant once the SCTP_UNCONFIRMED
path no longer uses continue. Drop it in this revision and retain the
original wraparound comparison after the candidate-selection block.
The reproducer is attached.
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.
Fix this by considering a transport only when it is not UNCONFIRMED,
then checking whether the walk has returned to retran_path. This makes
the full-pass termination independent of the transport state while
preserving the existing fallback selection semantics.
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@gmail.com>
---
Changes in v2:
- Drop the redundant 'last' variable as suggested by Xin Long.
- Link to v1: https://lore.kernel.org/r/20260827075006.3979566-1-sunyiqixm@gmail.com/
---
net/sctp/associola.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index c0512c827d0f..4521be3bd85a 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -1289,18 +1289,19 @@ 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;
+ 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)
break;
}
- 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
On Tue, Sep 1, 2026 at 10:53 PM Yiqi Sun <sunyiqixm@gmail.com> wrote:
>
> On Thu, Aug 27, 2026 at 3:50 AM, Xin Long wrote:
> > After removing the continue, I think you can keep using
> > if (trans == asoc->peer.retran_path) here without 'last' needed.
>
> Yes. The v1 'last' variable was redundant once the SCTP_UNCONFIRMED
> path no longer uses continue. Drop it in this revision and retain the
> original wraparound comparison after the candidate-selection block.
>
> The reproducer is attached.
>
> 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.
>
> Fix this by considering a transport only when it is not UNCONFIRMED,
> then checking whether the walk has returned to retran_path. This makes
> the full-pass termination independent of the transport state while
> preserving the existing fallback selection semantics.
>
> 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@gmail.com>
> ---
> Changes in v2:
> - Drop the redundant 'last' variable as suggested by Xin Long.
> - Link to v1: https://lore.kernel.org/r/20260827075006.3979566-1-sunyiqixm@gmail.com/
> ---
> net/sctp/associola.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index c0512c827d0f..4521be3bd85a 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -1289,18 +1289,19 @@ 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;
> + 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)
> break;
> }
>
> - 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
>
Hi Yiqi,
Could you repost the patch in a separate thread without including
the discussion below in the commit message?
On Thu, Aug 27, 2026 at 3:50 AM, Xin Long wrote:
> After removing the continue, I think you can keep using
> if (trans == asoc->peer.retran_path) here without 'last' needed.
Yes. The v1 'last' variable was redundant once the SCTP_UNCONFIRMED
path no longer uses continue. Drop it in this revision and retain the
original wraparound comparison after the candidate-selection block.
Thanks.
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 1 · Low: 0
- [Medium] The permanent part of the commit message (everything above the
`---` line) is not a changelog.
--
sctp: avoid livelock while updating retransmit path
sctp_assoc_update_retran_path() could loop forever when every remaining
transport, including the current retran_path, was in SCTP_UNCONFIRMED
state, because the state check ran before the wraparound test. The patch
moves the candidate selection under a state test so the wraparound
comparison is always reached, and restores the NULL guard before assigning
asoc->peer.retran_path.
Link: https://lore.kernel.org/r/20260827075006.3979566-1-sunyiqixm@gmail.com/
The code change itself looks fine to me; my only comments are about the
commit message.
> sctp: avoid livelock while updating retransmit path
>
> --jv3lfyjkrnvkybth
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
>
> On Thu, Aug 27, 2026 at 3:50 AM, Xin Long wrote:
> > After removing the continue, I think you can keep using
> > if (trans == asoc->peer.retran_path) here without 'last' needed.
>
> Yes. The v1 'last' variable was redundant once the SCTP_UNCONFIRMED
> path no longer uses continue. Drop it in this revision and retain the
> original wraparound comparison after the candidate-selection block.
>
> The reproducer is attached.
[Severity: Medium]
Was this text meant to end up above the --- separator? Everything above
--- is what git am records permanently, so the MIME boundary and headers:
--jv3lfyjkrnvkybth
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
get committed verbatim into the changelog.
The reply to the reviewer and the v1/v2 discussion ("The v1 'last'
variable was redundant once the SCTP_UNCONFIRMED path no longer uses
continue. Drop it in this revision") also read as thread material rather
than changelog material, and the same information is already recorded in
the "Changes in v2" block below the --- marker.
Can the statement "The reproducer is attached." be dropped or replaced?
Nothing is attached to the patch, and there is no selftest or
Reported-by/Closes tag pointing at one, so a reader of
git log net/sctp/associola.c has no way to reach the described ASCONF
wildcard DEL-IP scenario.
Would you mind respinning with only the last three paragraphs (the
livelock description, the fix description, and the NULL guard note) plus
the Fixes and Signed-off-by tags above the --- line?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902025206.phbpyxmpf4zrtdpx%40sunyiqi-llm-kernel
© 2016 - 2026 Red Hat, Inc.