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

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

Pre-existing issues:
- [High] A pre-existing socket reference leak exists when `bpf_sk_assign()` assigns a socket to an skb and `sock_pfree()` acts as the destructor. `bpf_sk_assign()` conditionally increments the reference count if `sk_is_refcounted()` returns true, but `sock_pfree()` independently re-evaluates `sk_is_refcounted()`. If the socket transitions to `SOCK_RCU_FREE` between these two calls (e.g., via `connect(AF_UNSPEC)` followed by `listen()`), `sock_pfree()` returns early without calling `sock_gen_put()`, permanently leaking the reference.
--

--- Patch [2]: [PATCH bpf v2 2/2] bpf: Unconditionally take socket references in lookup helpers ---
commit 7463d9a6a26a249e959688cc8e1b8c2847ee47d0
Author: Michal Luczaj <[email protected]>

bpf: Unconditionally take socket references in lookup helpers

This commit updates BPF socket lookup helpers to unconditionally acquire
and release socket references. It fixes a leak that occurs if a socket
transitions to SOCK_RCU_FREE between the time the reference is
conditionally taken and released.

Closes: https://lore.kernel.org/bpf/[email protected]/

> TC bpf_sk_assign() has the same issue; it takes a reference only when
> sk_is_refcounted() is true at assign time, but sock_pfree() (the skb
> destructor it installs) re-checks sk_is_refcounted() independently at
> release time. The same connect(AF_UNSPEC)+listen() transition leaks the
> socket here too. I'd welcome suggestions on the right way to handle this.

[Severity: High]
This is a pre-existing issue, but as you noted, a socket reference is
permanently leaked if the socket state changes to unrefcounted between
bpf_sk_assign() and the skb destructor running.

In net/core/filter.c:bpf_sk_assign(), the reference is conditionally acquired
and the destructor is set:

	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;

Then in net/core/sock.c:sock_pfree(), the reference drop is independently
evaluated:

void sock_pfree(struct sk_buff *skb)
{
	struct sock *sk = skb->sk;

	if (!sk_is_refcounted(sk))
		return;

	...
	sock_gen_put(sk);
}

If the socket transitions to SOCK_RCU_FREE via a connect(AF_UNSPEC) followed
by a listen() before the skb is freed, sock_pfree() returns early without
calling sock_gen_put(). 

Could this be addressed by either applying the same unconditional reference
counting approach to bpf_sk_assign(), or by introducing a dedicated
destructor for BPF-assigned sockets that unconditionally drops the reference
without re-evaluating sk_is_refcounted()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2