* [PATCH bpf 1/3] bpf, sockmap: Use sock_hold() instead of refcount_inc_not_zero() in lookup
2026-07-23 11:33 [PATCH bpf 0/3] bpf: Fix socket leaks around connect(AF_UNSPEC)+listen() Michal Luczaj
@ 2026-07-23 11:33 ` Michal Luczaj
2026-07-25 0:00 ` John Fastabend
2026-07-23 11:33 ` [PATCH bpf 2/3] bpf: Extract shared reqsk-to-listener upgrade Michal Luczaj
2026-07-23 11:33 ` [PATCH bpf 3/3] bpf: Unconditionally take socket references in lookup helpers Michal Luczaj
2 siblings, 1 reply; 6+ messages in thread
From: Michal Luczaj @ 2026-07-23 11:33 UTC (permalink / raw)
To: Eric Dumazet, Kuniyuki Iwashima, Paolo Abeni, Willem de Bruijn,
John Fastabend, Jakub Sitnicki, Jiayuan Chen, David S. Miller,
Jakub Kicinski, Simon Horman, Daniel Borkmann, Stanislav Fomichev,
Martin KaFai Lau, Alexei Starovoitov, Andrii Nakryiko,
Eduard Zingerman, Kumar Kartikeya Dwivedi, Song Liu,
Yonghong Song, Jiri Olsa, Emil Tsalapatis, Joe Stringer
Cc: netdev, bpf, linux-kernel, Michal Luczaj
psock's hold on the looked up socket isn't dropped until sk_psock_drop() ->
queue_rcu_work() -> sk_psock_destroy() runs, which happens only after the
entry is unlinked and an RCU grace period elapses. Since the lookup runs
under RCU, a non-NULL result guarantees sk_refcnt >= 1:
refcount_inc_not_zero() can never fail here. Use sock_hold() instead.
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
net/core/sock_map.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 9efbd8ca7db8..ca49bc7f8687 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -392,8 +392,8 @@ static void *sock_map_lookup(struct bpf_map *map, void *key)
sk = __sock_map_lookup_elem(map, *(u32 *)key);
if (!sk)
return NULL;
- if (sk_is_refcounted(sk) && !refcount_inc_not_zero(&sk->sk_refcnt))
- return NULL;
+ if (sk_is_refcounted(sk))
+ sock_hold(sk);
return sk;
}
@@ -1218,8 +1218,8 @@ static void *sock_hash_lookup(struct bpf_map *map, void *key)
sk = __sock_hash_lookup_elem(map, key);
if (!sk)
return NULL;
- if (sk_is_refcounted(sk) && !refcount_inc_not_zero(&sk->sk_refcnt))
- return NULL;
+ if (sk_is_refcounted(sk))
+ sock_hold(sk);
return sk;
}
--
2.55.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf 1/3] bpf, sockmap: Use sock_hold() instead of refcount_inc_not_zero() in lookup
2026-07-23 11:33 ` [PATCH bpf 1/3] bpf, sockmap: Use sock_hold() instead of refcount_inc_not_zero() in lookup Michal Luczaj
@ 2026-07-25 0:00 ` John Fastabend
0 siblings, 0 replies; 6+ messages in thread
From: John Fastabend @ 2026-07-25 0:00 UTC (permalink / raw)
To: Michal Luczaj
Cc: Eric Dumazet, Kuniyuki Iwashima, Paolo Abeni, Willem de Bruijn,
Jakub Sitnicki, Jiayuan Chen, David S. Miller, Jakub Kicinski,
Simon Horman, Daniel Borkmann, Stanislav Fomichev,
Martin KaFai Lau, Alexei Starovoitov, Andrii Nakryiko,
Eduard Zingerman, Kumar Kartikeya Dwivedi, Song Liu,
Yonghong Song, Jiri Olsa, Emil Tsalapatis, Joe Stringer, netdev,
bpf, linux-kernel
On Thu, Jul 23, 2026 at 01:33:27PM +0200, Michal Luczaj wrote:
>psock's hold on the looked up socket isn't dropped until sk_psock_drop() ->
>queue_rcu_work() -> sk_psock_destroy() runs, which happens only after the
>entry is unlinked and an RCU grace period elapses. Since the lookup runs
>under RCU, a non-NULL result guarantees sk_refcnt >= 1:
>refcount_inc_not_zero() can never fail here. Use sock_hold() instead.
>
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
Should bpf-next right? this is an optimization not a fix?
> net/core/sock_map.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/net/core/sock_map.c b/net/core/sock_map.c
>index 9efbd8ca7db8..ca49bc7f8687 100644
>--- a/net/core/sock_map.c
>+++ b/net/core/sock_map.c
>@@ -392,8 +392,8 @@ static void *sock_map_lookup(struct bpf_map *map, void *key)
> sk = __sock_map_lookup_elem(map, *(u32 *)key);
> if (!sk)
> return NULL;
>- if (sk_is_refcounted(sk) && !refcount_inc_not_zero(&sk->sk_refcnt))
>- return NULL;
>+ if (sk_is_refcounted(sk))
>+ sock_hold(sk);
> return sk;
> }
>
>@@ -1218,8 +1218,8 @@ static void *sock_hash_lookup(struct bpf_map *map, void *key)
> sk = __sock_hash_lookup_elem(map, key);
> if (!sk)
> return NULL;
>- if (sk_is_refcounted(sk) && !refcount_inc_not_zero(&sk->sk_refcnt))
>- return NULL;
>+ if (sk_is_refcounted(sk))
>+ sock_hold(sk);
> return sk;
> }
>
>
>--
>2.55.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH bpf 2/3] bpf: Extract shared reqsk-to-listener upgrade
2026-07-23 11:33 [PATCH bpf 0/3] bpf: Fix socket leaks around connect(AF_UNSPEC)+listen() Michal Luczaj
2026-07-23 11:33 ` [PATCH bpf 1/3] bpf, sockmap: Use sock_hold() instead of refcount_inc_not_zero() in lookup Michal Luczaj
@ 2026-07-23 11:33 ` Michal Luczaj
2026-07-23 11:33 ` [PATCH bpf 3/3] bpf: Unconditionally take socket references in lookup helpers Michal Luczaj
2 siblings, 0 replies; 6+ messages in thread
From: Michal Luczaj @ 2026-07-23 11:33 UTC (permalink / raw)
To: Eric Dumazet, Kuniyuki Iwashima, Paolo Abeni, Willem de Bruijn,
John Fastabend, Jakub Sitnicki, Jiayuan Chen, David S. Miller,
Jakub Kicinski, Simon Horman, Daniel Borkmann, Stanislav Fomichev,
Martin KaFai Lau, Alexei Starovoitov, Andrii Nakryiko,
Eduard Zingerman, Kumar Kartikeya Dwivedi, Song Liu,
Yonghong Song, Jiri Olsa, Emil Tsalapatis, Joe Stringer
Cc: netdev, bpf, linux-kernel, Michal Luczaj
__bpf_sk_lookup() and bpf_sk_lookup() duplicate the same sk_to_full_sk()
reqsk-to-listener upgrade. Extract it into a helper.
Leave the currently unreachable WARN_ONCE as a defensive assert.
No functional change.
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
net/core/filter.c | 57 ++++++++++++++++++++++++-------------------------------
1 file changed, 25 insertions(+), 32 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index b446aa8be5c3..403aba3ce891 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -7074,6 +7074,27 @@ __bpf_skc_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
return sk;
}
+static struct sock *
+bpf_sk_lookup_full_sk(struct sock *sk)
+{
+ struct sock *sk2 = sk_to_full_sk(sk);
+
+ /* sk_to_full_sk() may return sk->rsk_listener, make sure the original
+ * sk sock refcnt is decremented to prevent a request_sock leak.
+ */
+ 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;
+ }
+ sk = sk2;
+ }
+
+ return sk;
+}
+
static struct sock *
__bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
struct net *caller_net, u32 ifindex, u8 proto, u64 netns_id,
@@ -7083,22 +7104,8 @@ __bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
ifindex, proto, netns_id, flags,
sdif);
- if (sk) {
- struct sock *sk2 = sk_to_full_sk(sk);
-
- /* sk_to_full_sk() may return (sk)->rsk_listener, so make sure the original sk
- * sock refcnt is decremented to prevent a request_sock leak.
- */
- 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;
- }
- sk = sk2;
- }
- }
+ if (sk)
+ sk = bpf_sk_lookup_full_sk(sk);
return sk;
}
@@ -7129,22 +7136,8 @@ bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
struct sock *sk = bpf_skc_lookup(skb, tuple, len, proto, netns_id,
flags);
- if (sk) {
- struct sock *sk2 = sk_to_full_sk(sk);
-
- /* sk_to_full_sk() may return (sk)->rsk_listener, so make sure the original sk
- * sock refcnt is decremented to prevent a request_sock leak.
- */
- 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;
- }
- sk = sk2;
- }
- }
+ if (sk)
+ sk = bpf_sk_lookup_full_sk(sk);
return sk;
}
--
2.55.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH bpf 3/3] bpf: Unconditionally take socket references in lookup helpers
2026-07-23 11:33 [PATCH bpf 0/3] bpf: Fix socket leaks around connect(AF_UNSPEC)+listen() Michal Luczaj
2026-07-23 11:33 ` [PATCH bpf 1/3] bpf, sockmap: Use sock_hold() instead of refcount_inc_not_zero() in lookup Michal Luczaj
2026-07-23 11:33 ` [PATCH bpf 2/3] bpf: Extract shared reqsk-to-listener upgrade Michal Luczaj
@ 2026-07-23 11:33 ` Michal Luczaj
2026-07-23 15:25 ` bot+bpf-ci
2 siblings, 1 reply; 6+ messages in thread
From: Michal Luczaj @ 2026-07-23 11:33 UTC (permalink / raw)
To: Eric Dumazet, Kuniyuki Iwashima, Paolo Abeni, Willem de Bruijn,
John Fastabend, Jakub Sitnicki, Jiayuan Chen, David S. Miller,
Jakub Kicinski, Simon Horman, Daniel Borkmann, Stanislav Fomichev,
Martin KaFai Lau, Alexei Starovoitov, Andrii Nakryiko,
Eduard Zingerman, Kumar Kartikeya Dwivedi, Song Liu,
Yonghong Song, Jiri Olsa, Emil Tsalapatis, Joe Stringer
Cc: netdev, bpf, linux-kernel, Michal Luczaj, Sashiko
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().
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;
}
--
2.55.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH bpf 3/3] bpf: Unconditionally take socket references in lookup helpers
2026-07-23 11:33 ` [PATCH bpf 3/3] bpf: Unconditionally take socket references in lookup helpers Michal Luczaj
@ 2026-07-23 15:25 ` bot+bpf-ci
0 siblings, 0 replies; 6+ messages in thread
From: bot+bpf-ci @ 2026-07-23 15:25 UTC (permalink / raw)
To: mhal, edumazet, kuniyu, pabeni, willemb, john.fastabend, jakub,
jiayuan.chen, davem, kuba, horms, daniel, sdf, martin.lau, ast,
andrii, eddyz87, memxor, song, yonghong.song, jolsa, emil, joe
Cc: netdev, bpf, linux-kernel, mhal, sashiko-bot, ast, andrii, daniel,
martin.lau, eddyz87, yonghong.song, clm, ihor.solodrai
[-- Attachment #1: Type: text/plain, Size: 1549 bytes --]
> 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;
> }
The BPF subsystem guide specifies that multi-line comments under
net/core/filter.c should have the opening '/*' on its own line. Could these
comments follow that format?
[ ... ]
> @@ -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;
> }
The same multi-line comment style appears here as well.
[ ... ]
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/30016591832
^ permalink raw reply [flat|nested] 6+ messages in thread