Re: [PATCH bpf 3/3] bpf: Unconditionally take socket references in lookup helpers

Re: [PATCH bpf 3/3] bpf: Unconditionally take socket references in lookup helpers

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 1549 bytes --]

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 403aba3ce891..4bc0b59668fa 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -7027,6 +7027,13 @@ static struct sock *sk_lookup(struct net *net, struct bpf_sock_tuple *tuple,
>  		WARN_ONCE(1, "Found non-RCU, unreferenced socket!");
>  		sk = NULL;
>  	}
> +
> +	/* Always take a reference, even if the lookup skipped one;
> +	 * bpf_sk_release() always puts one.
> +	 */
> +	if (sk && !refcounted && !refcount_inc_not_zero(&sk->sk_refcnt))
> +		sk = NULL;
> +
>  	return sk;
>  }

The BPF subsystem guide specifies that multi-line comments under
net/core/filter.c should have the opening '/*' on its own line. Could these
comments follow that format?

[ ... ]

> @@ -7084,11 +7091,15 @@ bpf_sk_lookup_full_sk(struct sock *sk)
>  	 */
>  	if (sk2 != sk) {
>  		sock_gen_put(sk);
> -		/* Ensure there is no need to bump sk2 refcnt. */
>  		if (unlikely(sk2 && !sock_flag(sk2, SOCK_RCU_FREE))) {
>  			WARN_ONCE(1, "Found non-RCU, unreferenced socket!");
>  			return NULL;
>  		}
> +		/* sk2 is RCU-free, but take a reference anyway;
> +		 * bpf_sk_release() puts.
> +		 */
> +		if (sk2 && !refcount_inc_not_zero(&sk2->sk_refcnt))
> +			sk2 = NULL;
>  		sk = sk2;
>  	}

The same multi-line comment style appears here as well.

[ ... ]

---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/30016591832

Re: [PATCH bpf 3/3] bpf: Unconditionally take socket references in lookup helpers

[Emil Tsalapatis]

On Thu Jul 23, 2026 at 7:33 AM EDT, Michal Luczaj wrote:
> Lookup helpers gate whether to acquire a socket reference on
> sk_is_refcounted(), a check re-evaluated at release. An established socket
> refcounted at acquire time can gain SOCK_RCU_FREE via
> connect(AF_UNSPEC)+listen() before release runs; the release-side re-check
> then reads sk_is_refcounted() == false and skips the put. The reference
> leaks.
>
> Make acquire and release unconditional and symmetric: always take a
> reference, always put it. Adapt sk_select_reuseport().

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

The bot's concern about the comment style is obviously invalid here.

>
> Fixes: 6acc9b432e67 ("bpf: Add helper to retrieve socket in BPF")
> Fixes: 64d85290d79c ("bpf: Allow bpf_map_lookup_elem for SOCKMAP and SOCKHASH")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/bpf/20260701235552.2B0AA1F00A3F@smtp.kernel.org/
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---
>  net/core/filter.c   | 25 ++++++++++++++++---------
>  net/core/sock_map.c |  8 ++------
>  2 files changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 403aba3ce891..4bc0b59668fa 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -7027,6 +7027,13 @@ static struct sock *sk_lookup(struct net *net, struct bpf_sock_tuple *tuple,
>  		WARN_ONCE(1, "Found non-RCU, unreferenced socket!");
>  		sk = NULL;
>  	}
> +
> +	/* Always take a reference, even if the lookup skipped one;
> +	 * bpf_sk_release() always puts one.
> +	 */
> +	if (sk && !refcounted && !refcount_inc_not_zero(&sk->sk_refcnt))
> +		sk = NULL;
> +
>  	return sk;
>  }
>  
> @@ -7084,11 +7091,15 @@ bpf_sk_lookup_full_sk(struct sock *sk)
>  	 */
>  	if (sk2 != sk) {
>  		sock_gen_put(sk);
> -		/* Ensure there is no need to bump sk2 refcnt. */
>  		if (unlikely(sk2 && !sock_flag(sk2, SOCK_RCU_FREE))) {
>  			WARN_ONCE(1, "Found non-RCU, unreferenced socket!");
>  			return NULL;
>  		}
> +		/* sk2 is RCU-free, but take a reference anyway;
> +		 * bpf_sk_release() puts.
> +		 */
> +		if (sk2 && !refcount_inc_not_zero(&sk2->sk_refcnt))
> +			sk2 = NULL;
>  		sk = sk2;
>  	}
>  
> @@ -7273,7 +7284,7 @@ static const struct bpf_func_proto bpf_tc_sk_lookup_udp_proto = {
>  
>  BPF_CALL_1(bpf_sk_release, struct sock *, sk)
>  {
> -	if (sk && sk_is_refcounted(sk))
> +	if (sk)
>  		sock_gen_put(sk);
>  	return 0;
>  }
> @@ -11565,11 +11576,13 @@ BPF_CALL_4(sk_select_reuseport, struct sk_reuseport_kern *, reuse_kern,
>  	bool is_sockarray = map->map_type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY;
>  	struct sock_reuseport *reuse;
>  	struct sock *selected_sk;
> -	int err;
> +	int err = 0;
>  
>  	selected_sk = map->ops->map_lookup_elem(map, key);
>  	if (!selected_sk)
>  		return -ENOENT;
> +	if (!is_sockarray)
> +		sock_put(selected_sk);
>  
>  	reuse = rcu_dereference(selected_sk->sk_reuseport_cb);
>  	if (!reuse) {
> @@ -11599,13 +11612,7 @@ BPF_CALL_4(sk_select_reuseport, struct sk_reuseport_kern *, reuse_kern,
>  	}
>  
>  	reuse_kern->selected_sk = selected_sk;
> -
> -	return 0;
>  error:
> -	/* Lookup in sock_map can return TCP ESTABLISHED sockets. */
> -	if (sk_is_refcounted(selected_sk))
> -		sock_put(selected_sk);
> -
>  	return err;
>  }
>  
> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index ca49bc7f8687..ae18dc4d60f9 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -390,9 +390,7 @@ static void *sock_map_lookup(struct bpf_map *map, void *key)
>  	struct sock *sk;
>  
>  	sk = __sock_map_lookup_elem(map, *(u32 *)key);
> -	if (!sk)
> -		return NULL;
> -	if (sk_is_refcounted(sk))
> +	if (sk)
>  		sock_hold(sk);
>  	return sk;
>  }
> @@ -1216,9 +1214,7 @@ static void *sock_hash_lookup(struct bpf_map *map, void *key)
>  	struct sock *sk;
>  
>  	sk = __sock_hash_lookup_elem(map, key);
> -	if (!sk)
> -		return NULL;
> -	if (sk_is_refcounted(sk))
> +	if (sk)
>  		sock_hold(sk);
>  	return sk;
>  }

Re: [PATCH bpf 3/3] bpf: Unconditionally take socket references in lookup helpers

[Michal Luczaj]

On 7/29/26 22:32, Emil Tsalapatis wrote:
> On Thu Jul 23, 2026 at 7:33 AM EDT, Michal Luczaj wrote:
>> Lookup helpers gate whether to acquire a socket reference on
>> sk_is_refcounted(), a check re-evaluated at release. An established socket
>> refcounted at acquire time can gain SOCK_RCU_FREE via
>> connect(AF_UNSPEC)+listen() before release runs; the release-side re-check
>> then reads sk_is_refcounted() == false and skips the put. The reference
>> leaks.
>>
>> Make acquire and release unconditional and symmetric: always take a
>> reference, always put it. Adapt sk_select_reuseport().
> 
> Reviewed-by: Emil Tsalapatis <emil@etsalapatis.com>

Thanks!

> The bot's concern about the comment style is obviously invalid here.

Are the prompts incorrect?
https://github.com/masoncl/review-prompts/blob/59469708305eca305cbd9eb94e5aa0ee3627529c/kernel/subsystem/bpf.md#bpf-comment-style

Michal

Re: [PATCH bpf 3/3] bpf: Unconditionally take socket references in lookup helpers

[Kumar Kartikeya Dwivedi]

