* [PATCH net 1/2] bpf, sockmap: Avoid sk_prot reset on sockmap unlink with ULP set
2025-03-14 8:20 [PATCH net 0/2] bpf, sockmap: Avoid sk_prot reset on sockmap unlink with ULP set Dong Chenchen
@ 2025-03-14 8:20 ` Dong Chenchen
2025-03-19 23:42 ` Cong Wang
2025-03-14 8:20 ` [PATCH net 2/2] selftests: bpf: Add case for sockmap_ktls set when verdict attached Dong Chenchen
1 sibling, 1 reply; 4+ messages in thread
From: Dong Chenchen @ 2025-03-14 8:20 UTC (permalink / raw)
To: edumazet, kuniyu, pabeni, willemb, john.fastabend, jakub, davem,
kuba, horms, daniel
Cc: netdev, bpf, stfomichev, mrpre, xiyou.wangcong, zhangchangzhong,
weiyongjun1, Dong Chenchen
WARNING: CPU: 0 PID: 6558 at net/core/sock_map.c:1703 sock_map_close+0x3c4/0x480
Modules linked in:
CPU: 0 UID: 0 PID: 6558 Comm: syz-executor.14 Not tainted 6.14.0-rc5+ #238
RIP: 0010:sock_map_close+0x3c4/0x480
Call Trace:
<TASK>
inet_release+0x144/0x280
__sock_release+0xb8/0x270
sock_close+0x1e/0x30
__fput+0x3c6/0xb30
__fput_sync+0x7b/0x90
__x64_sys_close+0x90/0x120
do_syscall_64+0x5d/0x170
entry_SYSCALL_64_after_hwframe+0x76/0x7e
The root cause is:
bpf_prog_attach(BPF_SK_SKB_STREAM_VERDICT)
tcp_set_ulp //set ulp after sockmap add
icsk->icsk_ulp_ops = ulp_ops;
sock_hash_update_common
sock_map_unref
sock_map_del_link
psock->psock_update_sk_prot(sk, psock, false);
sk->sk_prot->close = sock_map_close
sk_psock_drop
sk_psock_restore_proto
tcp_bpf_update_proto
tls_update //not redo sk_prot to tcp prot
inet_release
sk->sk_prot->close
sock_map_close
WARN(sk->sk_prot->close == sock_map_close)
commit e34a07c0ae39 ("sock: redo the psock vs ULP protection check")
has moved ulp check from tcp_bpf_update_proto() to psock init.
If sk sets ulp after being added to sockmap, it will reset sk_prot to
BPF_BASE when removed from sockmap. After the psock is dropped, it will
not reset sk_prot back to the tcp prot, only tls context update is
performed. This can trigger a warning in sock_map_close() due to
recursion of sk->sk_prot->close.
To fix this issue, skip the sk_prot operations redo when deleting link
from sockmap if ULP is set.
Fixes: e34a07c0ae39 ("sock: redo the psock vs ULP protection check")
Fixes: c0d95d3380ee ("bpf, sockmap: Re-evaluate proto ops when psock is removed from sockmap")
Signed-off-by: Dong Chenchen <dongchenchen2@huawei.com>
---
net/core/sock_map.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 82a14f131d00..a3ed1f2cf8a2 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -170,7 +170,7 @@ static void sock_map_del_link(struct sock *sk,
if (verdict_stop)
sk_psock_stop_verdict(sk, psock);
- if (psock->psock_update_sk_prot)
+ if (!(sk_is_inet(sk) && inet_csk_has_ulp(sk)) && psock->psock_update_sk_prot)
psock->psock_update_sk_prot(sk, psock, false);
write_unlock_bh(&sk->sk_callback_lock);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH net 2/2] selftests: bpf: Add case for sockmap_ktls set when verdict attached
2025-03-14 8:20 [PATCH net 0/2] bpf, sockmap: Avoid sk_prot reset on sockmap unlink with ULP set Dong Chenchen
2025-03-14 8:20 ` [PATCH net 1/2] " Dong Chenchen
@ 2025-03-14 8:20 ` Dong Chenchen
1 sibling, 0 replies; 4+ messages in thread
From: Dong Chenchen @ 2025-03-14 8:20 UTC (permalink / raw)
To: edumazet, kuniyu, pabeni, willemb, john.fastabend, jakub, davem,
kuba, horms, daniel
Cc: netdev, bpf, stfomichev, mrpre, xiyou.wangcong, zhangchangzhong,
weiyongjun1, Dong Chenchen
Cover the scenario when close a socket after inserted into the sockmap
(verdict attach) and set ULP. It will trigger sock_map_close warning.
Signed-off-by: Dong Chenchen <dongchenchen2@huawei.com>
---
.../selftests/bpf/prog_tests/sockmap_ktls.c | 70 +++++++++++++++++++
1 file changed, 70 insertions(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c b/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c
index 2d0796314862..d54bd5f41d4d 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c
@@ -9,6 +9,7 @@
#define MAX_TEST_NAME 80
#define TCP_ULP 31
+#define SOCKMAP_VERDICT_PROG "test_sockmap_skb_verdict_attach.bpf.o"
static int tcp_server(int family)
{
@@ -132,6 +133,73 @@ static void test_sockmap_ktls_update_fails_when_sock_has_ulp(int family, int map
close(s);
}
+/* close a kTLS socket after removing it from sockmap. */
+static void test_sockmap_ktls_close_after_delete(int family, int map)
+{
+ struct sockaddr_storage addr = {0};
+ socklen_t len = sizeof(addr);
+ int err, cli, srv, zero = 0;
+ struct bpf_program *prog;
+ struct bpf_object *obj;
+ int verdict;
+
+ obj = bpf_object__open_file(SOCKMAP_VERDICT_PROG, NULL);
+ if (!ASSERT_OK(libbpf_get_error(obj), "bpf_object__open_file"))
+ return;
+
+ err = bpf_object__load(obj);
+ if (!ASSERT_OK(err, "bpf_object__load"))
+ goto close_obj;
+
+ prog = bpf_object__next_program(obj, NULL);
+ verdict = bpf_program__fd(prog);
+ if (!ASSERT_GE(verdict, 0, "bpf_program__fd"))
+ goto close_obj;
+
+ err = bpf_prog_attach(verdict, map, BPF_SK_SKB_STREAM_VERDICT, 0);
+ if (!ASSERT_OK(err, "bpf_prog_attach"))
+ goto close_verdict;
+
+ srv = tcp_server(family);
+ if (srv == -1)
+ goto detach;
+
+ err = getsockname(srv, (struct sockaddr *)&addr, &len);
+ if (!ASSERT_OK(err, "getsockopt"))
+ goto close_srv;
+
+ cli = socket(family, SOCK_STREAM, 0);
+ if (!ASSERT_GE(cli, 0, "socket"))
+ goto close_srv;
+
+ err = connect(cli, (struct sockaddr *)&addr, len);
+ if (!ASSERT_OK(err, "connect"))
+ goto close_cli;
+
+ err = bpf_map_update_elem(map, &zero, &cli, 0);
+ if (!ASSERT_OK(err, "bpf_map_update_elem"))
+ goto close_cli;
+
+ err = setsockopt(cli, IPPROTO_TCP, TCP_ULP, "tls", strlen("tls"));
+ if (!ASSERT_OK(err, "setsockopt(TCP_ULP)"))
+ goto close_cli;
+
+ err = bpf_map_delete_elem(map, &zero);
+ if (!ASSERT_OK(err, "bpf_map_delete_elem"))
+ goto close_cli;
+
+close_cli:
+ close(cli);
+close_srv:
+ close(srv);
+detach:
+ bpf_prog_detach2(verdict, map, BPF_SK_SKB_STREAM_VERDICT);
+close_verdict:
+ close(verdict);
+close_obj:
+ bpf_object__close(obj);
+}
+
static const char *fmt_test_name(const char *subtest_name, int family,
enum bpf_map_type map_type)
{
@@ -158,6 +226,8 @@ static void run_tests(int family, enum bpf_map_type map_type)
test_sockmap_ktls_disconnect_after_delete(family, map);
if (test__start_subtest(fmt_test_name("update_fails_when_sock_has_ulp", family, map_type)))
test_sockmap_ktls_update_fails_when_sock_has_ulp(family, map);
+ if (test__start_subtest(fmt_test_name("close_after_delete", family, map_type)))
+ test_sockmap_ktls_close_after_delete(family, map);
close(map);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread