Re: [PATCH bpf v2] bpf: tcp: Fix use-after-free in bpf_iter_tcp_established_batch()

[Kuniyuki Iwashima]

On Fri, Jul 17, 2026 at 4:34 AM Jose Fernandez (Anthropic)
<jose.fernandez@xxxxxxxxx> wrote:
>
> reqsk_queue_hash_req() publishes a TCP_NEW_SYN_RECV request_sock onto
> the ehash chain, drops the bucket lock, and only afterwards sets
> rsk_refcnt to 3.
>
> Lockless readers such as __inet_lookup_established() handle this with
> refcount_inc_not_zero(), but bpf_iter_tcp_established_batch() uses plain
> sock_hold() while holding the bucket lock, on the assumption that the
> lock guarantees sk_refcnt > 0. That assumption does not hold for
> request_sock:
>
>   CPU 0                                CPU 1
>   -----                                -----
>   tcp_conn_request()
>    reqsk_queue_hash_req()
>     inet_ehash_insert(req)
>      spin_lock(bucket)
>      __sk_nulls_add_node_rcu(req)      // rsk_refcnt == 0
>      spin_unlock(bucket)
>                                        bpf_iter_tcp_established_batch()
>                                         spin_lock(bucket)
>                                         sock_hold(req)   <-- addition on 0
>                                         spin_unlock(bucket)
>     refcount_set(&req->rsk_refcnt, 3)  // clobbers saturated value
>
> which surfaces as:
>
>   refcount_t: addition on 0; use-after-free.
>   WARNING: lib/refcount.c:25 at refcount_warn_saturate+0x48/0x90, CPU#1
>   Call Trace:
>    bpf_iter_tcp_established_batch+0x14e/0x170
>    bpf_iter_tcp_batch+0x53/0x200
>    bpf_iter_tcp_seq_next+0x27/0x70
>    bpf_seq_read+0x107/0x410
>    vfs_read+0xb9/0x380
>
> The iterator's stolen reference is lost when the publishing CPU's
> refcount_set() overwrites the count, leaving the socket one reference
> short. When the last legitimate owner drops its reference the reqsk is
> freed while still reachable, leading to use-after-free.
>
> This reproduces in seconds with tcp_syncookies=0, a handful of threads
> doing connect()/close() to a local listener while others read an
> iter/tcp link in a tight loop.
>
> Use refcount_inc_not_zero() and skip the socket on failure. A skipped
> socket is still part of the bucket, so keep counting it in expected.
> The reallocations are sized from expected, and a request sock whose
> refcount gets published while the lock is held across the last realloc
> must already have room.
>
> A skipped socket is counted in expected but never batched, so end_sk
> can be short of expected on a batch that is actually complete. Decide
> completeness by whether the walk left any socket behind instead. The
> WARN after the locked realloc checks the same, replacing an
> end_sk == expected check that could not hold on that path since
> cdec67a489d4.

nit: please use the cacnonical format

commit cdec67a489d4 ("bpf: tcp: Make sure iter->batch always
contains a full bucket snapshot")


>
> If every matching socket in a bucket is mid-init (refcount 0), end_sk
> stays 0. Advance to the next bucket rather than returning a batch entry
> that was never filled this round.
>
> Fixes: 04c7820b776f ("bpf: tcp: Bpf iter batching and lock_sock")
> Assisted-by: Claude:unspecified
> Signed-off-by: Jose Fernandez (Anthropic) <jose.fernandez@xxxxxxxxx>
> ---
> Changes in v2:
> - Count expected right after seq_sk_match() so the batch reallocations
>   are sized for the whole bucket, including request socks whose
>   refcount is not yet published (Kuniyuki)
> - Signal batch completeness by the walk leaving no leftover socket
>   instead of end_sk == expected, and check the same condition in the
>   WARN after the locked reallocation
> - Drop the Reviewed-by tags given the code changes
> - Rebase onto bpf/master
> - Link to v1: https://lore.kernel.org/bpf/20260620-bpf-iter-tcp-refcnt-v1-1-883bf9e69495@xxxxxxxxx
>
> The pre-existing double-put on the realloc failure path (raised in the
> v1 thread) will be addressed in a separate follow-up patch.
> ---
>  net/ipv4/tcp_ipv4.c | 44 +++++++++++++++++++++++++-------------------
>  1 file changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index 209ef7522508..d8640d114c0d 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -3073,24 +3073,24 @@ static unsigned int bpf_iter_tcp_established_batch(struct seq_file *seq,
>  {
>         struct bpf_tcp_iter_state *iter = seq->private;
>         struct hlist_nulls_node *node;
> -       unsigned int expected = 1;
> -       struct sock *sk;
> +       unsigned int expected = 0;
> +       struct sock *sk = *start_sk;

nit: please keep the reverse xmas tree order.

see: Documentation/process/maintainer-netdev.rst



>
> -       sock_hold(*start_sk);
> -       iter->batch[iter->end_sk++].sk = *start_sk;
> -
> -       sk = sk_nulls_next(*start_sk);
>         *start_sk = NULL;
>         sk_nulls_for_each_from(sk, node) {
> -               if (seq_sk_match(seq, sk)) {
> -                       if (iter->end_sk < iter->max_sk) {
> -                               sock_hold(sk);
> -                               iter->batch[iter->end_sk++].sk = sk;
> -                       } else if (!*start_sk) {
> -                               /* Remember where we left off. */
> -                               *start_sk = sk;
> -                       }
> -                       expected++;
> +               if (!seq_sk_match(seq, sk))
> +                       continue;
> +               expected++;
> +               if (iter->end_sk < iter->max_sk) {
> +                       /* reqsk_queue_hash_req() inserts with sk_refcnt == 0
> +                        * and refcount_set()s it after the bucket lock drops.
> +                        */
> +                       if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
> +                               continue;
> +                       iter->batch[iter->end_sk++].sk = sk;
> +               } else if (!*start_sk) {
> +                       /* Remember where we left off. */
> +                       *start_sk = sk;
>                 }
>         }
>
> @@ -3128,12 +3128,14 @@ static struct sock *bpf_iter_tcp_batch(struct seq_file *seq)
>         struct sock *sk;
>         int err;
>
> +again:
>         sk = bpf_iter_tcp_resume(seq);
>         if (!sk)
>                 return NULL; /* Done */
>
>         expected = bpf_iter_fill_batch(seq, &sk);
> -       if (likely(iter->end_sk == expected))
> +       /* sk carries the first socket that did not fit in the batch. */

nit: please remove this comment since this is not always true,
e.g. resume function does not find the old sk and moves to the
next bucket.

The change itself looks good.

Thanks !