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

[Xin Long]

On Wed, Nov 11, 2026 at 3:44 AM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 3 · 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).
>
> - [High] Kernel memory can grow without limit through repeated DEL_IP
>   ASCONF chunks.
> - [Medium] The new early exit breaks an invariant that
>   sctp_addr_wq_mgmt() depends on.
> - [Medium] One socket can now stall address-event processing for the
>   whole netns.
> - [Medium] The commit message says that sockets processed earlier 'will
>   see a duplicate ASCONF on retry; the peer reports that in an ASCONF-…
> - [Low] The new sock_owned_by_user() deferral branch in
>   sctp_addr_wq_timeout_handler() (net/sctp/protocol.c) sets retry=true…
>
> --
>
> sctp: fix race between addr_wq timer handler and backlog draining
>
> This patch makes sctp_addr_wq_timeout_handler() check sock_owned_by_user()
> on each auto-asconf socket before it calls sctp_asconf_mgmt(). If the
> socket is owned, processing stops and the addr_waitq entry stays queued.
> The addr_wq_timer is then re-armed to retry after SCTP_ADDRESS_TICK_DELAY,
> in a single mod_timer() shared with the existing IPv6 DAD retry.
>
> > 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.
>
> [Severity: Medium]
> Is this description accurate for both event types?
>
> For an SCTP_ADDR_NEW entry, the first pass through
> sctp_send_asconf_add_ip() has already added the address with
> sctp_add_bind_addr(..., SCTP_ADDR_NEW). On retry the lookup finds it and
> skips the association, so no duplicate is sent:
>
> net/sctp/socket.c:sctp_send_asconf_add_ip() {
>     ...
>                         if (sctp_assoc_lookup_laddr(asoc, addr))
>                                 break;
>     ...
>                 if (i < addrcnt)
>                         continue;
>     ...
> }
>
> For an SCTP_ADDR_DEL entry, the first pass in sctp_send_asconf_del_ip()
> only marks the bind address:
>
>                                 if (sctp_cmp_addr_exact(&saddr->a, laddr))
>                                         saddr->state = SCTP_ADDR_DEL;
>
> The entry stays valid until the ASCONF-ACK arrives. sctp_bind_addr_match()
> only tests ->valid, so the !sctp_assoc_lookup_laddr() check still passes.
> Each retry builds another DEL_IP chunk and queues it on addip_chunk_list
> behind the outstanding ASCONF. That is one new chunk per retry, not a
> single duplicate.
>
> Could the commit message describe what actually happens on retry?
>
> > diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> > index 27c26e12f95d8..267dc9b7a0e73 100644
> > --- a/net/sctp/protocol.c
> > +++ b/net/sctp/protocol.c
>
> [ ... ]
>
> > @@ -645,16 +643,27 @@ 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);
> > +                             retry = true;
> > +                             break;
> > +                     }
>
> [Severity: Medium]
> Can a single owned socket stall address event processing for the whole
> netns here?
>
> The first user-owned boundall socket in net->sctp.auto_asconf_splist
> breaks out of both loops. No later socket gets processed, and neither does
> any later addr_waitq entry, including events for unrelated addresses.
>
> Each retry starts again at the head of both lists. Every socket ahead of
> the owned one therefore gets sctp_asconf_mgmt() again on every tick.
>
> User context can hold the socket lock for an unbounded time. For example,
> sctp_recvmsg() holds lock_sock() across skb_copy_datagram_msg(). A copy
> that faults on a FUSE or userfaultfd backed page keeps sk_lock.owned set
> for as long as the user wants.
>
> The other async entry points defer per socket or per association: the
> sctp_rcv() backlog queueing and the timer handlers in sm_sideeffect.c. One
> busy socket does not block the others there.
>
> Would it make sense to skip only this socket and track which sockets
> still need the event, instead of halting all processing?
>
> [Severity: Low]
> This isn't a bug, but the sibling deferral sites in
> net/sctp/sm_sideeffect.c log before they reschedule. One example is
> sctp_generate_heartbeat_event():
>
>         if (sock_owned_by_user(sk)) {
>                 pr_debug("%s: sock is busy\n", __func__);
>
> The DAD retry in this same handler also logs. Should this branch have a
> similar pr_debug() so repeated deferrals show up in dynamic debug?
>
> >                       if (sctp_asconf_mgmt(sp, addrw) < 0)
> >                               pr_debug("%s: sctp_asconf_mgmt failed\n", __func__);
> >                       bh_unlock_sock(sk);
> >               }
> > +             if (retry)
> > +                     break;
>
> [Severity: Medium]
> Does leaving a partly delivered addrw on addr_waitq break the offset logic
> in sctp_addr_wq_mgmt()?
>
> By this point sctp_asconf_mgmt() has run for every boundall socket ahead
> of the owned one, but the entry stays queued after addr_wq_lock is
> dropped. sctp_addr_wq_mgmt() assumes no socket has seen a queued entry
> yet:
>
> net/sctp/protocol.c:sctp_addr_wq_mgmt() {
>     ...
>         /* Offsets existing events in addr_wq */
>         addrw = sctp_addr_wq_lookup(net, addr);
>         if (addrw) {
>                 if (addrw->state != cmd) {
>                         ...
>                         list_del(&addrw->list);
>                         kfree(addrw);
>                 }
>                 spin_unlock_bh(&net->sctp.addr_wq_lock);
>                 return;
>         }
>     ...
> }
>
> Take a partly delivered SCTP_ADDR_NEW entry, and suppose RTM_DELADDR or
> NETDEV_DOWN for the same address arrives before the retry. Both events are
> dropped. The earlier sockets' associations have already sent ADD_IP for
> the address. They keep it in asoc->base.bind_addr after it is gone, and no
> DEL_IP is ever sent.
>
> The reverse happens with a partly delivered SCTP_ADDR_DEL entry if the
> address comes back before the retry. The earlier sockets' associations
> remove the address when the ASCONF-ACK arrives and never re-add it.
>
> Before this patch, the only path that left an entry queued was the IPv6
> DAD break. That break runs before the socket loop, so no socket had seen
> the entry yet.
>
> >  #if IS_ENABLED(CONFIG_IPV6)
> >  free_next:
> >  #endif
> >               list_del(&addrw->list);
> >               kfree(addrw);
> >       }
> > +
> > +     if (retry)
> > +             mod_timer(&net->sctp.addr_wq_timer,
> > +                     jiffies + msecs_to_jiffies(SCTP_ADDRESS_TICK_DELAY));
>
> [Severity: High]
> Can this retry grow kernel memory without bound for an SCTP_ADDR_DEL
> entry?
>
> The retry has no limit and keeps no record of which sockets were already
> handled. Every SCTP_ADDRESS_TICK_DELAY (500 ms), the timer replays the
> same DEL addrw on every boundall socket ahead of the owned one:
>
> sctp_addr_wq_timeout_handler()
>   sctp_asconf_mgmt()
>     sctp_send_asconf_del_ip()
>       sctp_assoc_lookup_laddr()     <- still matches, ->valid is 1
>       sctp_find_unmatch_addr()      <- still returns another address
>       sctp_make_asconf_update_ip()  <- new skb and chunk, GFP_ATOMIC
>       sctp_send_asconf()
>
> While asoc->addip_last_asconf is set, sctp_send_asconf() appends the new
> chunk with no length limit:
>
>         if (asoc->addip_last_asconf) {
>                 list_add_tail(&chunk->list, &asoc->addip_chunk_list);
>                 goto out;
>         }
>
> Each retry also calls sctp_transport_route() on every transport of each
> association. These allocations happen in timer softirq context and are
> not charged to any memcg.
>
> The growth only stops when the owned socket is released or the ASCONF-ACK
> arrives. A peer that drops only ASCONF-ACKs keeps the association alive,
> because heartbeat ACKs and data SACKs reset overall_error_count. So
> sctp_sf_t4_timer_expire() never reaches max_retrans.
>
> A local unprivileged user with user and net namespaces can set this up:
>
>   - enable net.sctp.addip_enable in their own netns
>   - flap an address on a dummy interface
>   - filter ASCONF-ACKs
>   - keep a socket owned through a stalled user copy
>
> Before this patch, one address event produced exactly one DEL ASCONF per
> association.
>
> >       spin_unlock_bh(&net->sctp.addr_wq_lock);
> >  }
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109024219.347985-1-18983559317%40163.com

Looks like we can't re-arm the timer in this case.

Can you try adding a list to struct sctp_sock to store the addr
entries and continue the loop when it occurs, and implement
sctp_release_cb() to handle them?

Thanks.