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

[sashiko-bot]

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

Pre-existing issues:
- [High] bpf_sk_assign() and sock_pfree() are vulnerable to the exact same reference leak via SOCK_RCU_FREE state transition that this patch fixes for bpf_sk_release().
--

--- Patch [3]: [PATCH bpf 3/3] bpf: Unconditionally take socket references in lookup helpers ---
commit a73ae21a3d81c1ea14d30abf4a0636c703e1fcfa
Author: Michal Luczaj <mhal@rbox.co>

bpf: Unconditionally take socket references in lookup helpers

This commit addresses a socket reference leak in BPF lookup helpers.
A socket refcounted at acquire time can gain SOCK_RCU_FREE before release,
causing the release to skip the put. The patch makes acquire and release
unconditional to fix this issue.

Closes: https://lore.kernel.org/bpf/20260701235552.2B0AA1F00A3F@smtp.kernel.org/

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 403aba3ce8913..4bc0b59668fae 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
[ ... ]
> @@ -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;
>  }

[Severity: High]
This is a pre-existing issue, but bpf_sk_assign() and sock_pfree() appear to
be vulnerable to the exact same reference leak via SOCK_RCU_FREE state
transitions that this patch fixes for bpf_sk_release().

If a BPF program calls bpf_sk_assign() on an unbound or established socket,
it conditionally acquires a reference:

net/core/filter.c:bpf_sk_assign() {
	...
	if (sk_is_refcounted(sk) &&
	    unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
		return -ENOENT;

	skb_orphan(skb);
	skb->sk = sk;
	skb->destructor = sock_pfree;
	...
}

Before the skb is freed, if the application calls listen() on the socket,
granting it the SOCK_RCU_FREE flag, what happens during destruction?

When the skb is destroyed, sock_pfree() dynamically re-evaluates
sk_is_refcounted(sk), which would now return false:

net/core/sock.c:sock_pfree() {
	...
	if (!sk_is_refcounted(sk))
		return;

	...
	sock_gen_put(sk);
}

Would this cause sock_pfree() to return early and permanently leak the
reference acquired in bpf_sk_assign()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-sockmap-lookup-tcp-leak-v1-0-1bbdc58ce514@rbox.co?part=3

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
>