Re: [PATCH v1] sctp: fix race between addr_wq timer handler and backlog draining

[Xin Long]

On Fri, Nov 6, 2026 at 4:20 AM Binbin Deng <18983559317@xxxxxxx> wrote:
>
> sctp_addr_wq_timeout_handler() walks net->sctp.auto_asconf_splist and,
> holding only bh_lock_sock(), calls sctp_asconf_mgmt() on every
> auto-asconf socket. Unlike every other asynchronous SCTP entry point
> (sctp_rcv() queuing packets to the backlog, and the timer handlers in
> sctp_sm_side_effect()), it does not check sock_owned_by_user() to defer
> its work while a user context owns the socket.
>
> While a user thread owns the socket (e.g. recvmsg() blocked in a copy
> to user, or release_sock() draining the backlog), sk_lock.owned is set
> but the socket spinlock has been dropped, so bh_lock_sock() succeeds
> and the timer enters
>
>     sctp_asconf_mgmt() -> sctp_send_asconf_add/del_ip() -> sctp_do_sm()
>
> concurrently with the backlog drain path
>
>     release_sock() -> __release_sock() -> sk_backlog_rcv()
>       -> sctp_assoc_bh_rcv() -> sctp_do_sm()
>
> Two state machines then run on the same association with no common
> serialization and mutate shared state concurrently: the outqueue
> control chunk list, transport->packet.chunk_list and packet->auth, and
> the transport/dst lifetime.
>
> The bug was found by code review of this timer entry point against the
> sock_owned_by_user() avoidance used by all other SCTP asynchronous
> entries, and confirmed with a reproducer that races an RTM_DELADDR
> address event against a user-owned auto-asconf socket. It triggers the
> following, observed with KASAN and refcount debugging:
>
>   - rcuref "imbalanced put()" from dst_release() in
>     sctp_transport_route() racing the timer's transport routing,
>   - a GPF in sctp_packet_transmit() dereferencing a chunk pointer
>     corrupted by concurrent packet/chunk list use, and
>   - associations dying silently (ETIMEDOUT, GET_PEER_ADDRS failure).
>
> Defer the work like the other entries do: if the socket is owned by
> the user, drop both locks, leave the addr_waitq entry queued and
> re-arm addr_wq_timer to retry after SCTP_ADDRESS_TICK_DELAY. Sockets
> processed earlier for the same address event will see a duplicate
> ASCONF on retry; the peer reports that in an ASCONF-ACK error
> indication, which the association tolerates.
>
> Fixes: 9f7d653b67ae ("sctp: Add Auto-ASCONF support (core).")
> Signed-off-by: Binbin Deng <18983559317@xxxxxxx>
> ---
>  net/sctp/protocol.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> index 27c26e12f95d..7528a40a87f5 100644
> --- a/net/sctp/protocol.c
> +++ b/net/sctp/protocol.c
> @@ -645,6 +645,14 @@ static void sctp_addr_wq_timeout_handler(struct timer_list *t)
>                         if (!sctp_is_ep_boundall(sk))
>                                 continue;
>                         bh_lock_sock(sk);
> +                       if (sock_owned_by_user(sk)) {
> +                               bh_unlock_sock(sk);
> +                               spin_unlock_bh(&net->sctp.addr_wq_lock);
> +                               /* Try again later. */
> +                               mod_timer(&net->sctp.addr_wq_timer,
> +                                       jiffies + msecs_to_jiffies(SCTP_ADDRESS_TICK_DELAY));
> +                               return;
> +                       }

Can you merge these two mod_timer(), and move it out of the loops? like:

...
+
+       if (retry)
+               mod_timer(&net->sctp.addr_wq_timer,
+                         jiffies + msecs_to_jiffies(SCTP_ADDRESS_TICK_DELAY));
+
        spin_unlock_bh(&net->sctp.addr_wq_lock);

Thanks.