[PATCH net 1/2] Revert "net/tls: fix tls_sk_proto_close executed repeatedly"

[PATCH net 1/2] Revert "net/tls: fix tls_sk_proto_close executed repeatedly"

[Jakub Kicinski]

This reverts commit 69135c572d1f84261a6de2a1268513a7e71753e2.

This commit was just papering over the issue, ULP should not
get ->update() called with its own sk_prot. Each ULP would
need to add this check.

Fixes: 69135c572d1f ("net/tls: fix tls_sk_proto_close executed repeatedly")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: borisp@nvidia.com
CC: john.fastabend@gmail.com
CC: william.xuanziyang@huawei.com
---
 net/tls/tls_main.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index 46bd5f26338b..da176411c1b5 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -921,9 +921,6 @@ static void tls_update(struct sock *sk, struct proto *p,
 {
 	struct tls_context *ctx;
 
-	if (sk->sk_prot == p)
-		return;
-
 	ctx = tls_get_ctx(sk);
 	if (likely(ctx)) {
 		ctx->sk_write_space = write_space;
-- 
2.36.1

[PATCH net 2/2] sock: redo the psock vs ULP protection check

[Jakub Kicinski]

Commit 8a59f9d1e3d4 ("sock: Introduce sk->sk_prot->psock_update_sk_prot()")
has moved the inet_csk_has_ulp(sk) check from sk_psock_init() to
the new tcp_bpf_update_proto() function. I'm guessing that this
was done to allow creating psocks for non-inet sockets.

Unfortunately the destruction path for psock includes the ULP
unwind, so we need to fail the sk_psock_init() itself.
Otherwise if ULP is already present we'll notice that later,
and call tcp_update_ulp() with the sk_proto of the ULP
itself, which will most likely result in the ULP looping
its callbacks.

Fixes: 8a59f9d1e3d4 ("sock: Introduce sk->sk_prot->psock_update_sk_prot()")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: john.fastabend@gmail.com
CC: jakub@cloudflare.com
CC: yoshfuji@linux-ipv6.org
CC: dsahern@kernel.org
CC: ast@kernel.org
CC: daniel@iogearbox.net
CC: andrii@kernel.org
CC: kafai@fb.com
CC: songliubraving@fb.com
CC: yhs@fb.com
CC: kpsingh@kernel.org
CC: borisp@nvidia.com
CC: cong.wang@bytedance.com
CC: bpf@vger.kernel.org
---
 include/net/inet_sock.h | 5 +++++
 net/core/skmsg.c        | 5 +++++
 net/ipv4/tcp_bpf.c      | 3 ---
 net/tls/tls_main.c      | 2 ++
 4 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h
index c1b5dcd6597c..daead5fb389a 100644
--- a/include/net/inet_sock.h
+++ b/include/net/inet_sock.h
@@ -253,6 +253,11 @@ struct inet_sock {
 #define IP_CMSG_CHECKSUM	BIT(7)
 #define IP_CMSG_RECVFRAGSIZE	BIT(8)
 
+static inline bool sk_is_inet(struct sock *sk)
+{
+	return sk->sk_family == AF_INET || sk->sk_family == AF_INET6;
+}
+
 /**
  * sk_to_full_sk - Access to a full socket
  * @sk: pointer to a socket
diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index 22b983ade0e7..b0fcd0200e84 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -699,6 +699,11 @@ struct sk_psock *sk_psock_init(struct sock *sk, int node)
 
 	write_lock_bh(&sk->sk_callback_lock);
 
+	if (sk_is_inet(sk) && inet_csk_has_ulp(sk)) {
+		psock = ERR_PTR(-EINVAL);
+		goto out;
+	}
+
 	if (sk->sk_user_data) {
 		psock = ERR_PTR(-EBUSY);
 		goto out;
diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index be3947e70fec..0d3f68bb51c0 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -611,9 +611,6 @@ int tcp_bpf_update_proto(struct sock *sk, struct sk_psock *psock, bool restore)
 		return 0;
 	}
 
-	if (inet_csk_has_ulp(sk))
-		return -EINVAL;
-
 	if (sk->sk_family == AF_INET6) {
 		if (tcp_bpf_assert_proto_ops(psock->sk_proto))
 			return -EINVAL;
diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index da176411c1b5..2ffede463e4a 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -921,6 +921,8 @@ static void tls_update(struct sock *sk, struct proto *p,
 {
 	struct tls_context *ctx;
 
+	WARN_ON_ONCE(sk->sk_prot == p);
+
 	ctx = tls_get_ctx(sk);
 	if (likely(ctx)) {
 		ctx->sk_write_space = write_space;
-- 
2.36.1

RE: [PATCH net 2/2] sock: redo the psock vs ULP protection check

[John Fastabend]

Jakub Kicinski wrote:
> Commit 8a59f9d1e3d4 ("sock: Introduce sk->sk_prot->psock_update_sk_prot()")
> has moved the inet_csk_has_ulp(sk) check from sk_psock_init() to
> the new tcp_bpf_update_proto() function. I'm guessing that this
> was done to allow creating psocks for non-inet sockets.
> 
> Unfortunately the destruction path for psock includes the ULP
> unwind, so we need to fail the sk_psock_init() itself.
> Otherwise if ULP is already present we'll notice that later,
> and call tcp_update_ulp() with the sk_proto of the ULP
> itself, which will most likely result in the ULP looping
> its callbacks.
> 
> Fixes: 8a59f9d1e3d4 ("sock: Introduce sk->sk_prot->psock_update_sk_prot()")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: john.fastabend@gmail.com
> CC: jakub@cloudflare.com
> CC: yoshfuji@linux-ipv6.org
> CC: dsahern@kernel.org
> CC: ast@kernel.org
> CC: daniel@iogearbox.net
> CC: andrii@kernel.org
> CC: kafai@fb.com
> CC: songliubraving@fb.com
> CC: yhs@fb.com
> CC: kpsingh@kernel.org
> CC: borisp@nvidia.com
> CC: cong.wang@bytedance.com
> CC: bpf@vger.kernel.org
> ---
>  include/net/inet_sock.h | 5 +++++
>  net/core/skmsg.c        | 5 +++++
>  net/ipv4/tcp_bpf.c      | 3 ---
>  net/tls/tls_main.c      | 2 ++
>  4 files changed, 12 insertions(+), 3 deletions(-)

Thanks Jakub this looks correct to me. I can't remember at the moment
or dig up in the email or git why it was originally moved into the
update logic.  Maybe Cong can comment seeing it was his original patch?
I seemed to have missed the error path on original review :(

From my side though,

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

RE: [PATCH net 1/2] Revert "net/tls: fix tls_sk_proto_close executed repeatedly"

[John Fastabend]

Jakub Kicinski wrote:
> This reverts commit 69135c572d1f84261a6de2a1268513a7e71753e2.
> 
> This commit was just papering over the issue, ULP should not
> get ->update() called with its own sk_prot. Each ULP would
> need to add this check.
> 
> Fixes: 69135c572d1f ("net/tls: fix tls_sk_proto_close executed repeatedly")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: borisp@nvidia.com
> CC: john.fastabend@gmail.com
> CC: william.xuanziyang@huawei.com
> ---
>  net/tls/tls_main.c | 3 ---
>  1 file changed, 3 deletions(-)

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

[PATCH net] selftests/bpf: Test sockmap update when socket has ULP

[Jakub Sitnicki]

Cover the scenario when we cannot insert a socket into the sockmap, because
it has it is using ULP. Failed insert should not have any effect on the ULP
state. This is a regression test.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
CC: john.fastabend@gmail.com
CC: jakub@cloudflare.com
CC: yoshfuji@linux-ipv6.org
CC: dsahern@kernel.org
CC: ast@kernel.org
CC: daniel@iogearbox.net
CC: andrii@kernel.org
CC: kafai@fb.com
CC: songliubraving@fb.com
CC: yhs@fb.com
CC: kpsingh@kernel.org
CC: borisp@nvidia.com
CC: cong.wang@bytedance.com
CC: bpf@vger.kernel.org
---
 .../selftests/bpf/prog_tests/sockmap_ktls.c   | 84 +++++++++++++++++--
 1 file changed, 75 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c b/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c
index af293ea1542c..86b0741d2464 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c
@@ -4,6 +4,7 @@
  * Tests for sockmap/sockhash holding kTLS sockets.
  */
 
+#include <netinet/tcp.h>
 #include "test_progs.h"
 
 #define MAX_TEST_NAME 80
@@ -92,9 +93,78 @@ static void test_sockmap_ktls_disconnect_after_delete(int family, int map)
 	close(srv);
 }
 
+static void test_sockmap_ktls_update_fails_when_sock_has_ulp(int family, int map)
+{
+	struct sockaddr_storage addr = {};
+	socklen_t len = sizeof(addr);
+	struct sockaddr_in6 *v6;
+	struct sockaddr_in *v4;
+	int err, s, zero = 0;
+
+	s = socket(family, SOCK_STREAM, 0);
+	if (!ASSERT_GE(s, 0, "socket"))
+		return;
+
+	switch (family) {
+	case AF_INET:
+		v4 = (struct sockaddr_in *)&addr;
+		v4->sin_family = AF_INET;
+		break;
+	case AF_INET6:
+		v6 = (struct sockaddr_in6 *)&addr;
+		v6->sin6_family = AF_INET6;
+		break;
+	default:
+		PRINT_FAIL("unsupported socket family %d", family);
+		return;
+	}
+
+	err = bind(s, (struct sockaddr *)&addr, len);
+	if (!ASSERT_OK(err, "bind"))
+		goto close;
+
+	err = getsockname(s, (struct sockaddr *)&addr, &len);
+	if (!ASSERT_OK(err, "getsockname"))
+		goto close;
+
+	err = connect(s, (struct sockaddr *)&addr, len);
+	if (!ASSERT_OK(err, "connect"))
+		goto close;
+
+	/* save sk->sk_prot and set it to tls_prots */
+	err = setsockopt(s, IPPROTO_TCP, TCP_ULP, "tls", strlen("tls"));
+	if (!ASSERT_OK(err, "setsockopt(TCP_ULP)"))
+		goto close;
+
+	/* sockmap update should not affect saved sk_prot */
+	err = bpf_map_update_elem(map, &zero, &s, BPF_ANY);
+	if (!ASSERT_ERR(err, "sockmap update elem"))
+		goto close;
+
+	/* call sk->sk_prot->setsockopt to dispatch to saved sk_prot */
+	err = setsockopt(s, IPPROTO_TCP, TCP_NODELAY, &zero, sizeof(zero));
+	ASSERT_OK(err, "setsockopt(TCP_NODELAY)");
+
+close:
+	close(s);
+}
+
+static const char *fmt_test_name(const char *subtest_name, int family,
+				 enum bpf_map_type map_type)
+{
+	const char *map_type_str = BPF_MAP_TYPE_SOCKMAP ? "SOCKMAP" : "SOCKHASH";
+	const char *family_str = AF_INET ? "IPv4" : "IPv6";
+	static char test_name[MAX_TEST_NAME];
+
+	snprintf(test_name, MAX_TEST_NAME,
+		 "sockmap_ktls %s %s %s",
+		 subtest_name, family_str, map_type_str);
+
+	return test_name;
+}
+
 static void run_tests(int family, enum bpf_map_type map_type)
 {
-	char test_name[MAX_TEST_NAME];
 	int map;
 
 	map = bpf_map_create(map_type, NULL, sizeof(int), sizeof(int), 1, NULL);
@@ -103,14 +173,10 @@ static void run_tests(int family, enum bpf_map_type map_type)
 		return;
 	}
 
-	snprintf(test_name, MAX_TEST_NAME,
-		 "sockmap_ktls disconnect_after_delete %s %s",
-		 family == AF_INET ? "IPv4" : "IPv6",
-		 map_type == BPF_MAP_TYPE_SOCKMAP ? "SOCKMAP" : "SOCKHASH");
-	if (!test__start_subtest(test_name))
-		return;
-
-	test_sockmap_ktls_disconnect_after_delete(family, map);
+	if (test__start_subtest(fmt_test_name("disconnect_after_delete", family, 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);
 
 	close(map);
 }
-- 
2.35.3

Re: [PATCH net 2/2] sock: redo the psock vs ULP protection check

[Jakub Sitnicki]

On Mon, Jun 20, 2022 at 12:13 PM -07, Jakub Kicinski wrote:
> Commit 8a59f9d1e3d4 ("sock: Introduce sk->sk_prot->psock_update_sk_prot()")
> has moved the inet_csk_has_ulp(sk) check from sk_psock_init() to
> the new tcp_bpf_update_proto() function. I'm guessing that this
> was done to allow creating psocks for non-inet sockets.
>
> Unfortunately the destruction path for psock includes the ULP
> unwind, so we need to fail the sk_psock_init() itself.
> Otherwise if ULP is already present we'll notice that later,
> and call tcp_update_ulp() with the sk_proto of the ULP
> itself, which will most likely result in the ULP looping
> its callbacks.
>
> Fixes: 8a59f9d1e3d4 ("sock: Introduce sk->sk_prot->psock_update_sk_prot()")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---

I followed up with a regression test, if you would like to pick it up
through net tree.

Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>
Tested-by: Jakub Sitnicki <jakub@cloudflare.com>

Re: [PATCH net 2/2] sock: redo the psock vs ULP protection check

[Jakub Kicinski]

On Wed, 22 Jun 2022 19:24:16 +0200 Jakub Sitnicki wrote:
> I followed up with a regression test, if you would like to pick it up
> through net tree.

Sweet, thanks!

> Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>
> Tested-by: Jakub Sitnicki <jakub@cloudflare.com>

RE: [PATCH net] selftests/bpf: Test sockmap update when socket has ULP

[John Fastabend]

Jakub Sitnicki wrote:
> Cover the scenario when we cannot insert a socket into the sockmap, because
> it has it is using ULP. Failed insert should not have any effect on the ULP
> state. This is a regression test.
> 
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> ---

Thanks, looks good. One small nit.

>  
> +#include <netinet/tcp.h>
>  #include "test_progs.h"
>  
>  #define MAX_TEST_NAME 80
> @@ -92,9 +93,78 @@ static void test_sockmap_ktls_disconnect_after_delete(int family, int map)
>  	close(srv);
>  }
>  
> +static void test_sockmap_ktls_update_fails_when_sock_has_ulp(int family, int map)
> +{
> +	struct sockaddr_storage addr = {};
> +	socklen_t len = sizeof(addr);
> +	struct sockaddr_in6 *v6;
> +	struct sockaddr_in *v4;
> +	int err, s, zero = 0;
> +
> +	s = socket(family, SOCK_STREAM, 0);
> +	if (!ASSERT_GE(s, 0, "socket"))
> +		return;
> +
> +	switch (family) {
> +	case AF_INET:
> +		v4 = (struct sockaddr_in *)&addr;
> +		v4->sin_family = AF_INET;
> +		break;
> +	case AF_INET6:
> +		v6 = (struct sockaddr_in6 *)&addr;
> +		v6->sin6_family = AF_INET6;
>k+		break;
> +	default:
> +		PRINT_FAIL("unsupported socket family %d", family);

Probably want goto close here right?

> +		return;
> +	}
> +
> +	err = bind(s, (struct sockaddr *)&addr, len);
> +	if (!ASSERT_OK(err, "bind"))
> +		goto close;
> +
> +	err = getsockname(s, (struct sockaddr *)&addr, &len);
> +	if (!ASSERT_OK(err, "getsockname"))
> +		goto close;
> +
> +	err = connect(s, (struct sockaddr *)&addr, len);
> +	if (!ASSERT_OK(err, "connect"))
> +		goto close;
> +
> +	/* save sk->sk_prot and set it to tls_prots */
> +	err = setsockopt(s, IPPROTO_TCP, TCP_ULP, "tls", strlen("tls"));
> +	if (!ASSERT_OK(err, "setsockopt(TCP_ULP)"))
> +		goto close;
> +
> +	/* sockmap update should not affect saved sk_prot */
> +	err = bpf_map_update_elem(map, &zero, &s, BPF_ANY);
> +	if (!ASSERT_ERR(err, "sockmap update elem"))
> +		goto close;
> +
> +	/* call sk->sk_prot->setsockopt to dispatch to saved sk_prot */
> +	err = setsockopt(s, IPPROTO_TCP, TCP_NODELAY, &zero, sizeof(zero));
> +	ASSERT_OK(err, "setsockopt(TCP_NODELAY)");
> +
> +close:
> +	close(s);

Re: [PATCH net 1/2] Revert "net/tls: fix tls_sk_proto_close executed repeatedly"

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (master)
by Paolo Abeni <pabeni@redhat.com>:

On Mon, 20 Jun 2022 12:13:52 -0700 you wrote:
> This reverts commit 69135c572d1f84261a6de2a1268513a7e71753e2.
> 
> This commit was just papering over the issue, ULP should not
> get ->update() called with its own sk_prot. Each ULP would
> need to add this check.
> 
> Fixes: 69135c572d1f ("net/tls: fix tls_sk_proto_close executed repeatedly")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> 
> [...]

Here is the summary with links:
  - [net,1/2] Revert "net/tls: fix tls_sk_proto_close executed repeatedly"
    https://git.kernel.org/netdev/net/c/1b205d948fbb
  - [net,2/2] sock: redo the psock vs ULP protection check
    https://git.kernel.org/netdev/net/c/e34a07c0ae39

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

[PATCH net v2] selftests/bpf: Test sockmap update when socket has ULP

[Jakub Sitnicki]

Cover the scenario when we cannot insert a socket into the sockmap, because
it has it is using ULP. Failed insert should not have any effect on the ULP
state. This is a regression test.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
v2:
- Don't leak open socket if family is not supported (John)

CC: john.fastabend@gmail.com
CC: jakub@cloudflare.com
CC: yoshfuji@linux-ipv6.org
CC: dsahern@kernel.org
CC: ast@kernel.org
CC: daniel@iogearbox.net
CC: andrii@kernel.org
CC: kafai@fb.com
CC: songliubraving@fb.com
CC: yhs@fb.com
CC: kpsingh@kernel.org
CC: borisp@nvidia.com
CC: cong.wang@bytedance.com
CC: bpf@vger.kernel.org
---
 .../selftests/bpf/prog_tests/sockmap_ktls.c   | 84 +++++++++++++++++--
 1 file changed, 75 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c b/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c
index af293ea1542c..e172d89e92e1 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c
@@ -4,6 +4,7 @@
  * Tests for sockmap/sockhash holding kTLS sockets.
  */
 
+#include <netinet/tcp.h>
 #include "test_progs.h"
 
 #define MAX_TEST_NAME 80
@@ -92,9 +93,78 @@ static void test_sockmap_ktls_disconnect_after_delete(int family, int map)
 	close(srv);
 }
 
+static void test_sockmap_ktls_update_fails_when_sock_has_ulp(int family, int map)
+{
+	struct sockaddr_storage addr = {};
+	socklen_t len = sizeof(addr);
+	struct sockaddr_in6 *v6;
+	struct sockaddr_in *v4;
+	int err, s, zero = 0;
+
+	switch (family) {
+	case AF_INET:
+		v4 = (struct sockaddr_in *)&addr;
+		v4->sin_family = AF_INET;
+		break;
+	case AF_INET6:
+		v6 = (struct sockaddr_in6 *)&addr;
+		v6->sin6_family = AF_INET6;
+		break;
+	default:
+		PRINT_FAIL("unsupported socket family %d", family);
+		return;
+	}
+
+	s = socket(family, SOCK_STREAM, 0);
+	if (!ASSERT_GE(s, 0, "socket"))
+		return;
+
+	err = bind(s, (struct sockaddr *)&addr, len);
+	if (!ASSERT_OK(err, "bind"))
+		goto close;
+
+	err = getsockname(s, (struct sockaddr *)&addr, &len);
+	if (!ASSERT_OK(err, "getsockname"))
+		goto close;
+
+	err = connect(s, (struct sockaddr *)&addr, len);
+	if (!ASSERT_OK(err, "connect"))
+		goto close;
+
+	/* save sk->sk_prot and set it to tls_prots */
+	err = setsockopt(s, IPPROTO_TCP, TCP_ULP, "tls", strlen("tls"));
+	if (!ASSERT_OK(err, "setsockopt(TCP_ULP)"))
+		goto close;
+
+	/* sockmap update should not affect saved sk_prot */
+	err = bpf_map_update_elem(map, &zero, &s, BPF_ANY);
+	if (!ASSERT_ERR(err, "sockmap update elem"))
+		goto close;
+
+	/* call sk->sk_prot->setsockopt to dispatch to saved sk_prot */
+	err = setsockopt(s, IPPROTO_TCP, TCP_NODELAY, &zero, sizeof(zero));
+	ASSERT_OK(err, "setsockopt(TCP_NODELAY)");
+
+close:
+	close(s);
+}
+
+static const char *fmt_test_name(const char *subtest_name, int family,
+				 enum bpf_map_type map_type)
+{
+	const char *map_type_str = BPF_MAP_TYPE_SOCKMAP ? "SOCKMAP" : "SOCKHASH";
+	const char *family_str = AF_INET ? "IPv4" : "IPv6";
+	static char test_name[MAX_TEST_NAME];
+
+	snprintf(test_name, MAX_TEST_NAME,
+		 "sockmap_ktls %s %s %s",
+		 subtest_name, family_str, map_type_str);
+
+	return test_name;
+}
+
 static void run_tests(int family, enum bpf_map_type map_type)
 {
-	char test_name[MAX_TEST_NAME];
 	int map;
 
 	map = bpf_map_create(map_type, NULL, sizeof(int), sizeof(int), 1, NULL);
@@ -103,14 +173,10 @@ static void run_tests(int family, enum bpf_map_type map_type)
 		return;
 	}
 
-	snprintf(test_name, MAX_TEST_NAME,
-		 "sockmap_ktls disconnect_after_delete %s %s",
-		 family == AF_INET ? "IPv4" : "IPv6",
-		 map_type == BPF_MAP_TYPE_SOCKMAP ? "SOCKMAP" : "SOCKHASH");
-	if (!test__start_subtest(test_name))
-		return;
-
-	test_sockmap_ktls_disconnect_after_delete(family, map);
+	if (test__start_subtest(fmt_test_name("disconnect_after_delete", family, 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);
 
 	close(map);
 }
-- 
2.35.3

Re: [PATCH net] selftests/bpf: Test sockmap update when socket has ULP

[Jakub Sitnicki]

On Wed, Jun 22, 2022 at 10:42 PM -07, John Fastabend wrote:
> Jakub Sitnicki wrote:
>> Cover the scenario when we cannot insert a socket into the sockmap, because
>> it has it is using ULP. Failed insert should not have any effect on the ULP
>> state. This is a regression test.
>> 
>> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
>> ---
>
> Thanks, looks good. One small nit.
>
>>  
>> +#include <netinet/tcp.h>
>>  #include "test_progs.h"
>>  
>>  #define MAX_TEST_NAME 80
>> @@ -92,9 +93,78 @@ static void test_sockmap_ktls_disconnect_after_delete(int family, int map)
>>  	close(srv);
>>  }
>>  
>> +static void test_sockmap_ktls_update_fails_when_sock_has_ulp(int family, int map)
>> +{
>> +	struct sockaddr_storage addr = {};
>> +	socklen_t len = sizeof(addr);
>> +	struct sockaddr_in6 *v6;
>> +	struct sockaddr_in *v4;
>> +	int err, s, zero = 0;
>> +
>> +	s = socket(family, SOCK_STREAM, 0);
>> +	if (!ASSERT_GE(s, 0, "socket"))
>> +		return;
>> +
>> +	switch (family) {
>> +	case AF_INET:
>> +		v4 = (struct sockaddr_in *)&addr;
>> +		v4->sin_family = AF_INET;
>> +		break;
>> +	case AF_INET6:
>> +		v6 = (struct sockaddr_in6 *)&addr;
>> +		v6->sin6_family = AF_INET6;
>>k+		break;
>> +	default:
>> +		PRINT_FAIL("unsupported socket family %d", family);
>
> Probably want goto close here right?

Ah, thanks. Sent v2. I hope we can borrow a trick from systemd's book
and adapt __attribute__((cleanup(f))) in the future.

[...]

Re: [PATCH net v2] selftests/bpf: Test sockmap update when socket has ULP

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 23 Jun 2022 11:12:31 +0200 you wrote:
> Cover the scenario when we cannot insert a socket into the sockmap, because
> it has it is using ULP. Failed insert should not have any effect on the ULP
> state. This is a regression test.
> 
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> ---
> v2:
> - Don't leak open socket if family is not supported (John)
> 
> [...]

Here is the summary with links:
  - [net,v2] selftests/bpf: Test sockmap update when socket has ULP
    https://git.kernel.org/netdev/net/c/935336c19104

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html