Re: [PATCH net v2 1/2] tcp: diag: bound bucket lock hold in tcp_diag_dump()
From: zihan xi
Date: Mon Sep 07 2026 - 00:55:23 EST
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: 2 · High: 1 · Medium: 2 · Low: 1
>
> - [Critical] Use-after-free / wild-pointer traversal: the new bind-hash
> dump cursor revalidation in tcp_diag_dump() cannot detect that the…
> - [Critical] Refcount initialization race: tcp_diag_bind_collect_sock()
> calls sock_hold() unconditionally on every node of tb2->owners,…
> - [High] Loss of dump forward progress and duplicate output: the patch
> deletes every positional (`s_num`) resume path in tcp_diag_dump()…
> - [Medium] Lockless, unordered reads of cursor state: the listen and
> ehash cursor validations establish "this socket is in the bucket I…
> - [Medium] The commit message mischaracterizes the pre-existing defect.
> - [Low] Concern that the sibling MPTCP listener dump
> (mptcp_diag_dump_listeners()) keeps evaluating request-supplied…
>
> --
>
> tcp: diag: bound bucket lock hold in tcp_diag_dump()
>
> This reworks the three tcp_diag_dump() hash walks so that only referenced
> sockets are collected under each bucket lock, and moves the family/port
> filters, inet_diag_bc_sk() bytecode evaluation and the fill work outside the
> critical section. A referenced dump cursor is added to
> struct inet_diag_dump_data so a following batch resumes after the previously
> reported socket, with per-table validation of the cursor.
>
> v1 Link: https://lore.kernel.org/all/cover.1785307984.git.zihanx@xxxxxxxxxx/
>
> > inet_diag dumps run request-supplied bytecode through inet_diag_bc_sk().
> > tcp_diag_dump() currently evaluates socket filters and fills replies while
> > holding the listener, bind, and ehash bucket locks.
>
> [Severity: Medium]
> Is this description accurate for all three tables?
>
> At the baseline (544d85de4dc2) only the listener walk fills replies under
> ilb->lock. The bind walk already collected at most SKARR_SZ = 16 referenced
> sockets under spin_lock_bh(&ibb->lock) and only called inet_sk_diag_fill()
> after the lock was dropped:
>
> pause_bind_walk:
> spin_unlock_bh(&ibb->lock);
>
> res = 0;
> for (idx = 0; idx < accum; idx++) {
> if (res >= 0) {
> res = inet_sk_diag_fill(sk_arr[idx],
> NULL, skb, cb,
> r, NLM_F_MULTI,
> net_admin);
>
> The ehash walk did the same with sk_diag_fill() after spin_unlock_bh(lock).
> What remained under the bind and ehash locks was inet_diag_bc_sk() plus the
> cheap family/port comparisons, bounded to 16 sockets per lock hold.
>
> Could the changelog be narrowed to the listener walk, or state explicitly
> what part of the bind/ehash work it is moving out?
>
> > diff --git a/include/linux/inet_diag.h b/include/linux/inet_diag.h
> > index 704fd415c2b49..6ccd32bc48f91 100644
> > --- a/include/linux/inet_diag.h
> > +++ b/include/linux/inet_diag.h
>
> [ ... ]
>
> > @@ -32,12 +33,23 @@ struct inet_diag_handler {
> > };
> >
> > struct bpf_sk_storage_diag;
> > +
> > +enum inet_diag_dump_cursor_type {
> > + INET_DIAG_DUMP_CURSOR_NONE,
> > + INET_DIAG_DUMP_CURSOR_TCP_LISTEN,
> > + INET_DIAG_DUMP_CURSOR_TCP_BIND,
> > + INET_DIAG_DUMP_CURSOR_TCP_EHASH,
> > +};
> > +
>
> [Severity: Low]
> This isn't a bug, but a note for backporters. The sibling MPTCP listener
> dump mptcp_diag_dump_listeners() is converted by the follow-up patch in this
> series (INET_DIAG_DUMP_CURSOR_MPTCP_LISTEN, which is why the enum slot and
> the exported inet_diag_dump_clear_cursor() appear here), so at the tip of the
> series the MPTCP listener walk also drops ilb->lock before calling
> sk_diag_dump()/inet_diag_bc_sk().
>
> This patch is Cc: stable, and the commit message body does not mention that
> the MPTCP listener walk keeps running request-supplied bytecode under the
> same hashinfo->lhash2 bucket lock until the follow-up patch is applied. Only
> the v2 changelog, which is stripped on apply, mentions "Left
> INET_DIAG_DUMP_CURSOR_MPTCP_LISTEN to the MPTCP patch". Could that
> dependency be stated in the commit message?
>
> [ ... ]
>
> > diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
> > index 6e2fe186d0dcb..d95639ac70c67 100644
> > --- a/include/net/inet_hashtables.h
> > +++ b/include/net/inet_hashtables.h
> > @@ -188,6 +188,24 @@ inet_lhash2_bucket(struct inet_hashinfo *h, u32 hash)
> > return &h->lhash2[hash & h->lhash2_mask];
> > }
> >
> > +static inline struct inet_listen_hashbucket *
> > +inet_lhash2_bucket_sk(struct inet_hashinfo *h, struct sock *sk)
> > +{
> > + u32 hash;
> > +
> > +#if IS_ENABLED(CONFIG_IPV6)
> > + if (sk->sk_family == AF_INET6)
> > + hash = ipv6_portaddr_hash(sock_net(sk),
> > + &sk->sk_v6_rcv_saddr,
> > + inet_sk(sk)->inet_num);
> > + else
> > +#endif
> > + hash = ipv4_portaddr_hash(sock_net(sk),
> > + inet_sk(sk)->inet_rcv_saddr,
> > + inet_sk(sk)->inet_num);
> > + return inet_lhash2_bucket(h, hash);
> > +}
> > +
>
> [ ... ]
>
> > diff --git a/net/ipv4/tcp_diag.c b/net/ipv4/tcp_diag.c
> > index ba1fdbe9807fd..842e13ee79e5b 100644
> > --- a/net/ipv4/tcp_diag.c
> > +++ b/net/ipv4/tcp_diag.c
> > @@ -285,6 +285,73 @@ static int sk_diag_fill(struct sock *sk, struct sk_buff *skb,
> > net_admin);
> > }
> >
>
> [ ... ]
>
> > +static struct inet_bind2_bucket *tcp_diag_sk_bind2(const struct sock *sk)
> > +{
> > + if (sk->sk_state == TCP_TIME_WAIT)
> > + return inet_twsk(sk)->tw_tb2;
> > +
> > + return inet_csk(sk)->icsk_bind2_hash;
> > +}
> > +
> > +static bool tcp_diag_bind_collect_sock(struct sock *sk, struct sock **sk_arr,
> > + int *num_arr, int *accum, int num)
> > +{
> > + sock_hold(sk);
> > + num_arr[*accum] = num;
> > + sk_arr[*accum] = sk;
> > +
> > + return ++*accum == SKARR_SZ;
> > +}
> > +
> > +static bool tcp_diag_bind_collect_owners(struct hlist_head *owners,
> > + struct sock **sk_arr, int *num_arr,
> > + int *accum, int *num, int s_num)
> > +{
> > + struct sock *sk;
> > +
> > + sk_for_each_bound(sk, owners) {
> > + if (*num < s_num) {
> > + (*num)++;
> > + continue;
> > + }
> > +
> > + if (tcp_diag_bind_collect_sock(sk, sk_arr, num_arr, accum, *num))
> > + return true;
> > + (*num)++;
> > + }
> > +
> > + return false;
> > +}
>
> [Severity: Critical]
> Can the unconditional sock_hold() in tcp_diag_bind_collect_sock() hit a
> TIME_WAIT socket whose tw_refcnt is still 0?
>
> The changelog says "Bind collection counts TIME_WAIT nodes toward the batch
> limit", and tcp_diag_bind_collect_owners() no longer filters on
> sk->sk_state != TCP_CLOSE before taking the reference, so every node of
> tb2->owners now gets a refcount_inc().
>
> inet_twsk_alloc() starts the object at zero on purpose:
>
> /*
> * Because we use RCU lookups, we should not set tw_refcnt
> * to a non null value before everything is setup for this
> * timewait socket.
> */
> refcount_set(&tw->tw_refcnt, 0);
>
> and inet_twsk_hashdance_schedule() publishes it into the bind owners list and
> drops both bind locks before initializing the count:
>
> sk_add_bind_node((struct sock *)tw, &tw->tw_tb2->owners);
>
> spin_unlock(&bhead2->lock);
> spin_unlock(&bhead->lock);
>
> spin_lock(lock);
>
> refcount_set(&tw->tw_refcnt, 3);
>
> A dumper blocked on bhead2->lock acquires it right after that unlock and
> immediately walks tb2->owners, so it can observe the published tw with
> tw_refcnt == 0. sock_hold() then calls refcount_inc() on zero, which trips
> refcount_warn_saturate() and leaves the counter saturated, after which
> refcount_set(&tw->tw_refcnt, 3) discards the dump's reference. The later
> sock_gen_put() from the fill loop then consumes one of the three structural
> references (bhash, ehash, timer).
>
> Would using refcount_inc_not_zero() here, or keeping the TCP_CLOSE check
> under the lock, be safer?
>
> > @@ -335,8 +402,15 @@ static void tcp_diag_dump(struct sk_buff *skb, struct netlink_callback *cb,
> > for (i = s_i; i <= hashinfo->lhash2_mask; i++) {
> > struct inet_listen_hashbucket *ilb;
> > struct hlist_nulls_node *node;
> > + struct sock *sk_arr[SKARR_SZ];
> > + int num_arr[SKARR_SZ];
> > + struct sock *cursor;
> > + int idx, accum, res;
> > + bool use_cursor;
> >
> > +resume_listen_walk:
> > num = 0;
> > + accum = 0;
> > ilb = &hashinfo->lhash2[i];
> >
> > if (hlist_nulls_empty(&ilb->nulls_head)) {
> > @@ -344,52 +418,81 @@ static void tcp_diag_dump(struct sk_buff *skb, struct netlink_callback *cb,
> > continue;
> > }
> > spin_lock(&ilb->lock);
> > - sk_nulls_for_each(sk, node, &ilb->nulls_head) {
> > - struct inet_sock *inet = inet_sk(sk);
> > + cursor = cb_data->dump_cursor;
> > + use_cursor = cursor &&
> > + cb_data->dump_cursor_type ==
> > + INET_DIAG_DUMP_CURSOR_TCP_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(hashinfo, cursor) == ilb;
> > + node = use_cursor ? cursor->sk_nulls_node.next :
> > + ilb->nulls_head.first;
>
> [Severity: Medium]
> Is the "cursor is still in this bucket" conclusion sound when every input to
> it is read without the lock that publishes it?
>
> In the listener path the recomputation uses sock_net(cursor),
> sk_v6_rcv_saddr (a 16-byte non-atomic read) and inet_sk(cursor)->inet_num via
> the newly exported inet_lhash2_bucket_sk(), none of which is protected by
> ilb->lock, and the pprev test and the bucket computation are separate plain
> loads with no barrier between them.
>
> The ehash variant later in this function has the same shape with
> cursor->sk_nulls_node.pprev followed by cursor->sk_hash. The insertion side
> sets sk->sk_hash before taking the target bucket lock and linking with
> __sk_nulls_add_node_rcu(), and inet_csk_listen_start() stores TCP_LISTEN
> before hashing into lhash2, so a reader that observes the new linkage is not
> guaranteed to observe the matching sk_hash/sk_state store. A socket that
> left ehash bucket i and called listen() keeps its old sk_hash, so
> inet_sk_state_load(cursor) != TCP_LISTEN can still be true while the lhash2
> linkage is already visible.
>
> If validation passes on a stale snapshot, the walk then follows
> cursor->sk_nulls_node.next into a chain whose spinlock is not held and
> sock_hold()s the entries it finds. On x86 the store order makes the
> inconsistent snapshot hard to observe, but is bucket membership something
> that can be established from lockless per-socket fields at all?
>
> > + if (!use_cursor)
> > + s_num = 0;
> > + hlist_nulls_for_each_entry_from(sk, node, sk_nulls_node) {
> >
> > - if (!net_eq(sock_net(sk), net))
> > - continue;
> > + sock_hold(sk);
> > + num_arr[accum] = num;
> > + sk_arr[accum] = sk;
> > + if (++accum == SKARR_SZ)
> > + break;
> >
> > - if (num < s_num) {
> > - num++;
> > - continue;
> > - }
> > + ++num;
> > + }
> > + spin_unlock(&ilb->lock);
>
> [Severity: High]
> With the num < s_num skip removed from this loop and s_num forced to 0 when
> the cursor is rejected, what stops the bucket from being re-dumped from its
> head?
>
> All three walks now do:
>
> if (!use_cursor)
> s_num = 0;
>
> and the batch continuations replace s_num = num + 1 with s_num = 0:
>
> if (accum == SKARR_SZ) {
> s_num = 0;
> goto resume_listen_walk;
> }
>
> The bind walk and the ehash walk have the same two changes, and the s_num
> parameter of tcp_diag_bind_collect_owners() is dead because its only caller
> sits in the !use_cursor branch where s_num has just been zeroed.
>
> So whenever cursor validation fails, which the changelog describes as the
> "safe restart on mismatch", the walk restarts at ilb->nulls_head.first, at
> the head of ibb->chain, or at head->chain.first, and re-emits sockets from
> that bucket that were already sent to user space earlier in the same dump.
> Before this change s_num = num + 1 guaranteed the restart advanced.
>
> There is a second effect: if all of the re-collected leading SKARR_SZ sockets
> are discarded after the lock is dropped (foreign netns, since lhash2 and
> ehash are shared across netns, or sdiag_family/port mismatch, or bytecode
> rejection), nothing is emitted, res stays 0 and the goto resume_*_walk loop
> repeats over the same head with only cond_resched() yielding.
>
> tcp_diag_dump() still loads s_num = num = cb->args[2] on entry and still
> stores cb->args[2] = num at the done: label, but num is now a chunk-relative
> counter that no resume path consumes. Should that state be dropped, or
> should a positional fallback be kept for the mismatch case? Also, since the
> walk can now repeat objects within a dump, should NLM_F_DUMP_INTR be set via
> a generation counter in netlink_callback.seq?
>
> [ ... ]
>
> > @@ -412,34 +517,46 @@ static void tcp_diag_dump(struct sk_buff *skb, struct netlink_callback *cb,
> > continue;
> > }
> > spin_lock_bh(&ibb->lock);
> > - inet_bind_bucket_for_each(tb2, &ibb->chain) {
> > - if (!net_eq(ib2_net(tb2), net))
> > - continue;
> > -
> > - sk_for_each_bound(sk, &tb2->owners) {
> > - struct inet_sock *inet = inet_sk(sk);
> > -
> > - if (num < s_num)
> > - goto next_bind;
> > -
> > - if (sk->sk_state != TCP_CLOSE ||
> > - !inet->inet_num)
> > - goto next_bind;
> > -
> > - if (r->sdiag_family != AF_UNSPEC &&
> > - r->sdiag_family != sk->sk_family)
> > - goto next_bind;
> > -
> > - if (!inet_diag_bc_sk(cb_data, sk))
> > - goto next_bind;
> > -
> > - sock_hold(sk);
> > - num_arr[accum] = num;
> > - sk_arr[accum] = sk;
> > - if (++accum == SKARR_SZ)
> > + cursor = cb_data->dump_cursor;
> > + use_cursor = cursor &&
> > + cb_data->dump_cursor_type ==
> > + INET_DIAG_DUMP_CURSOR_TCP_BIND &&
> > + cb_data->dump_cursor_slot == i &&
> > + !hlist_unhashed(&cursor->sk_bind_node) &&
> > + cursor->sk_bind_node.pprev != LIST_POISON2;
> > + if (use_cursor) {
> > + tb2 = tcp_diag_sk_bind2(cursor);
> > + use_cursor = tb2 &&
> > + inet_bhashfn_portaddr(hashinfo, cursor,
> > + sock_net(cursor),
> > + inet_sk(cursor)->inet_num) ==
> > + ibb;
> > + }
> > + if (!use_cursor)
> > + s_num = 0;
> > + if (use_cursor) {
> > + sk = cursor;
> > + if (tcp_diag_bind_collect_owners_continue(sk, sk_arr,
> > + num_arr,
> > + &accum,
> > + &num))
> > + goto pause_bind_walk;
> > + hlist_for_each_entry_continue(tb2, node) {
> > + if (tcp_diag_bind_collect_owners(&tb2->owners,
> > + sk_arr,
> > + num_arr,
> > + &accum,
> > + &num, 0))
> > + goto pause_bind_walk;
> > + }
>
> [Severity: Critical]
> Can these two liveness checks ever detect that the cursor left
> tb2->owners, and can tb2 here already be freed?
>
> Removal from a bind chain goes through __sk_del_bind_node():
>
> include/net/sock.h:
> static inline void __sk_del_bind_node(struct sock *sk)
> {
> __hlist_del(&sk->sk_bind_node);
> }
>
> __hlist_del() only writes *pprev and next->pprev; it leaves the removed
> node's own pprev and next untouched. So for a socket that was unlinked,
> hlist_unhashed(&cursor->sk_bind_node) is false and
> cursor->sk_bind_node.pprev != LIST_POISON2 holds, and both tests pass.
>
> The freed-bucket part comes from __inet_bhash2_update_saddr():
>
> net/ipv4/inet_hashtables.c:
> spin_lock(&head2->lock);
> __sk_del_bind_node(sk);
> inet_bind2_bucket_destroy(hinfo->bind2_bucket_cachep, inet_csk(sk)->icsk_bind2_hash);
> spin_unlock(&head2->lock);
>
> if (reset)
> inet_reset_saddr(sk);
> else
> inet_update_saddr(sk, saddr, family);
>
> icsk_bind2_hash is never cleared, and inet_bind2_bucket_destroy() frees the
> bucket immediately with no grace period when it was the sole owner:
>
> if (hlist_empty(&tb->owners)) {
> __hlist_del(&tb->node);
> __hlist_del(&tb->bhash_node);
> kmem_cache_free(cachep, tb);
> return;
> }
>
> In the window between that spin_unlock() and the address update, the address
> is unchanged, so inet_bhashfn_portaddr() still resolves to ibb, and
> tcp_diag_sk_bind2(cursor) returns the dangling icsk_bind2_hash. use_cursor
> therefore becomes true.
>
> tcp_diag_bind_collect_owners_continue() then iterates the cursor's stale
> sk_bind_node.next, and hlist_for_each_entry_continue(tb2, node) reads
> tb2->node.next out of the freed slab object, producing an arbitrary
> struct inet_bind2_bucket * whose owners list is walked with sock_hold()
> applied to each apparent entry.
>
> Note the dumper holds only hashinfo->bhash2[i].lock and never the primary
> bhash head lock, and a cursor is saved for every collected owner including
> the ones filtered out afterwards, so a process that dumps with
> TCPF_BOUND_INACTIVE while calling connect() on a bound socket in that bucket
> can reach this. Could the cursor instead record a validated tb2 identity
> that is re-looked-up under ibb->lock, rather than being derived from
> icsk_bind2_hash?
>
> > + } else {
> > + inet_bind_bucket_for_each(tb2, &ibb->chain) {
> > + if (tcp_diag_bind_collect_owners(&tb2->owners,
> > + sk_arr,
> > + num_arr,
> > + &accum,
> > + &num, s_num))
> > goto pause_bind_walk;
> > -next_bind:
> > - num++;
> > }
> > }
> > pause_bind_walk:
>
> [ ... ]
>
> > @@ -463,13 +598,15 @@ static void tcp_diag_dump(struct sk_buff *skb, struct netlink_callback *cb,
> > cond_resched();
> >
> > if (accum == SKARR_SZ) {
> > - s_num = num + 1;
> > + s_num = 0;
> > goto resume_bind_walk;
> > }
> >
> > + inet_diag_dump_clear_cursor(cb_data);
> > s_num = 0;
> > }
>
> [ ... ]
>
> > if (accum == SKARR_SZ) {
> > - s_num = num + 1;
> > + s_num = 0;
> > goto next_chunk;
> > }
>
> [ ... ]
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1788248601.git.zihanx%40nebusec.ai
- bind cursor UAF / dangling tb2: valid
- unconditional sock_hold() on unpublished TIME_WAIT: valid
- clearing s_num on cursor mismatch: valid
- lockless cursor-membership checks: valid as a concern
- commit message overstating bind/ehash fill-under-lock: valid
- missing note that MPTCP needs the follow-up patch: valid as a
backport note
- endless re-scan of the same bucket head: not valid as a
guaranteed livelock; duplicate output after a rejected cursor
is valid