On Thu Jul 30, 2026 at 1:55 PM CEST, Michal Luczaj wrote:
> On 7/29/26 22:32, Emil Tsalapatis wrote:
>> On Thu Jul 23, 2026 at 7:33 AM EDT, Michal Luczaj wrote:
>>> Lookup helpers gate whether to acquire a socket reference on
>>> sk_is_refcounted(), a check re-evaluated at release. An established socket
>>> refcounted at acquire time can gain SOCK_RCU_FREE via
>>> connect(AF_UNSPEC)+listen() before release runs; the release-side re-check
>>> then reads sk_is_refcounted() == false and skips the put. The reference
>>> leaks.
>>>
>>> Make acquire and release unconditional and symmetric: always take a
>>> reference, always put it. Adapt sk_select_reuseport().
>>
>> Reviewed-by: Emil Tsalapatis <emil@etsalapatis.com>
>
> Thanks!
>
>> The bot's concern about the comment style is obviously invalid here.
>
> Are the prompts incorrect?
> https://github.com/masoncl/review-prompts/blob/59469708305eca305cbd9eb94e5aa0ee3627529c/kernel/subsystem/bpf.md#bpf-comment-style
>

The prompt is correct, but we don't bother for existing comments, if you add a
new one, you can use the new style.

Overall, looks like the set is pretty close. You can respin targeting bpf-next
as John suggested (and we can wait for his ack before landing) so it can go
through CI again.

pw-bot: cr

> Michal

Re: [PATCH bpf 3/3] bpf: Unconditionally take socket references in lookup helpers

[Michal Luczaj]

On 8/1/26 18:29, Kumar Kartikeya Dwivedi wrote:
> On Thu Jul 30, 2026 at 1:55 PM CEST, Michal Luczaj wrote:
>> On 7/29/26 22:32, Emil Tsalapatis wrote:
>>> On Thu Jul 23, 2026 at 7:33 AM EDT, Michal Luczaj wrote:
>>>> Lookup helpers gate whether to acquire a socket reference on
>>>> sk_is_refcounted(), a check re-evaluated at release. An established socket
>>>> refcounted at acquire time can gain SOCK_RCU_FREE via
>>>> connect(AF_UNSPEC)+listen() before release runs; the release-side re-check
>>>> then reads sk_is_refcounted() == false and skips the put. The reference
>>>> leaks.
>>>>
>>>> Make acquire and release unconditional and symmetric: always take a
>>>> reference, always put it. Adapt sk_select_reuseport().
>>>
>>> Reviewed-by: Emil Tsalapatis <emil@etsalapatis.com>
>>
>> Thanks!
>>
>>> The bot's concern about the comment style is obviously invalid here.
>>
>> Are the prompts incorrect?
>> https://github.com/masoncl/review-prompts/blob/59469708305eca305cbd9eb94e5aa0ee3627529c/kernel/subsystem/bpf.md#bpf-comment-style
>>
> 
> The prompt is correct, but we don't bother for existing comments, if you add a
> new one, you can use the new style.
> 
> Overall, looks like the set is pretty close. You can respin targeting bpf-next
> as John suggested (and we can wait for his ack before landing) so it can go
> through CI again.

Sure, here's v2 with patch 1 dropped (I'll repost it targeting bpf-next
later):
https://lore.kernel.org/bpf/20260803-sockmap-lookup-tcp-leak-2-0-306e025bfe66@rbox.co/

thanks,
Michal

Re: [PATCH bpf 1/3] bpf, sockmap: Use sock_hold() instead of refcount_inc_not_zero() in lookup

[John Fastabend]

On Thu, Jul 23, 2026 at 01:33:27PM +0200, Michal Luczaj wrote:
>psock's hold on the looked up socket isn't dropped until sk_psock_drop() ->
>queue_rcu_work() -> sk_psock_destroy() runs, which happens only after the
>entry is unlinked and an RCU grace period elapses. Since the lookup runs
>under RCU, a non-NULL result guarantees sk_refcnt >= 1:
>refcount_inc_not_zero() can never fail here. Use sock_hold() instead.
>
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---

Should bpf-next right? this is an optimization not a fix?

