Thread (9 messages) 9 messages, 5 authors, 3d ago

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

From: Kuniyuki Iwashima <kuniyu@google.com>
Date: 2026-07-11 12:36:19
Also in: bpf, lkml

On Fri, Jun 19, 2026 at 5:33 PM Jose Fernandez (Anthropic)
[off-list ref] wrote:
quoted hunk ↗ jump to hunk
reqsk_queue_hash_req() publishes a TCP_NEW_SYN_RECV request_sock onto
the ehash chain (via inet_ehash_insert(), which drops the bucket lock on
return) and only afterwards refcount_set()s rsk_refcnt to 3.

Lockless readers such as __inet_lookup_established() account for this by
using 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

refcount_warn_saturate() then saturates the count, the publishing CPU's
refcount_set() clobbers it, and the socket is left one reference short.
When the last legitimate owner drops its reference the reqsk is freed
while still reachable, leading to use-after-free panics in e.g.
inet_csk_accept() or inet_csk_listen_stop().

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, the same way
every other ehash walker does. The listening hash is unaffected as
listeners are always inserted into lhash2 with sk_refcnt >= 1, so
bpf_iter_tcp_listening_batch() is left as-is.

If every matching socket in a bucket is mid-init, end_sk can stay at 0;
advance to the next bucket in that case rather than terminating the
whole iteration on a stale batch[0].

Fixes: 04c7820b776f ("bpf: tcp: Bpf iter batching and lock_sock")
Reviewed-by: Ben Cressey <redacted>
Assisted-by: Claude:unspecified
Signed-off-by: Jose Fernandez (Anthropic) <redacted>
---
 net/ipv4/tcp_ipv4.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index fdc81150ff6c..92342dcc6892 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -3074,25 +3074,25 @@ 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;

-       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;
+               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;
                }
+               expected++;
This should be incremented just after seq_sk_match()
(see below)

quoted hunk ↗ jump to hunk
        }

        return expected;
@@ -3129,6 +3129,7 @@ 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 */
@@ -3167,6 +3168,10 @@ static struct sock *bpf_iter_tcp_batch(struct seq_file *seq)
        WARN_ON_ONCE(iter->end_sk != expected);
Let's say the batch array was smaller than the hash chain length
and we reallocate the array based on "expected" w/ the bucket lock.

What happens if refcount_set(..., 3) is called during reallocation ?
bpf_iter_fill_batch() will see the larger "expected", and WARN_ON_ONCE()
will be triggered.

 done:
        bpf_iter_tcp_unlock_bucket(seq);
+       if (unlikely(!iter->end_sk)) {
+               ++iter->state.bucket;
+               goto again;
+       }
        return iter->batch[0].sk;
 }


---
base-commit: 4549871118cf616eecdd2d939f78e3b9e1dddc48
change-id: 20260619-bpf-iter-tcp-refcnt-107d52b238da

Best regards,
--
Jose Fernandez (Anthropic) [off-list ref]
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help