The Linux Kernel Mailing List
 help / color / mirror / Atom feed
* [PATCH bpf] bpf: tcp: Fix use-after-free in bpf_iter_tcp_established_batch()
@ 2026-06-20  0:32 Jose Fernandez (Anthropic)
  2026-06-20 14:06 ` Jiayuan Chen
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Jose Fernandez (Anthropic) @ 2026-06-20  0:32 UTC (permalink / raw)
  To: Eric Dumazet, Neal Cardwell, Kuniyuki Iwashima, David S. Miller,
	Jakub Kicinski, Paolo Abeni, Simon Horman, Andrii Nakryiko,
	Yonghong Song, Martin KaFai Lau
  Cc: netdev, linux-kernel, bpf, Ben Cressey,
	Jose Fernandez (Anthropic)

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@cressey.dev>
Assisted-by: Claude:unspecified
Signed-off-by: Jose Fernandez (Anthropic) <jose.fernandez@linux.dev>
---
 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++;
 	}
 
 	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);
 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) <jose.fernandez@linux.dev>


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf] bpf: tcp: Fix use-after-free in bpf_iter_tcp_established_batch()
  2026-06-20  0:32 [PATCH bpf] bpf: tcp: Fix use-after-free in bpf_iter_tcp_established_batch() Jose Fernandez (Anthropic)
@ 2026-06-20 14:06 ` Jiayuan Chen
  2026-06-29  7:37 ` Emil Tsalapatis
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Jiayuan Chen @ 2026-06-20 14:06 UTC (permalink / raw)
  To: Jose Fernandez (Anthropic), Eric Dumazet, Neal Cardwell,
	Kuniyuki Iwashima, David S. Miller, Jakub Kicinski, Paolo Abeni,
	Simon Horman, Andrii Nakryiko, Yonghong Song, Martin KaFai Lau
  Cc: netdev, linux-kernel, bpf, Ben Cressey


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@cressey.dev>
> Assisted-by: Claude:unspecified
> Signed-off-by: Jose Fernandez (Anthropic) <jose.fernandez@linux.dev>


LGTM.

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


> ---
>   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.



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf] bpf: tcp: Fix use-after-free in bpf_iter_tcp_established_batch()
  2026-06-20  0:32 [PATCH bpf] bpf: tcp: Fix use-after-free in bpf_iter_tcp_established_batch() Jose Fernandez (Anthropic)
  2026-06-20 14:06 ` Jiayuan Chen
@ 2026-06-29  7:37 ` Emil Tsalapatis
  2026-07-10 13:44 ` Jose Fernandez (Anthropic)
  2026-07-11 12:36 ` Kuniyuki Iwashima
  3 siblings, 0 replies; 8+ messages in thread
From: Emil Tsalapatis @ 2026-06-29  7:37 UTC (permalink / raw)
  To: Jose Fernandez (Anthropic), Eric Dumazet, Neal Cardwell,
	Kuniyuki Iwashima, David S. Miller, Jakub Kicinski, Paolo Abeni,
	Simon Horman, Andrii Nakryiko, Yonghong Song, Martin KaFai Lau
  Cc: netdev, linux-kernel, bpf, Ben Cressey

On Fri Jun 19, 2026 at 8:32 PM EDT, 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@cressey.dev>
> Assisted-by: Claude:unspecified
> Signed-off-by: Jose Fernandez (Anthropic) <jose.fernandez@linux.dev>

Reviewed-by: Emil Tsalapatis <emil@etsalapatis.com>

> ---
>  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++;
>  	}
>  
>  	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);
>  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) <jose.fernandez@linux.dev>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf] bpf: tcp: Fix use-after-free in bpf_iter_tcp_established_batch()
  2026-06-20  0:32 [PATCH bpf] bpf: tcp: Fix use-after-free in bpf_iter_tcp_established_batch() Jose Fernandez (Anthropic)
  2026-06-20 14:06 ` Jiayuan Chen
  2026-06-29  7:37 ` Emil Tsalapatis
@ 2026-07-10 13:44 ` Jose Fernandez (Anthropic)
  2026-07-10 14:32   ` Daniel Borkmann
  2026-07-11 12:36 ` Kuniyuki Iwashima
  3 siblings, 1 reply; 8+ messages in thread