> net/core/sock_map.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/net/core/sock_map.c b/net/core/sock_map.c
>index 9efbd8ca7db8..ca49bc7f8687 100644
>--- a/net/core/sock_map.c
>+++ b/net/core/sock_map.c
>@@ -392,8 +392,8 @@ static void *sock_map_lookup(struct bpf_map *map, void *key)
> 	sk = __sock_map_lookup_elem(map, *(u32 *)key);
> 	if (!sk)
> 		return NULL;
>-	if (sk_is_refcounted(sk) && !refcount_inc_not_zero(&sk->sk_refcnt))
>-		return NULL;
>+	if (sk_is_refcounted(sk))
>+		sock_hold(sk);
> 	return sk;
> }
>
>@@ -1218,8 +1218,8 @@ static void *sock_hash_lookup(struct bpf_map *map, void *key)
> 	sk = __sock_hash_lookup_elem(map, key);
> 	if (!sk)
> 		return NULL;
>-	if (sk_is_refcounted(sk) && !refcount_inc_not_zero(&sk->sk_refcnt))
>-		return NULL;
>+	if (sk_is_refcounted(sk))
>+		sock_hold(sk);
> 	return sk;
> }
>
>
>-- 
>2.55.0
>

Re: [PATCH bpf 1/3] bpf, sockmap: Use sock_hold() instead of refcount_inc_not_zero() in lookup

[Michal Luczaj]

On 7/25/26 02:00, John Fastabend wrote:
> On Thu, Jul 23, 2026 at 01:33:27PM +0200, Michal Luczaj wrote:
>> psock's hold on the looked up socket isn't dropped until sk_psock_drop() ->
>> queue_rcu_work() -> sk_psock_destroy() runs, which happens only after the
>> entry is unlinked and an RCU grace period elapses. Since the lookup runs
>> under RCU, a non-NULL result guarantees sk_refcnt >= 1:
>> refcount_inc_not_zero() can never fail here. Use sock_hold() instead.
>>
>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>> ---
> 
> Should bpf-next right? this is an optimization not a fix?

Right. Would you rather have it squashed with patch 3 or should I save it
for bpf-next?

>> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
>> index 9efbd8ca7db8..ca49bc7f8687 100644
>> --- a/net/core/sock_map.c
>> +++ b/net/core/sock_map.c
>> @@ -392,8 +392,8 @@ static void *sock_map_lookup(struct bpf_map *map, void *key)
>> 	sk = __sock_map_lookup_elem(map, *(u32 *)key);
>> 	if (!sk)
>> 		return NULL;
>> -	if (sk_is_refcounted(sk) && !refcount_inc_not_zero(&sk->sk_refcnt))
>> -		return NULL;
>> +	if (sk_is_refcounted(sk))
>> +		sock_hold(sk);
>> 	return sk;
>> }
>>
>> @@ -1218,8 +1218,8 @@ static void *sock_hash_lookup(struct bpf_map *map, void *key)
>> 	sk = __sock_hash_lookup_elem(map, key);
>> 	if (!sk)
>> 		return NULL;
>> -	if (sk_is_refcounted(sk) && !refcount_inc_not_zero(&sk->sk_refcnt))
>> -		return NULL;
>> +	if (sk_is_refcounted(sk))
>> +		sock_hold(sk);
>> 	return sk;
>> }

Re: [PATCH bpf 1/3] bpf, sockmap: Use sock_hold() instead of refcount_inc_not_zero() in lookup

[Emil Tsalapatis]

On Thu Jul 23, 2026 at 7:33 AM EDT, Michal Luczaj wrote:
> psock's hold on the looked up socket isn't dropped until sk_psock_drop() ->
> queue_rcu_work() -> sk_psock_destroy() runs, which happens only after the
> entry is unlinked and an RCU grace period elapses. Since the lookup runs
> under RCU, a non-NULL result guarantees sk_refcnt >= 1:
> refcount_inc_not_zero() can never fail here. Use sock_hold() instead.
>
> Signed-off-by: Michal Luczaj <mhal@rbox.co>

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

> ---
>  net/core/sock_map.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index 9efbd8ca7db8..ca49bc7f8687 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -392,8 +392,8 @@ static void *sock_map_lookup(struct bpf_map *map, void *key)
>  	sk = __sock_map_lookup_elem(map, *(u32 *)key);
>  	if (!sk)
>  		return NULL;
> -	if (sk_is_refcounted(sk) && !refcount_inc_not_zero(&sk->sk_refcnt))
> -		return NULL;
> +	if (sk_is_refcounted(sk))
> +		sock_hold(sk);
>  	return sk;
>  }
>  
> @@ -1218,8 +1218,8 @@ static void *sock_hash_lookup(struct bpf_map *map, void *key)
>  	sk = __sock_hash_lookup_elem(map, key);
>  	if (!sk)
>  		return NULL;
> -	if (sk_is_refcounted(sk) && !refcount_inc_not_zero(&sk->sk_refcnt))
> -		return NULL;
> +	if (sk_is_refcounted(sk))
> +		sock_hold(sk);
>  	return sk;
>  }
>  

