[PATCH bpf 0/6] selftest/bpf: Various sockmap-related fixes

[PATCH bpf 0/6] selftest/bpf: Various sockmap-related fixes

[Michal Luczaj]

Series takes care of few bugs and missing features with the aim to improve
the test coverage of sockmap/sockhash.

Last patch is a create_pair() rewrite making use of
__attribute__((cleanup)) to handle socket fd lifetime.

v0: https://lore.kernel.org/netdev/027fdb41-ee11-4be0-a493-22f28a1abd7c@rbox.co/
  - No declarations in function body (Jakub)
  - Don't touch output arguments until function succeeds (Jakub)

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
Michal Luczaj (6):
      selftest/bpf: Support more socket types in create_pair()
      selftest/bpf: Socket pair creation, cleanups
      selftest/bpf: Simplify inet_socketpair() and vsock_unix_redir_connectible()
      selftest/bpf: Respect the sotype of af_unix redir tests
      selftest/bpf: Exercise SOCK_STREAM unix_inet_redir_to_connected()
      selftest/bpf: Introduce __attribute__((cleanup)) in create_pair()

 .../selftests/bpf/prog_tests/sockmap_basic.c       |  28 ++--
 .../selftests/bpf/prog_tests/sockmap_helpers.h     | 145 ++++++++++++++-------
 .../selftests/bpf/prog_tests/sockmap_listen.c      | 117 ++---------------
 3 files changed, 120 insertions(+), 170 deletions(-)
---
base-commit: 13c9b702e6cb8e406d5fa6b2dca422fa42d2f13e
change-id: 20240723-sockmap-selftest-fixes-666769755137

Best regards,
-- 
Michal Luczaj <mhal@rbox.co>

[PATCH bpf 1/6] selftest/bpf: Support more socket types in create_pair()

[Michal Luczaj]

Extend the function to allow creating socket pairs of SOCK_STREAM,
SOCK_DGRAM and SOCK_SEQPACKET.

Adapt direct callers and leave further cleanups for the following patch.

Suggested-by: Jakub Sitnicki <jakub@cloudflare.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 .../selftests/bpf/prog_tests/sockmap_basic.c       |  19 +--
 .../selftests/bpf/prog_tests/sockmap_helpers.h     | 138 ++++++++++++++-------
 2 files changed, 96 insertions(+), 61 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
