[PATCH net 0/2] bpf, sockmap: Avoid sk_prot reset on sockmap unlink with ULP set

[PATCH net 0/2] bpf, sockmap: Avoid sk_prot reset on sockmap unlink with ULP set

[Dong Chenchen]

Avoid sk_prot reset on sockmap unlink with ULP set to fix warning on
recurse in sock_map_close().

dongchenchen (2):
  bpf, sockmap: Skip sk_prot ops redo when ulp set
  selftests: bpf: Add case for sockmap_ktls set when verdict attached

 net/core/sock_map.c                           |  2 +-
 .../selftests/bpf/prog_tests/sockmap_ktls.c   | 70 +++++++++++++++++++
 2 files changed, 71 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH net 1/2] bpf, sockmap: Avoid sk_prot reset on sockmap unlink with ULP set

[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

[PATCH net 2/2] selftests: bpf: Add case for sockmap_ktls set when verdict attached

[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

Re: [PATCH net 1/2] bpf, sockmap: Avoid sk_prot reset on sockmap unlink with ULP set

[Cong Wang]

On Fri, Mar 14, 2025 at 04:20:03PM +0800, Dong Chenchen wrote:
> 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)

This makes sense now. Please see my comment below.

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

Can we put this TCP-specific logic into tcp_bpf_update_proto() instead?

Something like this...

diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index ba581785adb4..0bb363447fc7 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -708,6 +708,8 @@ int tcp_bpf_update_proto(struct sock *sk, struct sk_psock *psock, bool restore)
                return 0;
        }

+       if (inet_csk_has_ulp(sk))
+               return 0;
        if (sk->sk_family == AF_INET6) {
                if (tcp_bpf_assert_proto_ops(psock->sk_proto))
                        return -EINVAL;