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

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

[Jose Fernandez (Anthropic)]

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
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@linux.dev>
---
Changes in v3:
- Use the canonical format for the cdec67a489d4 reference in the
  commit message (Kuniyuki)
- Keep the reverse xmas tree ordering of the local variables in
  bpf_iter_tcp_established_batch() (Kuniyuki)
- Drop the stale comment at the again: retry site in
  bpf_iter_tcp_batch() (Kuniyuki)
- Link to v2: https://lore.kernel.org/bpf/20260717-bpf-iter-tcp-refcnt-v2-1-8e81f0ac6f3e@linux.dev

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@linux.dev

The pre-existing double-put on the realloc failure path (raised in the
v1 thread) has since been fixed upstream by commit 980a81345275 ("bpf:
tcp: fix double sock release on batch realloc").
---
 net/ipv4/tcp_ipv4.c | 43 ++++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 209ef7522508..1034757a5328 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;
+	struct sock *sk = *start_sk;
+	unsigned int expected = 0;
 
-	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,13 @@ 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))
+	if (likely(!sk))
 		goto done;
 
 	/* Batch size was too small. */
@@ -3149,7 +3150,7 @@ static struct sock *bpf_iter_tcp_batch(struct seq_file *seq)
 		return NULL; /* Done */
 
 	expected = bpf_iter_fill_batch(seq, &sk);
-	if (likely(iter->end_sk == expected))
+	if (likely(!sk))
 		goto done;
 
 	/* Batch size was still too small. Hold onto the lock while we try
@@ -3162,10 +3163,14 @@ static struct sock *bpf_iter_tcp_batch(struct seq_file *seq)
 		return ERR_PTR(err);
 	}
 
-	expected = bpf_iter_fill_batch(seq, &sk);
-	WARN_ON_ONCE(iter->end_sk != expected);
+	bpf_iter_fill_batch(seq, &sk);
+	WARN_ON_ONCE(sk);
 done:
 	bpf_iter_tcp_unlock_bucket(seq);
+	if (unlikely(!iter->end_sk)) {
+		++iter->state.bucket;
+		goto again;
+	}
 	return iter->batch[0].sk;
 }
 

---
base-commit: 7cbd0c4cebe4c9f678d15e6b9ba975e1155a107f
change-id: 20260619-bpf-iter-tcp-refcnt-107d52b238da

Best regards,
--  
Jose Fernandez (Anthropic) <jose.fernandez@linux.dev>

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Double release of socket references when batch reallocation fails, leading to a crash.
--

--- Patch [1]: [PATCH bpf v3] bpf: tcp: Fix use-after-free in bpf_iter_tcp_established_batch() ---
commit a3c30aa4537ca1f20e7ead4267815490d8e97126
Author: Jose Fernandez (Anthropic) <jose.fernandez@linux.dev>
Subject: bpf: tcp: Fix use-after-free in bpf_iter_tcp_established_batch()

This commit fixes a use-after-free condition in the TCP iterator caused by
reqsk_queue_hash_req() modifying a socket's refcount after insertion into
the hash chain. It switches to using refcount_inc_not_zero() while holding
the bucket lock and skipping unincremented sockets. Batch completeness is
now evaluated by checking if the walk left any socket behind.

Link to v2: https://lore.kernel.org/bpf/20260717-bpf-iter-tcp-refcnt-v2-1-8e81f0ac6f3e@linux.dev
Link to v1: https://lore.kernel.org/bpf/20260620-bpf-iter-tcp-refcnt-v1-1-883bf9e69495@linux.dev

> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index 209ef7522508f..1034757a53280 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c

[ ... ]

> @@ -3162,10 +3163,14 @@ static struct sock *bpf_iter_tcp_batch(struct seq_file *seq)
>  		return ERR_PTR(err);
>  	}

[Severity: High]
This is a pre-existing issue, but could this lead to a double release and
crash if the batch reallocation fails?

When bpf_iter_tcp_realloc_batch() fails (e.g. with -ENOMEM), it returns the
error here, but iter->end_sk is not reset. 

The seq_read() loop will then abort and invoke the stop callback,
bpf_iter_tcp_seq_stop(), which blindly calls bpf_iter_tcp_put_batch()
again:

	if (iter->cur_sk < iter->end_sk)
		bpf_iter_tcp_put_batch(iter);

Inside bpf_iter_tcp_put_batch(), the sk pointer and cookie share a union. The
first release clobbers the sk pointer:

	cookie = sock_gen_cookie(item->sk);
	sock_gen_put(item->sk);
	item->cookie = cookie;

On the second call, would this dereference the previously stored cookie as a
socket pointer, causing a panic?

It is noted in the commit message that this double-put bug was fixed upstream
by commit 980a81345275 ("bpf: tcp: fix double sock release on batch
realloc").

> -	expected = bpf_iter_fill_batch(seq, &sk);
> -	WARN_ON_ONCE(iter->end_sk != expected);
> +	bpf_iter_fill_batch(seq, &sk);
> +	WARN_ON_ONCE(sk);
>  done:
>  	bpf_iter_tcp_unlock_bucket(seq);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730-bpf-iter-tcp-refcnt-v3-1-754b9c8a6717@linux.dev?part=1

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

[Kuniyuki Iwashima]

On Fri, Jul 31, 2026 at 12:33 AM Jose Fernandez (Anthropic)
<jose.fernandez@linux.dev> 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
> 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@linux.dev>

Reviewed-by: Kuniyuki Iwashima <kuniyu@google.com>

Thanks !