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

On 8/3/26 11:35, [email protected] wrote:
...
>> 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()?

I was considering something like this in bpf_sk_assign():

	refcounted = sk_is_refcounted(sk);
	if (refcounted && unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
		return -ENOENT;
	skb->destructor = refcounted ? sock_edemux : sock_pfree;

which would unconditionally drop the ref in sock_edemux(), but setting a
non-sock_pfree dtor affects the logic in ip_rcv_core() where we have

	/* Must drop socket now because of tproxy. */
	if (!skb_sk_is_prefetched(skb))
		skb_orphan(skb);

And I'm uncertain about touching net/ipv4/ip_input.c.