From: Jose Fernandez (Anthropic) @ 2026-07-10 13:44 UTC (permalink / raw)
  To: Eric Dumazet, Neal Cardwell, Kuniyuki Iwashima, David S. Miller,
	Jakub Kicinski, Paolo Abeni, Simon Horman, Andrii Nakryiko,
	Yonghong Song, Martin KaFai Lau, Jiayuan Chen, Emil Tsalapatis
  Cc: netdev, linux-kernel, bpf, Ben Cressey

Hi all,

Gentle ping. This has Reviewed-by from Jiayuan Chen and Emil Tsalapatis
and no outstanding objections since Jun 29. Is anything else needed for
bpf-fixes?

On the automated review's flag about the pre-existing double-put on the
bpf_iter_tcp_realloc_batch() failure path (cur_sk/end_sk not reset
before seq_stop, so cookies can be re-put as pointers): agreed it is
real, and it predates this change. As Jiayuan noted, it is a separate,
pre-existing issue best addressed on its own; I kept this patch minimal
for stable.

Thanks,
Jose

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf] bpf: tcp: Fix use-after-free in bpf_iter_tcp_established_batch()
  2026-07-10 13:44 ` Jose Fernandez (Anthropic)
@ 2026-07-10 14:32   ` Daniel Borkmann
  2026-07-10 16:17     ` Kuniyuki Iwashima
  2026-07-10 22:24     ` Jose Fernandez (Anthropic)
  0 siblings, 2 replies; 8+ messages in thread
From: Daniel Borkmann @ 2026-07-10 14:32 UTC (permalink / raw)
  To: Jose Fernandez (Anthropic), Eric Dumazet, Neal Cardwell,
	Kuniyuki Iwashima, David S. Miller, Jakub Kicinski, Paolo Abeni,
	Simon Horman, Andrii Nakryiko, Yonghong Song, Martin KaFai Lau,
	Jiayuan Chen, Emil Tsalapatis
  Cc: netdev, linux-kernel, bpf, Ben Cressey

Hi Jose,

On 7/10/26 3:44 PM, Jose Fernandez (Anthropic) wrote:
> Hi all,
> 
> Gentle ping. This has Reviewed-by from Jiayuan Chen and Emil Tsalapatis
> and no outstanding objections since Jun 29. Is anything else needed for
> bpf-fixes?
> 
> On the automated review's flag about the pre-existing double-put on the
> bpf_iter_tcp_realloc_batch() failure path (cur_sk/end_sk not reset
> before seq_stop, so cookies can be re-put as pointers): agreed it is
> real, and it predates this change. As Jiayuan noted, it is a separate,
> pre-existing issue best addressed on its own; I kept this patch minimal
> for stable.

Given TCP, it would be good to still get an Ack from Kuniyuki or Eric and
then we can ship it. Are you planning to also follow-up with the other pre-
existing issue finding?

