Thread (8 messages) flat view 8 messages, 4 authors, 6h ago
HOTtoday REVIEWED: 1 (1M)

1 review trailer (1 from subsystem maintainers).

[PATCH bpf v3 1/2] bpf: Unconditionally take socket references in lookup helpers

From: Michal Luczaj <hidden>
Date: 2026-09-02 17:55:35
Also in: bpf, lkml, netfilter-devel
Subsystem: bpf [general] (safe dynamic programs and tools), bpf [l7 framework] (sockmap), bpf [networking] (tcx & tc bpf, sock_addr), networking [general], networking [sockets], the rest · Maintainers: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Eduard Zingerman, Kumar Kartikeya Dwivedi, John Fastabend, Jakub Sitnicki, Jiayuan Chen, "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Kuniyuki Iwashima, Willem de Bruijn, Linus Torvalds

Lookup helpers gate the reference acquisition on sk_is_refcounted(), and
the release side re-evaluates the same check. An established socket can be
refcounted at acquire time and later gain SOCK_RCU_FREE via
connect(AF_UNSPEC) + listen() before release. The release re-check then
sees sk_is_refcounted() == false and skips the put, leaking the reference.

Make acquire and release unconditional and symmetric: always take a
reference, always put it. Adapt sk_select_reuseport(). Drop the
misplaced/outdated comment above __bpf_skc_lookup().

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/ (local)
Signed-off-by: Michal Luczaj <redacted>
Reviewed-by: Emil Tsalapatis <emil@etsalapatis.com>
---
 net/core/filter.c   | 30 ++++++++++++++++++------------
 net/core/sock_map.c |  8 ++------
 2 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index 61940e753552..66d83a198ea2 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -7205,12 +7205,17 @@ 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;
 }
 
-/* bpf_skc_lookup performs the core lookup for different types of sockets,
- * taking a reference on the socket if it doesn't have the flag SOCK_RCU_FREE.
- */
 static struct sock *
 __bpf_skc_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
 		 struct net *caller_net, u32 ifindex, u8 proto, u64 netns_id,
@@ -7263,11 +7268,16 @@ 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 not refcounted, but take a reference anyway;
+		 * bpf_sk_release() puts.
+		 */
+		if (sk2 && !refcount_inc_not_zero(&sk2->sk_refcnt))
+			sk2 = NULL;
 		sk = sk2;
 	}
 
@@ -7448,7 +7458,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;
 }
@@ -11736,11 +11746,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) {
@@ -11770,13 +11782,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;
 }
-- 
2.55.0
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help