Re: [PATCH net v3] sctp: fix ASCONF address change on single-homed hosts

[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] In sctp_process_asconf_param()
  (net/sctp/sm_make_chunk.c:3181-3183), a plain DEL-IP that removes…
- [Low] The rewritten wildcard DEL-IP branch in
  sctp_process_asconf_param() (net/sctp/sm_make_chunk.c:3162-3170)…

--

sctp: fix ASCONF address change on single-homed hosts

This patch removes the refusal of a DEL-IP for the ASCONF's own transport
that commit 9b2854f86f0b added, since that refusal broke the RFC 5061
single-homed renumber sequence. The wildcard DEL-IP now looks up the
transport for the packet source, and both DEL-IP paths retarget
asconf->transport to a surviving transport.

> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index 90de272842512..15c2d87575a8a 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -3160,9 +3160,12 @@ static __be16 sctp_process_asconf_param(struct sctp_association *asoc,
>  		 * packet MUST be deleted.
>  		 */
>  		if (af->is_any(&addr)) {
> -			sctp_assoc_set_primary(asoc, asconf->transport);
> -			sctp_assoc_del_nonprimary_peers(asoc,
> -							asconf->transport);
> +			peer = sctp_assoc_lookup_paddr(asoc, &asconf->source);
> +			if (!peer)
> +				return SCTP_ERROR_REQ_REFUSED;

[Severity: Low]

This changes how a wildcard DEL-IP behaves on the wire. Before, this
branch always made asconf->transport primary, removed every other peer
address and returned SCTP_ERROR_NO_ERROR.

Now the request is refused if the packet source is not already a
transport of the association. That can happen when the ASCONF was found
through its Address Parameter and no earlier ADD-IP in the same chunk
added the source. For example:

    [Addr L] [DEL-IP 0.0.0.0] [ADD-IP S]

The delete used to succeed here. Now it returns an error cause and all
the other peer addresses stay.

Refusing may well be more correct. However, the commit message only says
the branch now "look[s] up the transport of the packet source rather than
reusing asconf->transport". Since this is tagged for stable, could the
changelog mention the new SCTP_ERROR_REQ_REFUSED case?

> +			sctp_assoc_set_primary(asoc, peer);
> +			sctp_assoc_del_nonprimary_peers(asoc, peer);
> +			asconf->transport = peer;
>  			return SCTP_ERROR_NO_ERROR;
>  		}
>  
> @@ -3175,13 +3178,9 @@ static __be16 sctp_process_asconf_param(struct sctp_association *asoc,
>  		if (!peer)
>  			return SCTP_ERROR_DNS_FAILED;
>  
> -		/* Don't free asconf->transport; a later wildcard DEL-IP
> -		 * parameter reuses it.
> -		 */
> -		if (peer == asconf->transport)
> -			return SCTP_ERROR_REQ_REFUSED;
> -
>  		sctp_assoc_rm_peer(asoc, peer);
> +		if (asconf->transport == peer)
> +			asconf->transport = asoc->peer.primary_path;

[Severity: Medium]

Can primary_path be a transport other than the packet source here?

The D8 check earlier in sctp_process_asconf_param() returns
SCTP_ERROR_DEL_SRC_IP, so this branch only removes asconf->transport when
two things are true. The association was found through the Address
Parameter L, and the packet came from a different address S.

In that case a transport for S exists only if an earlier ADD-IP S in the
same chunk created it. That is the [Addr L] [ADD-IP S] [DEL-IP L]
sequence this patch fixes.

With two peer addresses after the ADD, sctp_assoc_rm_peer() makes the
first remaining entry (T_S) primary, so this works. With three or more
addresses it does not seem to. For example, with peer addresses
{A (primary), L}:

    ADD-IP S  -> {A (primary), L, S}
    DEL-IP L  -> sctp_assoc_rm_peer(asoc, T_L), primary stays T_A
                 asconf->transport = T_A

This happens even though T_S exists.

sctp_inq_pop() reuses the same sctp_chunk for every chunk bundled after
the ASCONF and does not reset chunk->transport. Those chunks are then
credited to T_A in three places.

First, sctp_vtag_verify() copies the packet's encap_port into T_A:

    chunk->transport->encap_port = SCTP_INPUT_CB(chunk->skb)->encap_port;

Second, sctp_assoc_bh_rcv() updates last_data_from and last_time_heard
on T_A, so the SACK for bundled DATA goes to A:

    if (sctp_chunk_is_data(chunk))
        asoc->peer.last_data_from = chunk->transport;
    ...
    if (chunk->transport)
        chunk->transport->last_time_heard = ktime_get();

Third, a wildcard SET_PRIMARY later in the same ASCONF resolves
sctp_source(asconf) to T_A->ipaddr. A then becomes primary instead of
the packet source, and the request is still acked as a success.

Before this patch, bundled chunks in this case were credited to T_L,
which was not the source either. Still, the new code picks a transport
other than S even when T_S exists.

The wildcard branch in this same patch already calls
sctp_assoc_lookup_paddr(asoc, &asconf->source). Would it make sense to
do that here too, and fall back to asoc->peer.primary_path only when
there is no transport for the source? That would also match the
"Remember where we came from" meaning that sctp_rcv() gives
chunk->transport.

>  		break;
>  	case SCTP_PARAM_SET_PRIMARY:
>  		/* ADDIP Section 4.2.4

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106195126.1563231-1-andrey.misyrov%40gmail.com