Re: [PATCH bpf 2/3] bpf: Extract shared reqsk-to-listener upgrade

[Emil Tsalapatis]

On Thu Jul 23, 2026 at 7:33 AM EDT, Michal Luczaj wrote:
> __bpf_sk_lookup() and bpf_sk_lookup() duplicate the same sk_to_full_sk()
> reqsk-to-listener upgrade. Extract it into a helper.
>
> Leave the currently unreachable WARN_ONCE as a defensive assert.
>
> No functional change.

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

>
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---
>  net/core/filter.c | 57 ++++++++++++++++++++++++-------------------------------
>  1 file changed, 25 insertions(+), 32 deletions(-)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index b446aa8be5c3..403aba3ce891 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -7074,6 +7074,27 @@ __bpf_skc_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
>  	return sk;
>  }
>  
> +static struct sock *
> +bpf_sk_lookup_full_sk(struct sock *sk)
> +{
> +	struct sock *sk2 = sk_to_full_sk(sk);
> +
> +	/* sk_to_full_sk() may return sk->rsk_listener, make sure the original
> +	 * sk sock refcnt is decremented to prevent a request_sock leak.
> +	 */
> +	if (sk2 != sk) {
> +		sock_gen_put(sk);
> +		/* Ensure there is no need to bump sk2 refcnt. */
> +		if (unlikely(sk2 && !sock_flag(sk2, SOCK_RCU_FREE))) {
> +			WARN_ONCE(1, "Found non-RCU, unreferenced socket!");
> +			return NULL;
> +		}
> +		sk = sk2;
> +	}
> +
> +	return sk;
> +}
> +
>  static struct sock *
>  __bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
>  		struct net *caller_net, u32 ifindex, u8 proto, u64 netns_id,
> @@ -7083,22 +7104,8 @@ __bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
>  					   ifindex, proto, netns_id, flags,
>  					   sdif);
>  
> -	if (sk) {
> -		struct sock *sk2 = sk_to_full_sk(sk);
> -
> -		/* sk_to_full_sk() may return (sk)->rsk_listener, so make sure the original sk
> -		 * sock refcnt is decremented to prevent a request_sock leak.
> -		 */
> -		if (sk2 != sk) {
> -			sock_gen_put(sk);
> -			/* Ensure there is no need to bump sk2 refcnt */
> -			if (unlikely(sk2 && !sock_flag(sk2, SOCK_RCU_FREE))) {
> -				WARN_ONCE(1, "Found non-RCU, unreferenced socket!");
> -				return NULL;
> -			}
> -			sk = sk2;
> -		}
> -	}
> +	if (sk)
> +		sk = bpf_sk_lookup_full_sk(sk);
>  
>  	return sk;
>  }
> @@ -7129,22 +7136,8 @@ bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
>  	struct sock *sk = bpf_skc_lookup(skb, tuple, len, proto, netns_id,
>  					 flags);
>  
> -	if (sk) {
> -		struct sock *sk2 = sk_to_full_sk(sk);
> -
> -		/* sk_to_full_sk() may return (sk)->rsk_listener, so make sure the original sk
> -		 * sock refcnt is decremented to prevent a request_sock leak.
> -		 */
> -		if (sk2 != sk) {
> -			sock_gen_put(sk);
> -			/* Ensure there is no need to bump sk2 refcnt */
> -			if (unlikely(sk2 && !sock_flag(sk2, SOCK_RCU_FREE))) {
> -				WARN_ONCE(1, "Found non-RCU, unreferenced socket!");
> -				return NULL;
> -			}
> -			sk = sk2;
> -		}
> -	}
> +	if (sk)
> +		sk = bpf_sk_lookup_full_sk(sk);
>  
>  	return sk;
>  }