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

["Emil Tsalapatis" <emil@etsalapatis.com>]

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;
>  }