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

[Jiayuan Chen]

On 6/20/26 8:32 AM, Jose Fernandez (Anthropic) wrote:
> 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 <ben@xxxxxxxxxxx>
> Assisted-by: Claude:unspecified
> Signed-off-by: Jose Fernandez (Anthropic) <jose.fernandez@xxxxxxxxx>


LGTM.

Reviewed-by: Jiayuan Chen <jiayuan.chen@xxxxxxxxx>


> ---
>   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);



Folding the open-coded first *start_sk into the loop is a good
cleanup — it was the one socket that bypassed the refcnt check.


The double-put on the realloc-failure path reported by ai is a separate,
pre-existing issue and can be addressed in a follow-up.