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

[sashiko-bot@kernel.org]

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