Re: [PATCH net v2 2/2] mptcp: diag: bound listener bucket lock hold

[zihan xi]

On Mon, Sep 7, 2026 at 12:22 PM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> Thank you for your contribution! Sashiko AI review found 6 potential
> issue(s) to consider:
>
> Critical: 0 · High: 1 · Medium: 3 · Low: 2
>
> - [High] Use-after-free introduced by the new deferred processing loop in
>   mptcp_diag_dump_listeners().
> - [Medium] The patch silently changes which MPTCP listeners the dump
>   reports, and the commit message does not mention it.
> - [Medium] mptcp_diag_dump_listeners() mixes two incompatible counting
>   bases for the resume index diag_ctx->l_num.
> - [Medium] mptcp_diag_dump_listeners() performs all
>   identity/authorization checks on the parent MPTCP socket…
> - [Low] Commit message of "mptcp: diag: bound listener bucket lock hold"
>   carries Fixes: 4fa39b701ce9 + Cc: stable + Reported-by:, but never…
> - [Low] In the new post-lock loop of mptcp_diag_dump_listeners()
>   (net/mptcp/mptcp_diag.c), `tmp = ctx ?
>
> --
>
> mptcp: diag: bound listener bucket lock hold
>
> This reworks mptcp_diag_dump_listeners() so that the listener bucket lock is
> only held while collecting up to MPTCP_DIAG_BULK_SZ referenced listener
> sockets, with the ULP/property re-checks, the parent msk reference and
> sk_diag_dump() moved outside the lock.  A referenced dump cursor is kept in
> cb->data so later batches resume after the previous listener instead of
> rescanning the bucket prefix.
>
> v1 Link: https://lore.kernel.org/all/cover.1785307984.git.zihanx@xxxxxxxxxx/
>
> > diff --git a/include/linux/inet_diag.h b/include/linux/inet_diag.h
> > index 6ccd32bc48f91..4859e77a28c7a 100644
> > --- a/include/linux/inet_diag.h
> > +++ b/include/linux/inet_diag.h
> > @@ -39,6 +39,7 @@ enum inet_diag_dump_cursor_type {
> >       INET_DIAG_DUMP_CURSOR_TCP_LISTEN,
> >       INET_DIAG_DUMP_CURSOR_TCP_BIND,
> >       INET_DIAG_DUMP_CURSOR_TCP_EHASH,
> > +     INET_DIAG_DUMP_CURSOR_MPTCP_LISTEN,
> >  };
>
> [Severity: Low]
> Two questions about the changelog for this one.
>
> First, the message says the lock hold time "can therefore grow with the
> number of sockets visited", but it never states the observed symptom.  Was
> this a soft lockup, an RCU stall, or measured contention on ilb->lock?  Given
> the Fixes: 4fa39b701ce9 and Cc: stable tags plus the Reported-by:, could the
> symptom be spelled out?
>
> Second, could the backport dependency be mentioned?  enum
> inet_diag_dump_cursor_type does not exist in the tree this applies on top of:
>
>   git grep inet_diag_dump_cursor_type <base>   -> no match
>
> The whole enum, cb_data->dump_cursor, dump_cursor_slot, dump_cursor_type and
> inet_diag_dump_clear_cursor() come from the immediately preceding patch of
> this series, "tcp: diag: bound bucket lock hold in tcp_diag_dump()".  Stable
> trees carrying 4fa39b701ce9 cannot take this patch on its own, and nothing in
> the message says so.
>
> > diff --git a/net/mptcp/mptcp_diag.c b/net/mptcp/mptcp_diag.c
> > index 136c2d05c0eeb..37b33ea5d79de 100644
> > --- a/net/mptcp/mptcp_diag.c
> > +++ b/net/mptcp/mptcp_diag.c
> > @@ -12,6 +12,19 @@
> >  #include <net/netlink.h>
> >  #include "protocol.h"
> >
> > +/* Process a bounded number of listeners per bucket lock hold. */
> > +#define MPTCP_DIAG_BULK_SZ 16
> > +
> > +static void mptcp_diag_save_cursor(struct inet_diag_dump_data *cb_data,
> > +                                unsigned int slot, struct sock *sk)
> > +{
> > +     sock_hold(sk);
> > +     inet_diag_dump_clear_cursor(cb_data);
> > +     cb_data->dump_cursor = sk;
> > +     cb_data->dump_cursor_slot = slot;
> > +     cb_data->dump_cursor_type = INET_DIAG_DUMP_CURSOR_MPTCP_LISTEN;
> > +}
> > +
>
> [ ... ]
>
> > @@ -84,64 +98,102 @@ static void mptcp_diag_dump_listeners(struct sk_buff *skb, struct netlink_callba
> >       hinfo = net->ipv4.tcp_death_row.hashinfo;
> >
> >       for (i = diag_ctx->l_slot; i <= hinfo->lhash2_mask; i++) {
> > +             struct sock *tmp, *sk, *sk_arr[MPTCP_DIAG_BULK_SZ];
> >               struct inet_listen_hashbucket *ilb;
> > +             int num_arr[MPTCP_DIAG_BULK_SZ];
> >               struct hlist_nulls_node *node;
> > -             struct sock *sk;
> > -             int num = 0;
> > +             int accum, idx, num, ret;
> > +             struct sock *cursor;
> > +             bool use_cursor;
> >
> > +resume_listen_walk:
> > +             num = 0;
> > +             accum = 0;
> >               ilb = &hinfo->lhash2[i];
> > +             ret = 0;
> >
> >               rcu_read_lock();
> >               spin_lock(&ilb->lock);
> > -             sk_nulls_for_each(sk, node, &ilb->nulls_head) {
> > -                     const struct mptcp_subflow_context *ctx = mptcp_subflow_ctx(sk);
> > -                     struct inet_sock *inet = inet_sk(sk);
> > -                     int ret;
> > -
> > -                     if (num < diag_ctx->l_num)
> > -                             goto next_listen;
> > -
> > -                     if (!ctx || strcmp(inet_csk(sk)->icsk_ulp_ops->name, "mptcp"))
> > -                             goto next_listen;
> > -
> > -                     sk = ctx->conn;
> > -                     if (!sk || !net_eq(sock_net(sk), net))
> > -                             goto next_listen;
> > -
> > -                     if (r->sdiag_family != AF_UNSPEC &&
> > -                         sk->sk_family != r->sdiag_family)
> > -                             goto next_listen;
> > -
> > -                     if (r->id.idiag_sport != inet->inet_sport &&
> > -                         r->id.idiag_sport)
> > +             cursor = cb_data->dump_cursor;
> > +             use_cursor = cursor &&
> > +                          cb_data->dump_cursor_type ==
> > +                          INET_DIAG_DUMP_CURSOR_MPTCP_LISTEN &&
> > +                          cb_data->dump_cursor_slot == i &&
> > +                          inet_sk_state_load(cursor) == TCP_LISTEN &&
> > +                          !hlist_nulls_unhashed(&cursor->sk_nulls_node) &&
> > +                          cursor->sk_nulls_node.pprev != LIST_POISON2 &&
> > +                          inet_lhash2_bucket_sk(hinfo, cursor) == ilb;
> > +             node = use_cursor ? cursor->sk_nulls_node.next :
> > +                                 ilb->nulls_head.first;
> > +             hlist_nulls_for_each_entry_from(sk, node, sk_nulls_node) {
> > +                     if (!use_cursor && num < diag_ctx->l_num)
> >                               goto next_listen;
>
> [Severity: Medium]
> Are the two resume mechanisms here using the same counting base for
> diag_ctx->l_num?
>
> This test counts entries from the bucket head, but at resume_listen_walk num
> is reset to 0 and, when use_cursor is true, the walk starts at
> cursor->sk_nulls_node.next, i.e. mid-bucket.  The values stashed in num_arr[]
> and later stored are therefore cursor-relative and bounded by 0..15:
>
>         if (ret >= 0) {
>                 ret = sk_diag_dump(tmp, skb, cb, r, net_admin);
>                 if (ret < 0)
>                         num = num_arr[idx];
>         }
>         ...
>         if (ret < 0) {
>                 diag_ctx->l_slot = i;
>                 diag_ctx->l_num = num;
>                 return;
>         }
>
> Walking a bucket with more than 16 matching listeners:
>
>   batch 1: 16 listeners dumped, cursor saved on e15
>   goto resume_listen_walk with diag_ctx->l_num = 0, use_cursor true, so num
>     now counts from e16
>   skb fills at idx 5, so diag_ctx->l_num = 5 while the real bucket offset
>     is 21
>   the cursor socket is closed before the next recvmsg(), use_cursor becomes
>     false, and the walk resumes at bucket position 5
>
> Does that re-emit e5..e19, and, if entries were removed in the meantime and
> positions shifted, silently drop listeners that were never reported?
>
> The peer implementation of the same cursor protocol from the preceding patch,
> tcp_diag_dump(), zeroes the positional counter when the cursor is unusable:
>
>         node = use_cursor ? cursor->sk_nulls_node.next : ilb->nulls_head.first;
>         if (!use_cursor)
>                 s_num = 0;
>
> Should mptcp_diag_dump_listeners() do the same so the two bases are never
> mixed?  The intra-call restart also forces diag_ctx->l_num = 0 before goto
> resume_listen_walk, so a cursor invalidated between lock drops restarts the
> bucket at the head within a single dump call.
>
> >                       if (!refcount_inc_not_zero(&sk->sk_refcnt))
> >                               goto next_listen;
> >
> > -                     ret = sk_diag_dump(sk, skb, cb, r, net_admin);
> > -
> > -                     sock_put(sk);
> > -
> > -                     if (ret < 0) {
> > -                             spin_unlock(&ilb->lock);
> > -                             rcu_read_unlock();
> > -                             diag_ctx->l_slot = i;
> > -                             diag_ctx->l_num = num;
> > -                             return;
> > -                     }
> > -                     diag_ctx->l_num = num + 1;
> > -                     num = 0;
>
> [Severity: Medium]
> Removing this "l_num = num + 1; num = 0;" pair changes which listeners the
> dump reports, and the changelog does not mention it.
>
> In the pre-patch loop the skip test was "if (num < diag_ctx->l_num)" while
> num was reset to 0 after every successful dump and l_num kept growing, so for
> a bucket e0..e6 the emitted set was e0, e1, e3, e6:
>
>   e0 dumped -> l_num = 1, num = 0 then ++num = 1
>   e1 dumped -> l_num = 2, num = 1
>   e2 hits 1 < 2 and is skipped
>   e3 dumped -> l_num = 3
>   e4, e5 skipped
>
> The new code keeps num monotonic and applies the skip test only in the
> non-cursor fallback, so every collected listener is processed.  Is that the
> actual user-visible defect being fixed here?  ss -Ml output changes (more
> listeners reported) on any host with three or more MPTCP listeners hashing
> into the same lhash2 bucket, and this seems worth stating in the message,
> along with whether it belongs in the same patch as the lock-hold change.
>
> > +                     num_arr[accum] = num;
> > +                     sk_arr[accum] = sk;
> > +                     if (++accum == MPTCP_DIAG_BULK_SZ)
> > +                             break;
> >  next_listen:
> >                       ++num;
> >               }
> >               spin_unlock(&ilb->lock);
> >               rcu_read_unlock();
> >
> > +             for (idx = 0; idx < accum; idx++) {
> > +                     const struct mptcp_subflow_context *ctx;
> > +                     const struct tcp_ulp_ops *ulp_ops;
> > +                     struct inet_sock *inet;
> > +
> > +                     sk = sk_arr[idx];
> > +                     rcu_read_lock();
> > +                     ctx = rcu_dereference(inet_csk(sk)->icsk_ulp_data);
> > +                     ulp_ops = READ_ONCE(inet_csk(sk)->icsk_ulp_ops);
> > +                     inet = inet_sk(sk);
> > +                     tmp = ctx ? ctx->conn : NULL;
>
> [Severity: High]
> Can this read freed memory?  The rcu_read_lock() here is entered after
> spin_unlock(&ilb->lock) and rcu_read_unlock() above, so it cannot extend a
> grace period that already started.
>
> The collection loop only took sk_refcnt on the listener subflow, which does
> not keep the ULP context alive:
>
>   __tcp_close()
>     tcp_set_state(sk, TCP_CLOSE)
>     inet_unhash(sk)               /* takes ilb->lock, free once diag drops it */
>     inet_csk_destroy_sock()
>       sk->sk_prot->destroy(sk)
>         tcp_v4_destroy_sock()
>           tcp_cleanup_ulp()
>             subflow_ulp_release()
>
> and net/mptcp/subflow.c:subflow_ulp_release() drops the msk reference and
> frees the context:
>
>         sk = ctx->conn;
>         if (sk) {
>                 ...
>                 sock_put(sk);
>         }
>         mptcp_subflow_ops_undo_override(ssk);
>         if (release)
>                 kfree_rcu(ctx, rcu);
>
> net/ipv4/tcp_ulp.c:tcp_cleanup_ulp() clears only icsk_ulp_ops:
>
>         if (icsk->icsk_ulp_ops->release)
>                 icsk->icsk_ulp_ops->release(sk);
>         module_put(icsk->icsk_ulp_ops->owner);
>
>         icsk->icsk_ulp_ops = NULL;
>
> icsk_ulp_data is never unpublished on this path (subflow_ulp_fallback() does
> rcu_assign_pointer(icsk->icsk_ulp_data, NULL), this path does not).  If
> kfree_rcu(ctx) was queued before this loop iteration re-entered
> rcu_read_lock(), the grace period does not cover this reader, so
> rcu_dereference(inet_csk(sk)->icsk_ulp_data) can return a freed and reused
> slab object, and ctx->conn is dereferenced before the !ulp_ops guard is even
> evaluated.
>
> The loop is also preemptible for up to 15 preceding sk_diag_dump() /
> inet_diag_bc_sk() invocations with request-supplied bytecode, which widens the
> window.  Does the commit message claim ("After dropping the lock, read
> icsk_ulp_data with rcu_dereference()") hold here, or do ctx and the parent msk
> reference have to be acquired while ilb->lock is still held, as the pre-patch
> code did?
>
> [Severity: Low]
> Separately, is it intentional that ctx->conn is loaded before the check that
> the ULP is actually "mptcp"?  The collection loop under ilb->lock no longer
> filters by ULP at all, so listeners with a foreign ULP context now reach this
> loop, and the read happens at offsetof(struct mptcp_subflow_context, conn)
> inside an object of an unrelated type.
>
> net/xfrm/espintcp.c:espintcp_init_sk() publishes its own object with
> rcu_assign_pointer(icsk->icsk_ulp_data, ctx) and, unlike tls_init(), has no
> TCP-state restriction, so an unprivileged setsockopt(TCP_ULP, "espintcp")
> followed by listen() puts such a socket into lhash2.
>
> struct espintcp_ctx is larger than that offset today, so the read stays inside
> the allocation and the loaded value is discarded by the failing strcmp().
> Still, the pre-patch code did the strcmp() first and never touched a foreign
> context.  Could the ctx->conn load be moved after the ULP name check?
>
> > +                     if (!ctx || !ulp_ops || strcmp(ulp_ops->name, "mptcp") ||
> > +                         !tmp || !net_eq(sock_net(tmp), net) ||
> > +                         (r->sdiag_family != AF_UNSPEC &&
> > +                          tmp->sk_family != r->sdiag_family) ||
> > +                         (r->id.idiag_sport != inet->inet_sport &&
> > +                          r->id.idiag_sport) ||
> > +                         !refcount_inc_not_zero(&tmp->sk_refcnt)) {
> > +                             rcu_read_unlock();
> > +                             goto processed_listener_sk;
> > +                     }
> > +                     rcu_read_unlock();
> > +                     if (ret >= 0) {
> > +                             ret = sk_diag_dump(tmp, skb, cb, r, net_admin);
> > +                             if (ret < 0)
> > +                                     num = num_arr[idx];
> > +                     }
> > +                     sock_put(tmp);
>
> [Severity: Medium]
> Does this need a re-validation after refcount_inc_not_zero(&tmp->sk_refcnt)?
>
> All the identity and scoping checks on the parent msk (net_eq(sock_net(tmp),
> net), sdiag_family, idiag_sport) run before the reference is taken, and
> nothing is re-checked afterwards.  mptcp_prot uses SLAB_TYPESAFE_BY_RCU:
>
> net/mptcp/protocol.c:
>         .slab_flags     = SLAB_TYPESAFE_BY_RCU,
>
> so rcu_read_lock() only guarantees type stability, not identity.  If
> subflow_ulp_release() already dropped the last reference via
> sock_put(ctx->conn), the object can be recycled as a different MPTCP socket
> inside this reader's RCU section, refcount_inc_not_zero() then succeeds, and
> sk_diag_dump(tmp, ...) serializes that socket's addresses, ports, uid, cookie
> and state into the reply, possibly from another namespace.
>
> The in-tree pattern for this slab, net/mptcp/token.c:mptcp_token_get_sock(),
> re-checks after the acquisition:
>
>         if (!refcount_inc_not_zero(&sk->sk_refcnt))
>                 goto not_found;
>
>         if (READ_ONCE(msk->token) != token ||
>             !net_eq(sock_net(sk), net)) {
>
> Should the same re-check be done here?  Pre-patch the conn read and the msk
> refcount acquisition both happened under ilb->lock with the subflow still
> hashed, so the reference held by ctx was still alive.
>
> > +processed_listener_sk:
> > +                     if (ret >= 0)
> > +                             mptcp_diag_save_cursor(cb_data, i, sk);
> > +                     sock_put(sk);
> > +             }
> > +
> > +             if (ret < 0) {
> > +                     diag_ctx->l_slot = i;
> > +                     diag_ctx->l_num = num;
> > +                     return;
> > +             }
> > +
> >               cond_resched();
> > +
> > +             if (accum == MPTCP_DIAG_BULK_SZ) {
> > +                     diag_ctx->l_num = 0;
> > +                     goto resume_listen_walk;
> > +             }
> > +
> > +             inet_diag_dump_clear_cursor(cb_data);
> >               diag_ctx->l_num = 0;
> >       }
> >
> > +     inet_diag_dump_clear_cursor(cb_data);
> >       diag_ctx->l_num = 0;
> >       diag_ctx->l_slot = i;
> >  }
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1788248601.git.zihanx%40nebusec.ai

- UAF on icsk_ulp_data/ctx after dropping ilb->lock: valid
- mixing cursor-relative and absolute l_num: valid
- dropping "l_num = num + 1; num = 0" changes the reported set:
  valid as a behavior change, not a regression
- no re-check after msk refcount_inc_not_zero(): valid for the
  lockless path
- changelog/stable dependency on 0001: valid as documentation
- loading ctx->conn before the ULP name check: valid as ordering,
  not currently an OOB