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

[PATCH net v2 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().

Dong Chenchen (2):
  bpf, sockmap: Avoid sk_prot reset on sockmap unlink with ULP set
  selftests: bpf: Add case for sockmap_ktls set when verdict attached

 net/ipv4/tcp_bpf.c                            |  3 +
 .../selftests/bpf/prog_tests/sockmap_ktls.c   | 70 +++++++++++++++++++
 2 files changed, 73 insertions(+)

-- 
2.25.1

[PATCH net v2 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")
Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Dong Chenchen <dongchenchen2@huawei.com>
---
Changes for v2:
- Move ULP check from sock_map_del_link() to tcp_bpf_update_proto()
---
 net/ipv4/tcp_bpf.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index ba581785adb4..01b3930947cc 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -708,6 +708,9 @@ 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;
-- 
2.25.1

[PATCH net v2 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.25.1

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

[John Fastabend]

On 2025-03-31 09:21:26, Dong Chenchen wrote:
> 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>
> ---

Nice Thanks.

Reviewed-by: John Fastabend <john.fastabend@gmail.com>

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

[John Fastabend]

On 2025-03-31 09:21:25, 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)
> 
> 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")
> Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>
> Signed-off-by: Dong Chenchen <dongchenchen2@huawei.com>
> ---
> Changes for v2:
> - Move ULP check from sock_map_del_link() to tcp_bpf_update_proto()
> ---
>  net/ipv4/tcp_bpf.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index ba581785adb4..01b3930947cc 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -708,6 +708,9 @@ int tcp_bpf_update_proto(struct sock *sk, struct sk_psock *psock, bool restore)
>  		return 0;
>  	}
>  

I think in this case we also want to update the ULP so that when it
tears down it will restore the correct proto ops?

Either tcp_bpf_update_proto should be called with restore=true or
this logic should be similar to above?

	if (inet_csk_has_ulp(sk)) {
		/* TLS does not have an unhash proto in SW cases,
		 * but we need to ensure we stop using the sock_map
		 * unhash routine because the associated psock is being
		 * removed. So use the original unhash handler.
		 */
		WRITE_ONCE(sk->sk_prot->unhash, psock->saved_unhash);
		tcp_update_ulp(sk, psock->sk_proto, psock->saved_write_space);

> +	if (inet_csk_has_ulp(sk))
> +		return 0;

I'm thinking of this stack of psock/ULPs,

   TCP ops -> sockmap psock -> TLS (ULP) ops

When ULP ops was initialized it stored sockmap ops in its ctx->sk_proto
so that it if it was removed it could restore the correct ops. Similar
sockmap psock stored TCP ops to restore if its removed. So if we remove
psock in the middle we shoudl tell the ULP to update its stored value.

The concerning flow might be,

   socket insert into sockmap (gets psock)
   socket adds ULP
   socket removed from socketmap (removes psock)
   socket removes ULP (restores original tcp ops hopefully)

Perhaps we could also add a test for this case to the 2nd patch?

Thanks,
John