index 1337153eb0ad..5b17d69c9ee6 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
@@ -451,11 +451,11 @@ static void test_sockmap_progs_query(enum bpf_attach_type attach_type)
 #define MAX_EVENTS 10
 static void test_sockmap_skb_verdict_shutdown(void)
 {
+	int n, err, map, verdict, c1 = -1, p1 = -1;
 	struct epoll_event ev, events[MAX_EVENTS];
-	int n, err, map, verdict, s, c1 = -1, p1 = -1;
 	struct test_sockmap_pass_prog *skel;
-	int epollfd;
 	int zero = 0;
+	int epollfd;
 	char b;
 
 	skel = test_sockmap_pass_prog__open_and_load();
@@ -469,10 +469,7 @@ static void test_sockmap_skb_verdict_shutdown(void)
 	if (!ASSERT_OK(err, "bpf_prog_attach"))
 		goto out;
 
-	s = socket_loopback(AF_INET, SOCK_STREAM);
-	if (s < 0)
-		goto out;
-	err = create_pair(s, AF_INET, SOCK_STREAM, &c1, &p1);
+	err = create_pair(AF_INET, SOCK_STREAM, &c1, &p1);
 	if (err < 0)
 		goto out;
 
@@ -570,16 +567,12 @@ static void test_sockmap_skb_verdict_fionread(bool pass_prog)
 
 static void test_sockmap_skb_verdict_peek_helper(int map)
 {
-	int err, s, c1, p1, zero = 0, sent, recvd, avail;
+	int err, c1, p1, zero = 0, sent, recvd, avail;
 	char snd[256] = "0123456789";
 	char rcv[256] = "0";
 
-	s = socket_loopback(AF_INET, SOCK_STREAM);
-	if (!ASSERT_GT(s, -1, "socket_loopback(s)"))
-		return;
-
-	err = create_pair(s, AF_INET, SOCK_STREAM, &c1, &p1);
-	if (!ASSERT_OK(err, "create_pairs(s)"))
+	err = create_pair(AF_INET, SOCK_STREAM, &c1, &p1);
+	if (!ASSERT_OK(err, "create_pair()"))
 		return;
 
 	err = bpf_map_update_elem(map, &zero, &c1, BPF_NOEXIST);
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
index e880f97bc44d..77b73333f091 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
@@ -3,6 +3,9 @@
 
 #include <linux/vm_sockets.h>
 
+/* include/linux/net.h */
+#define SOCK_TYPE_MASK 0xf
+
 #define IO_TIMEOUT_SEC 30
 #define MAX_STRERR_LEN 256
 #define MAX_TEST_NAME 80
@@ -312,54 +315,6 @@ static inline int add_to_sockmap(int sock_mapfd, int fd1, int fd2)
 	return xbpf_map_update_elem(sock_mapfd, &key, &value, BPF_NOEXIST);
 }
 
-static inline int create_pair(int s, int family, int sotype, int *c, int *p)
-{
-	struct sockaddr_storage addr;
-	socklen_t len;
-	int err = 0;
-
-	len = sizeof(addr);
-	err = xgetsockname(s, sockaddr(&addr), &len);
-	if (err)
-		return err;
-
-	*c = xsocket(family, sotype, 0);
-	if (*c < 0)
-		return errno;
-	err = xconnect(*c, sockaddr(&addr), len);
-	if (err) {
-		err = errno;
-		goto close_cli0;
-	}
-
-	*p = xaccept_nonblock(s, NULL, NULL);
-	if (*p < 0) {
-		err = errno;
-		goto close_cli0;
-	}
-	return err;
-close_cli0:
-	close(*c);
-	return err;
-}
-
-static inline int create_socket_pairs(int s, int family, int sotype,
-				      int *c0, int *c1, int *p0, int *p1)
-{
-	int err;
-
-	err = create_pair(s, family, sotype, c0, p0);
-	if (err)
-		return err;
-
-	err = create_pair(s, family, sotype, c1, p1);
-	if (err) {
-		close(*c0);
-		close(*p0);
-	}
-	return err;
-}
-
 static inline int enable_reuseport(int s, int progfd)
 {
 	int err, one = 1;
@@ -412,5 +367,92 @@ static inline int socket_loopback(int family, int sotype)
 	return socket_loopback_reuseport(family, sotype, -1);
 }
 
+static inline int create_pair(int family, int sotype, int *p0, int *p1)
+{
+	struct sockaddr_storage addr;
+	socklen_t len = sizeof(addr);
+	int s, c, p, err;
+
+	s = socket_loopback(family, sotype);
+	if (s < 0)
+		return s;
+
+	err = xgetsockname(s, sockaddr(&addr), &len);
+	if (err)
+		goto close_s;
+
+	c = xsocket(family, sotype, 0);
+	if (c < 0) {
+		err = c;
+		goto close_s;
+	}
+
+	err = connect(c, sockaddr(&addr), len);
+	if (err) {
+		if (errno != EINPROGRESS) {
+			FAIL_ERRNO("connect");
+			goto close_c;
+		}
+
+		err = poll_connect(c, IO_TIMEOUT_SEC);
+		if (err) {
+			FAIL_ERRNO("poll_connect");
+			goto close_c;
+		}
+	}
+
+	switch (sotype & SOCK_TYPE_MASK) {
+	case SOCK_DGRAM:
+		err = xgetsockname(c, sockaddr(&addr), &len);
+		if (err)
+			goto close_c;
+
+		err = xconnect(s, sockaddr(&addr), len);
+		if (!err) {
+			*p0 = s;
+			*p1 = c;
+			return err;
+		}
+		break;
+	case SOCK_STREAM:
+	case SOCK_SEQPACKET:
+		p = xaccept_nonblock(s, NULL, NULL);
+		if (p >= 0) {
+			*p0 = p;
+			*p1 = c;
+			goto close_s;
+		}
+
+		err = p;
+		break;
+	default:
+		FAIL("Unsupported socket type %#x", sotype);
+		err = -EOPNOTSUPP;
+	}
+
+close_c:
+	close(c);
+close_s:
+	close(s);
+	return err;
+}
+
+static inline int create_socket_pairs(int s, int family, int sotype,
+				      int *c0, int *c1, int *p0, int *p1)
+{
+	int err;
+
+	err = create_pair(family, sotype, c0, p0);
+	if (err)
+		return err;
+
+	err = create_pair(family, sotype, c1, p1);
+	if (err) {
+		close(*c0);
+		close(*p0);
+	}
+
+	return err;
+}
 
 #endif // __SOCKMAP_HELPERS__

-- 
2.45.2

Re: [PATCH bpf 1/6] selftest/bpf: Support more socket types in create_pair()

[Jakub Sitnicki]

On Wed, Jul 24, 2024 at 01:32 PM +02, Michal Luczaj wrote:
> Extend the function to allow creating socket pairs of SOCK_STREAM,
> SOCK_DGRAM and SOCK_SEQPACKET.
>
> Adapt direct callers and leave further cleanups for the following patch.
>
> Suggested-by: Jakub Sitnicki <jakub@cloudflare.com>
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---
>  .../selftests/bpf/prog_tests/sockmap_basic.c       |  19 +--
>  .../selftests/bpf/prog_tests/sockmap_helpers.h     | 138 ++++++++++++++-------
>  2 files changed, 96 insertions(+), 61 deletions(-)
>

[...]

> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
> index e880f97bc44d..77b73333f091 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h

[...]

> +static inline int create_pair(int family, int sotype, int *p0, int *p1)
> +{
> +	struct sockaddr_storage addr;
> +	socklen_t len = sizeof(addr);
> +	int s, c, p, err;
> +
> +	s = socket_loopback(family, sotype);
> +	if (s < 0)
> +		return s;
> +
> +	err = xgetsockname(s, sockaddr(&addr), &len);
> +	if (err)
> +		goto close_s;
> +
> +	c = xsocket(family, sotype, 0);
> +	if (c < 0) {
> +		err = c;
> +		goto close_s;
> +	}
> +
> +	err = connect(c, sockaddr(&addr), len);
> +	if (err) {
> +		if (errno != EINPROGRESS) {
> +			FAIL_ERRNO("connect");
> +			goto close_c;
> +		}
> +
> +		err = poll_connect(c, IO_TIMEOUT_SEC);
> +		if (err) {
> +			FAIL_ERRNO("poll_connect");
> +			goto close_c;
> +		}
> +	}
> +
> +	switch (sotype & SOCK_TYPE_MASK) {
> +	case SOCK_DGRAM:
> +		err = xgetsockname(c, sockaddr(&addr), &len);
> +		if (err)
> +			goto close_c;
> +
> +		err = xconnect(s, sockaddr(&addr), len);
> +		if (!err) {
> +			*p0 = s;
> +			*p1 = c;
> +			return err;
> +		}
> +		break;
> +	case SOCK_STREAM:
> +	case SOCK_SEQPACKET:
> +		p = xaccept_nonblock(s, NULL, NULL);
> +		if (p >= 0) {
> +			*p0 = p;
> +			*p1 = c;
> +			goto close_s;
> +		}
> +
> +		err = p;
> +		break;
> +	default:
> +		FAIL("Unsupported socket type %#x", sotype);
> +		err = -EOPNOTSUPP;
> +	}
> +
> +close_c:
> +	close(c);
> +close_s:
> +	close(s);
> +	return err;
> +}

I was going to suggest that a single return path for success is better
than two (diff below), but I see that this is what you ended up with
after patch 6.

So I think we can leave it as is.

---
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
index 77b73333f091..ed266c6c0117 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
@@ -408,28 +408,31 @@ static inline int create_pair(int family, int sotype, int *p0, int *p1)
                        goto close_c;

                err = xconnect(s, sockaddr(&addr), len);
-               if (!err) {
-                       *p0 = s;
-                       *p1 = c;
-                       return err;
-               }
+               if (err)
+                       goto close_c;
+
+               p = s;
                break;
        case SOCK_STREAM:
        case SOCK_SEQPACKET:
                p = xaccept_nonblock(s, NULL, NULL);
-               if (p >= 0) {
-                       *p0 = p;
-                       *p1 = c;
-                       goto close_s;
+               if (p < 0) {
+                       err = p;
+                       goto close_c;
                }

-               err = p;
+               xclose(s);
                break;
        default:
                FAIL("Unsupported socket type %#x", sotype);
                err = -EOPNOTSUPP;
+               goto close_c;
        }

+       *p0 = p;
+       *p1 = c;
+       return 0;
+
 close_c:
        close(c);
 close_s:

Re: [PATCH bpf 1/6] selftest/bpf: Support more socket types in create_pair()

[Michal Luczaj]

On 7/26/24 19:23, Jakub Sitnicki wrote:
> I was going to suggest that a single return path for success is better
> than two (diff below), but I see that this is what you ended up with
> after patch 6.
> 
> So I think we can leave it as is.
> [...]

And speaking of which, would you rather have patch 1 and 6 squashed?

Re: [PATCH bpf 1/6] selftest/bpf: Support more socket types in create_pair()

[Jakub Sitnicki]

On Fri, Jul 26, 2024 at 10:29 PM +02, Michal Luczaj wrote:
> On 7/26/24 19:23, Jakub Sitnicki wrote:
>> I was going to suggest that a single return path for success is better
>> than two (diff below), but I see that this is what you ended up with
>> after patch 6.
>> 
>> So I think we can leave it as is.
>> [...]
>
> And speaking of which, would you rather have patch 1 and 6 squashed?

Don't have a straight answer, sorry . Would have to see if the diff is
clear enough after squashing it. Use your best judgement.

It's certainly fine with me to review the steps that were taken to
massage the code.

Re: [PATCH bpf 1/6] selftest/bpf: Support more socket types in create_pair()

[Michal Luczaj]

On 7/30/24 19:13, Jakub Sitnicki wrote:
> On Fri, Jul 26, 2024 at 10:29 PM +02, Michal Luczaj wrote:
>> On 7/26/24 19:23, Jakub Sitnicki wrote:
>>> I was going to suggest that a single return path for success is better
>>> than two (diff below), but I see that this is what you ended up with
>>> after patch 6.
>>>
>>> So I think we can leave it as is.
>>> [...]
>>
>> And speaking of which, would you rather have patch 1 and 6 squashed?
> 
> Don't have a straight answer, sorry . Would have to see if the diff is
> clear enough after squashing it. Use your best judgement.
> 
> It's certainly fine with me to review the steps that were taken to
> massage the code.

That's what I've assumed, thanks. So here's the bpf-next based respin:
https://lore.kernel.org/bpf/20240731-selftest-sockmap-fixes-v2-0-08a0c73abed2@rbox.co

[PATCH bpf 2/6] selftest/bpf: Socket pair creation, cleanups

[Michal Luczaj]

Following create_pair() changes, remove unused function argument in
create_socket_pairs() and adapt its callers, i.e. drop the open-coded
loopback socket creation.

Suggested-by: Jakub Sitnicki <jakub@cloudflare.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 .../selftests/bpf/prog_tests/sockmap_basic.c       |  9 +++-----
 .../selftests/bpf/prog_tests/sockmap_helpers.h     |  4 ++--
 .../selftests/bpf/prog_tests/sockmap_listen.c      | 26 +++++++---------------
 3 files changed, 13 insertions(+), 26 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
index 5b17d69c9ee6..82bfb266741c 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
@@ -503,8 +503,8 @@ static void test_sockmap_skb_verdict_shutdown(void)
 
 static void test_sockmap_skb_verdict_fionread(bool pass_prog)
 {
+	int err, map, verdict, c0 = -1, c1 = -1, p0 = -1, p1 = -1;
 	int expected, zero = 0, sent, recvd, avail;
-	int err, map, verdict, s, c0 = -1, c1 = -1, p0 = -1, p1 = -1;
 	struct test_sockmap_pass_prog *pass = NULL;
 	struct test_sockmap_drop_prog *drop = NULL;
 	char buf[256] = "0123456789";
@@ -531,11 +531,8 @@ static void test_sockmap_skb_verdict_fionread(bool pass_prog)
 	if (!ASSERT_OK(err, "bpf_prog_attach"))
 		goto out;
 
-	s = socket_loopback(AF_INET, SOCK_STREAM);
-	if (!ASSERT_GT(s, -1, "socket_loopback(s)"))
-		goto out;
-	err = create_socket_pairs(s, AF_INET, SOCK_STREAM, &c0, &c1, &p0, &p1);
-	if (!ASSERT_OK(err, "create_socket_pairs(s)"))
+	err = create_socket_pairs(AF_INET, SOCK_STREAM, &c0, &c1, &p0, &p1);
+	if (!ASSERT_OK(err, "create_socket_pairs()"))
 		goto out;
 
 	err = bpf_map_update_elem(map, &zero, &c1, BPF_NOEXIST);
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
index 77b73333f091..ead8ea4fd0da 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
@@ -437,8 +437,8 @@ static inline int create_pair(int family, int sotype, int *p0, int *p1)
 	return err;
 }
 
-static inline int create_socket_pairs(int s, int family, int sotype,
-				      int *c0, int *c1, int *p0, int *p1)
+static inline int create_socket_pairs(int family, int sotype, int *c0, int *c1,
+				      int *p0, int *p1)
 {
 	int err;
 
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index 9ce0e0e0b7da..bfbc217637d1 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -677,7 +677,7 @@ static void redir_to_connected(int family, int sotype, int sock_mapfd,
 			       int verd_mapfd, enum redir_mode mode)
 {
 	const char *log_prefix = redir_mode_str(mode);
-	int s, c0, c1, p0, p1;
+	int c0, c1, p0, p1;
 	unsigned int pass;
 	int err, n;
 	u32 key;
@@ -685,13 +685,10 @@ static void redir_to_connected(int family, int sotype, int sock_mapfd,
 
 	zero_verdict_count(verd_mapfd);
 
-	s = socket_loopback(family, sotype | SOCK_NONBLOCK);
-	if (s < 0)
-		return;
-
-	err = create_socket_pairs(s, family, sotype, &c0, &c1, &p0, &p1);
+	err = create_socket_pairs(family, sotype | SOCK_NONBLOCK, &c0, &c1,
+				  &p0, &p1);
 	if (err)
-		goto close_srv;
+		return;
 
 	err = add_to_sockmap(sock_mapfd, p0, p1);
 	if (err)
@@ -722,8 +719,6 @@ static void redir_to_connected(int family, int sotype, int sock_mapfd,
 	xclose(c1);
 	xclose(p0);
 	xclose(c0);
-close_srv:
-	xclose(s);
 }
 
 static void test_skb_redir_to_connected(struct test_sockmap_listen *skel,
@@ -909,7 +904,7 @@ static void test_msg_redir_to_listening_with_link(struct test_sockmap_listen *sk
 
 static void redir_partial(int family, int sotype, int sock_map, int parser_map)
 {
-	int s, c0 = -1, c1 = -1, p0 = -1, p1 = -1;
+	int c0 = -1, c1 = -1, p0 = -1, p1 = -1;
 	int err, n, key, value;
 	char buf[] = "abc";
 
@@ -919,13 +914,10 @@ static void redir_partial(int family, int sotype, int sock_map, int parser_map)
 	if (err)
 		return;
 
-	s = socket_loopback(family, sotype | SOCK_NONBLOCK);
-	if (s < 0)
-		goto clean_parser_map;
-
-	err = create_socket_pairs(s, family, sotype, &c0, &c1, &p0, &p1);
+	err = create_socket_pairs(family, sotype | SOCK_NONBLOCK, &c0, &c1,
+				  &p0, &p1);
 	if (err)
-		goto close_srv;
+		goto clean_parser_map;
 
 	err = add_to_sockmap(sock_map, p0, p1);
 	if (err)
@@ -944,8 +936,6 @@ static void redir_partial(int family, int sotype, int sock_map, int parser_map)
 	xclose(p0);
 	xclose(c1);
 	xclose(p1);
-close_srv:
-	xclose(s);
 
 clean_parser_map:
 	key = 0;

-- 
2.45.2

[PATCH bpf 3/6] selftest/bpf: Simplify inet_socketpair() and vsock_unix_redir_connectible()

[Michal Luczaj]

Replace implementation with a call to a generic function.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 .../selftests/bpf/prog_tests/sockmap_listen.c      | 83 +---------------------
 1 file changed, 2 insertions(+), 81 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index bfbc217637d1..ea2faacd146d 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -1490,49 +1490,7 @@ static void test_unix_redir(struct test_sockmap_listen *skel, struct bpf_map *ma
 /* Returns two connected loopback vsock sockets */
 static int vsock_socketpair_connectible(int sotype, int *v0, int *v1)
 {
-	struct sockaddr_storage addr;
-	socklen_t len = sizeof(addr);
-	int s, p, c;
-
-	s = socket_loopback(AF_VSOCK, sotype);
-	if (s < 0)
-		return -1;
-
-	c = xsocket(AF_VSOCK, sotype | SOCK_NONBLOCK, 0);
-	if (c == -1)
-		goto close_srv;
-
-	if (getsockname(s, sockaddr(&addr), &len) < 0)
-		goto close_cli;
-
-	if (connect(c, sockaddr(&addr), len) < 0 && errno != EINPROGRESS) {
-		FAIL_ERRNO("connect");
-		goto close_cli;
-	}
-
-	len = sizeof(addr);
-	p = accept_timeout(s, sockaddr(&addr), &len, IO_TIMEOUT_SEC);
-	if (p < 0)
-		goto close_cli;
-
-	if (poll_connect(c, IO_TIMEOUT_SEC) < 0) {
-		FAIL_ERRNO("poll_connect");
-		goto close_acc;
-	}
-
-	*v0 = p;
-	*v1 = c;
-
-	return 0;
-
-close_acc:
-	close(p);
-close_cli:
-	close(c);
-close_srv:
-	close(s);
-
-	return -1;
+	return create_pair(AF_VSOCK, sotype | SOCK_NONBLOCK, v0, v1);
 }
 
 static void vsock_unix_redir_connectible(int sock_mapfd, int verd_mapfd,
@@ -1681,44 +1639,7 @@ static void test_reuseport(struct test_sockmap_listen *skel,
 
 static int inet_socketpair(int family, int type, int *s, int *c)
 {
-	struct sockaddr_storage addr;
-	socklen_t len;
-	int p0, c0;
-	int err;
-
-	p0 = socket_loopback(family, type | SOCK_NONBLOCK);
-	if (p0 < 0)
-		return p0;
-
-	len = sizeof(addr);
-	err = xgetsockname(p0, sockaddr(&addr), &len);
-	if (err)
-		goto close_peer0;
-
-	c0 = xsocket(family, type | SOCK_NONBLOCK, 0);
-	if (c0 < 0) {
-		err = c0;
-		goto close_peer0;
-	}
-	err = xconnect(c0, sockaddr(&addr), len);
-	if (err)
-		goto close_cli0;
-	err = xgetsockname(c0, sockaddr(&addr), &len);
-	if (err)
-		goto close_cli0;
-	err = xconnect(p0, sockaddr(&addr), len);
-	if (err)
-		goto close_cli0;
-
-	*s = p0;
-	*c = c0;
-	return 0;
-
-close_cli0:
-	xclose(c0);
-close_peer0:
-	xclose(p0);
-	return err;
+	return create_pair(family, type | SOCK_NONBLOCK, s, c);
 }
 
 static void udp_redir_to_connected(int family, int sock_mapfd, int verd_mapfd,

-- 
2.45.2

Re: [PATCH bpf 3/6] selftest/bpf: Simplify inet_socketpair() and vsock_unix_redir_connectible()

[Michal Luczaj]

On 7/24/24 13:32, Michal Luczaj wrote:
> Replace implementation with a call to a generic function.
> 
> Signed-off-by: Michal Luczaj <mhal@rbox.co>

Patch subject is incorrect, it should be: "... and
vsock_socketpair_connectible()". Sorry for confusion.

Michal

[PATCH bpf 4/6] selftest/bpf: Respect the sotype of af_unix redir tests

[Michal Luczaj]

Do actually test the sotype as specified by the caller.

This picks up after commit 75e0e27db6cf ("selftest/bpf: Change udp to inet
in some function names").

Suggested-by: Jakub Sitnicki <jakub@cloudflare.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 tools/testing/selftests/bpf/prog_tests/sockmap_listen.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index ea2faacd146d..7ed223df5f12 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -1706,11 +1706,11 @@ static void inet_unix_redir_to_connected(int family, int type, int sock_mapfd,
 	int sfd[2];
 	int err;
 
-	if (socketpair(AF_UNIX, SOCK_DGRAM | SOCK_NONBLOCK, 0, sfd))
+	if (socketpair(AF_UNIX, type | SOCK_NONBLOCK, 0, sfd))
 		return;
 	c0 = sfd[0], p0 = sfd[1];
 
-	err = inet_socketpair(family, SOCK_DGRAM, &p1, &c1);
+	err = inet_socketpair(family, type, &p1, &c1);
 	if (err)
 		goto close;
 
@@ -1758,7 +1758,7 @@ static void unix_inet_redir_to_connected(int family, int type, int sock_mapfd,
 	int sfd[2];
 	int err;
 
-	err = inet_socketpair(family, SOCK_DGRAM, &p0, &c0);
+	err = inet_socketpair(family, type, &p0, &c0);
 	if (err)
 		return;
 

-- 
2.45.2

[PATCH bpf 5/6] selftest/bpf: Exercise SOCK_STREAM unix_inet_redir_to_connected()

[Michal Luczaj]

Constants got switched reducing the test's coverage. Replace SOCK_DGRAM
with SOCK_STREAM in one of unix_inet_skb_redir_to_connected() tests.

Fixes: 51354f700d40 ("bpf, sockmap: Add af_unix test with both sockets in map")
Suggested-by: Jakub Sitnicki <jakub@cloudflare.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 tools/testing/selftests/bpf/prog_tests/sockmap_listen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index 7ed223df5f12..da5a6fb03b69 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -1793,7 +1793,7 @@ static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,
 	unix_inet_redir_to_connected(family, SOCK_DGRAM,
 				     sock_map, -1, verdict_map,
 				     REDIR_EGRESS, NO_FLAGS);
-	unix_inet_redir_to_connected(family, SOCK_DGRAM,
+	unix_inet_redir_to_connected(family, SOCK_STREAM,
 				     sock_map, -1, verdict_map,
 				     REDIR_EGRESS, NO_FLAGS);
 

-- 
2.45.2

[PATCH bpf 6/6] selftest/bpf: Introduce __attribute__((cleanup)) in create_pair()

[Michal Luczaj]

Rewrite function to have (unneeded) socket descriptors automatically
close()d when leaving the scope. Make sure the "ownership" of fds is
correctly passed via take_fd(); i.e. descriptor returned to caller will
remain valid.

Suggested-by: Jakub Sitnicki <jakub@cloudflare.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 .../selftests/bpf/prog_tests/sockmap_helpers.h     | 57 ++++++++++++----------
 1 file changed, 32 insertions(+), 25 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
index ead8ea4fd0da..2e0f9fe459be 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
@@ -182,6 +182,21 @@
 		__ret;                                                         \
 	})
 
+#define take_fd(fd)                                                            \
+	({                                                                     \
+		__auto_type __val = (fd);                                      \
+		fd = -EBADF;                                                   \
+		__val;                                                         \
+	})
+
+static inline void close_fd(int *fd)
+{
+	if (*fd >= 0)
+		xclose(*fd);
+}
+
+#define __close_fd __attribute__((cleanup(close_fd)))
+
 static inline int poll_connect(int fd, unsigned int timeout_sec)
 {
 	struct timeval timeout = { .tv_sec = timeout_sec };
@@ -369,9 +384,10 @@ static inline int socket_loopback(int family, int sotype)
 
 static inline int create_pair(int family, int sotype, int *p0, int *p1)
 {
+	__close_fd int s, c = -1, p = -1;
 	struct sockaddr_storage addr;
 	socklen_t len = sizeof(addr);
-	int s, c, p, err;
+	int err;
 
 	s = socket_loopback(family, sotype);
 	if (s < 0)
@@ -379,25 +395,23 @@ static inline int create_pair(int family, int sotype, int *p0, int *p1)
 
 	err = xgetsockname(s, sockaddr(&addr), &len);
 	if (err)
-		goto close_s;
+		return err;
 
 	c = xsocket(family, sotype, 0);
-	if (c < 0) {
-		err = c;
-		goto close_s;
-	}
+	if (c < 0)
+		return c;
 
 	err = connect(c, sockaddr(&addr), len);
 	if (err) {
 		if (errno != EINPROGRESS) {
 			FAIL_ERRNO("connect");
-			goto close_c;
+			return err;
 		}
 
 		err = poll_connect(c, IO_TIMEOUT_SEC);
 		if (err) {
 			FAIL_ERRNO("poll_connect");
-			goto close_c;
+			return err;
 		}
 	}
 
@@ -405,36 +419,29 @@ static inline int create_pair(int family, int sotype, int *p0, int *p1)
 	case SOCK_DGRAM:
 		err = xgetsockname(c, sockaddr(&addr), &len);
 		if (err)
-			goto close_c;
+			return err;
 
 		err = xconnect(s, sockaddr(&addr), len);
-		if (!err) {
-			*p0 = s;
-			*p1 = c;
+		if (err)
 			return err;
-		}
+
+		*p0 = take_fd(s);
 		break;
 	case SOCK_STREAM:
 	case SOCK_SEQPACKET:
 		p = xaccept_nonblock(s, NULL, NULL);
-		if (p >= 0) {
-			*p0 = p;
-			*p1 = c;
-			goto close_s;
-		}
+		if (p < 0)
+			return p;
 
-		err = p;
+		*p0 = take_fd(p);
 		break;
 	default:
 		FAIL("Unsupported socket type %#x", sotype);
-		err = -EOPNOTSUPP;
+		return -EOPNOTSUPP;
 	}
 
-close_c:
-	close(c);
-close_s:
-	close(s);
-	return err;
+	*p1 = take_fd(c);
+	return 0;
 }
 
 static inline int create_socket_pairs(int family, int sotype, int *c0, int *c1,

-- 
2.45.2

Re: [PATCH bpf 6/6] selftest/bpf: Introduce __attribute__((cleanup)) in create_pair()

[Jakub Sitnicki]

On Wed, Jul 24, 2024 at 01:32 PM +02, Michal Luczaj wrote:
> Rewrite function to have (unneeded) socket descriptors automatically
> close()d when leaving the scope. Make sure the "ownership" of fds is
> correctly passed via take_fd(); i.e. descriptor returned to caller will
> remain valid.
>
> Suggested-by: Jakub Sitnicki <jakub@cloudflare.com>
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---
>  .../selftests/bpf/prog_tests/sockmap_helpers.h     | 57 ++++++++++++----------
>  1 file changed, 32 insertions(+), 25 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
> index ead8ea4fd0da..2e0f9fe459be 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
> @@ -182,6 +182,21 @@
>  		__ret;                                                         \
>  	})
>  
> +#define take_fd(fd)                                                            \
> +	({                                                                     \
> +		__auto_type __val = (fd);                                      \
> +		fd = -EBADF;                                                   \
> +		__val;                                                         \
> +	})

Probably should operate on a pointer to fd to avoid side effects, like
__get_and_null macro in include/linux/cleanup.h. take_fd is effectively
__get_and_null(fd, -EBADFD).

> +
> +static inline void close_fd(int *fd)
> +{
> +	if (*fd >= 0)
> +		xclose(*fd);
> +}
> +
> +#define __close_fd __attribute__((cleanup(close_fd)))
> +
>  static inline int poll_connect(int fd, unsigned int timeout_sec)
>  {
>  	struct timeval timeout = { .tv_sec = timeout_sec };
> @@ -369,9 +384,10 @@ static inline int socket_loopback(int family, int sotype)
>  
>  static inline int create_pair(int family, int sotype, int *p0, int *p1)
>  {
> +	__close_fd int s, c = -1, p = -1;
>  	struct sockaddr_storage addr;
>  	socklen_t len = sizeof(addr);
> -	int s, c, p, err;
> +	int err;
>  
>  	s = socket_loopback(family, sotype);
>  	if (s < 0)
> @@ -379,25 +395,23 @@ static inline int create_pair(int family, int sotype, int *p0, int *p1)
>  
>  	err = xgetsockname(s, sockaddr(&addr), &len);
>  	if (err)
> -		goto close_s;
> +		return err;
>  
>  	c = xsocket(family, sotype, 0);
> -	if (c < 0) {
> -		err = c;
> -		goto close_s;
> -	}
> +	if (c < 0)
> +		return c;
>  
>  	err = connect(c, sockaddr(&addr), len);
>  	if (err) {
>  		if (errno != EINPROGRESS) {
>  			FAIL_ERRNO("connect");
> -			goto close_c;
> +			return err;
>  		}
>  
>  		err = poll_connect(c, IO_TIMEOUT_SEC);
>  		if (err) {
>  			FAIL_ERRNO("poll_connect");
> -			goto close_c;
> +			return err;
>  		}
>  	}
>  
> @@ -405,36 +419,29 @@ static inline int create_pair(int family, int sotype, int *p0, int *p1)
>  	case SOCK_DGRAM:
>  		err = xgetsockname(c, sockaddr(&addr), &len);
>  		if (err)
> -			goto close_c;
> +			return err;
>  
>  		err = xconnect(s, sockaddr(&addr), len);
> -		if (!err) {
> -			*p0 = s;
> -			*p1 = c;
> +		if (err)
>  			return err;
> -		}
> +
> +		*p0 = take_fd(s);
>  		break;
>  	case SOCK_STREAM:
>  	case SOCK_SEQPACKET:
>  		p = xaccept_nonblock(s, NULL, NULL);
> -		if (p >= 0) {
> -			*p0 = p;
> -			*p1 = c;
> -			goto close_s;
> -		}
> +		if (p < 0)
> +			return p;
>  
> -		err = p;
> +		*p0 = take_fd(p);
>  		break;
>  	default:
>  		FAIL("Unsupported socket type %#x", sotype);
> -		err = -EOPNOTSUPP;
> +		return -EOPNOTSUPP;
>  	}
>  
> -close_c:
> -	close(c);
> -close_s:
> -	close(s);
> -	return err;
> +	*p1 = take_fd(c);
> +	return 0;
>  }
>  
>  static inline int create_socket_pairs(int family, int sotype, int *c0, int *c1,

This turned out nice and readable, IMHO.

Re: [PATCH bpf 6/6] selftest/bpf: Introduce __attribute__((cleanup)) in create_pair()

[Michal Luczaj]

On 7/26/24 19:27, Jakub Sitnicki wrote:
> On Wed, Jul 24, 2024 at 01:32 PM +02, Michal Luczaj wrote:
>> +#define take_fd(fd)                                                            \
>> +	({                                                                     \
>> +		__auto_type __val = (fd);                                      \
>> +		fd = -EBADF;                                                   \
>> +		__val;                                                         \
>> +	})
> 
> Probably should operate on a pointer to fd to avoid side effects, like
> __get_and_null macro in include/linux/cleanup.h. take_fd is effectively
> __get_and_null(fd, -EBADFD).
> [...]

OK, I'll just make use of stuff from cleanup.h.

Re: [PATCH bpf 0/6] selftest/bpf: Various sockmap-related fixes

[Jakub Sitnicki]

On Wed, Jul 24, 2024 at 01:32 PM +02, Michal Luczaj wrote:
> Series takes care of few bugs and missing features with the aim to improve
> the test coverage of sockmap/sockhash.
>
> Last patch is a create_pair() rewrite making use of
> __attribute__((cleanup)) to handle socket fd lifetime.
>
> v0: https://lore.kernel.org/netdev/027fdb41-ee11-4be0-a493-22f28a1abd7c@rbox.co/
>   - No declarations in function body (Jakub)
>   - Don't touch output arguments until function succeeds (Jakub)
>
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---

I see this depends on your previous series that got applied onto bpf
tree, but this seems more like bpf-next material considering it's all
tests, and a mix of improvements and fixups.

Re: [PATCH bpf 0/6] selftest/bpf: Various sockmap-related fixes

[Michal Luczaj]

On 7/26/24 19:36, Jakub Sitnicki wrote:
> On Wed, Jul 24, 2024 at 01:32 PM +02, Michal Luczaj wrote:
>> Series takes care of few bugs and missing features with the aim to improve
>> the test coverage of sockmap/sockhash.
>>
>> Last patch is a create_pair() rewrite making use of
>> __attribute__((cleanup)) to handle socket fd lifetime.
>>
>> v0: https://lore.kernel.org/netdev/027fdb41-ee11-4be0-a493-22f28a1abd7c@rbox.co/
>>   - No declarations in function body (Jakub)
>>   - Don't touch output arguments until function succeeds (Jakub)
>>
>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>> ---
> 
> I see this depends on your previous series that got applied onto bpf
> tree, but this seems more like bpf-next material considering it's all
> tests, and a mix of improvements and fixups.

Yeah, I guess you're right. I'll just wait for bpf-next to catch up, then
rebase and respin.