Thanks a lot,
Daniel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf] bpf: tcp: Fix use-after-free in bpf_iter_tcp_established_batch()
  2026-07-10 14:32   ` Daniel Borkmann
@ 2026-07-10 16:17     ` Kuniyuki Iwashima
  2026-07-10 22:24     ` Jose Fernandez (Anthropic)
  1 sibling, 0 replies; 8+ messages in thread
From: Kuniyuki Iwashima @ 2026-07-10 16:17 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Jose Fernandez (Anthropic), Eric Dumazet, Neal Cardwell,
	David S. Miller, Jakub Kicinski, Paolo Abeni, Simon Horman,
	Andrii Nakryiko, Yonghong Song, Martin KaFai Lau, Jiayuan Chen,
	Emil Tsalapatis, netdev, linux-kernel, bpf, Ben Cressey

On Fri, Jul 10, 2026 at 7:32 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> Hi Jose,
>
> On 7/10/26 3:44 PM, Jose Fernandez (Anthropic) wrote:
> > Hi all,
> >
> > Gentle ping. This has Reviewed-by from Jiayuan Chen and Emil Tsalapatis
> > and no outstanding objections since Jun 29. Is anything else needed for
> > bpf-fixes?
> >
> > On the automated review's flag about the pre-existing double-put on the
> > bpf_iter_tcp_realloc_batch() failure path (cur_sk/end_sk not reset
> > before seq_stop, so cookies can be re-put as pointers): agreed it is
> > real, and it predates this change. As Jiayuan noted, it is a separate,
> > pre-existing issue best addressed on its own; I kept this patch minimal
> > for stable.
>
> Given TCP, it would be good to still get an Ack from Kuniyuki or Eric and
> then we can ship it. Are you planning to also follow-up with the other pre-
> existing issue finding?

Oh sorry, somehow I missed this patch.  Will look into it today.

Thanks !

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf] bpf: tcp: Fix use-after-free in bpf_iter_tcp_established_batch()
  2026-07-10 14:32   ` Daniel Borkmann
  2026-07-10 16:17     ` Kuniyuki Iwashima
@ 2026-07-10 22:24     ` Jose Fernandez (Anthropic)
  1 sibling, 0 replies; 8+ messages in thread
From: Jose Fernandez (Anthropic) @ 2026-07-10 22:24 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Eric Dumazet, Neal Cardwell, Kuniyuki Iwashima, David S. Miller,
	Jakub Kicinski, Paolo Abeni, Simon Horman, Andrii Nakryiko,
	Yonghong Song, Martin KaFai Lau, Jiayuan Chen, Emil Tsalapatis,
	netdev, linux-kernel, bpf, Ben Cressey

On Fri, Jul 10, 2026 at 04:32:39PM +0200, Daniel Borkmann wrote:
> Given TCP, it would be good to still get an Ack from Kuniyuki or Eric and
> then we can ship it. Are you planning to also follow-up with the other pre-
> existing issue finding?

Yes, I'll send a separate patch for the realloc_batch() double-put next
week.

Thanks,
Jose

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf] bpf: tcp: Fix use-after-free in bpf_iter_tcp_established_batch()
  2026-06-20  0:32 [PATCH bpf] bpf: tcp: Fix use-after-free in bpf_iter_tcp_established_batch() Jose Fernandez (Anthropic)
                   ` (2 preceding siblings ...)
  2026-07-10 13:44 ` Jose Fernandez (Anthropic)
@ 2026-07-11 12:36 ` Kuniyuki Iwashima
  3 siblings, 0 replies; 8+ messages in thread
From: Kuniyuki Iwashima @ 2026-07-11 12:36 UTC (permalink / raw)
  To: Jose Fernandez (Anthropic)
  Cc: Eric Dumazet, Neal Cardwell, David S. Miller, Jakub Kicinski,
	Paolo Abeni, Simon Horman, Andrii Nakryiko, Yonghong Song,
	Martin KaFai Lau, netdev, linux-kernel, bpf, Ben Cressey

On Fri, Jun 19, 2026 at 5:33 PM Jose Fernandez (Anthropic)
<jose.fernandez@linux.dev> 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@cressey.dev>
> Assisted-by: Claude:unspecified
> Signed-off-by: Jose Fernandez (Anthropic) <jose.fernandez@linux.dev>
> ---
>  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)


>         }
>
>         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) <jose.fernandez@linux.dev>
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2026-07-11 12:36 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-20  0:32 [PATCH bpf] bpf: tcp: Fix use-after-free in bpf_iter_tcp_established_batch() Jose Fernandez (Anthropic)
2026-06-20 14:06 ` Jiayuan Chen
2026-06-29  7:37 ` Emil Tsalapatis
2026-07-10 13:44 ` Jose Fernandez (Anthropic)
2026-07-10 14:32   ` Daniel Borkmann
2026-07-10 16:17     ` Kuniyuki Iwashima
2026-07-10 22:24     ` Jose Fernandez (Anthropic)
2026-07-11 12:36 ` Kuniyuki Iwashima

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox