[PATCH bpf-next v2 0/2] cleanups for sockmap_listen

List overview All Threads
Download

newer

older

[PATCH v5 0/8] Add printf...

[patch net-next] selftests:...

Geliang Tang

5 Oct 2023 5 Oct '23

7:21 a.m.

v2: - rename c0/c1 to cli0/cli1, p0/p1 to peer0/perr1 as Daniel suggested.

Two cleanups for sockmap_listen selftests: enable a kconfig and add a new helper.

Geliang Tang (2): selftests/bpf: Enable CONFIG_VSOCKETS in config selftests/bpf: Add pairs_redir_to_connected helper

tools/testing/selftests/bpf/config | 1 + .../selftests/bpf/prog_tests/sockmap_listen.c | 159 ++++-------------- 2 files changed, 36 insertions(+), 124 deletions(-)

-- 2.35.3

Show replies by date

Geliang Tang

5 Oct 5 Oct

7:21 a.m.

New subject: [PATCH bpf-next v2 1/2] selftests/bpf: Enable CONFIG_VSOCKETS in config

CONFIG_VSOCKETS is required by BPF selftests, otherwise we get errors like this:

./test_progs:socket_loopback_reuseport:386: socket: Address family not supported by protocol socket_loopback_reuseport:FAIL:386 ./test_progs:vsock_unix_redir_connectible:1496: vsock_socketpair_connectible() failed vsock_unix_redir_connectible:FAIL:1496

So this patch enables it in tools/testing/selftests/bpf/config.

Signed-off-by: Geliang Tang geliang.tang@suse.com --- tools/testing/selftests/bpf/config | 1 + 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config index e41eb33b2704..02dd4409200e 100644 --- a/tools/testing/selftests/bpf/config +++ b/tools/testing/selftests/bpf/config @@ -84,3 +84,4 @@ CONFIG_USERFAULTFD=y CONFIG_VXLAN=y CONFIG_XDP_SOCKETS=y CONFIG_XFRM_INTERFACE=y +CONFIG_VSOCKETS=y

-- 2.35.3

Geliang Tang

7:21 a.m.

New subject: [PATCH bpf-next v2 2/2] selftests/bpf: Add pairs_redir_to_connected helper

Extract duplicate code from these four functions

unix_redir_to_connected() udp_redir_to_connected() inet_unix_redir_to_connected() unix_inet_redir_to_connected()

to create a new helper pairs_redir_to_connected(). Create the different socketpairs in these four functions, then pass the socketpairs info to the new common helper to do the connections.

Signed-off-by: Geliang Tang geliang.tang@suse.com --- .../selftests/bpf/prog_tests/sockmap_listen.c | 159 ++++-------------- 1 file changed, 35 insertions(+), 124 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c index 8df8cbb447f1..74cbce11e22a 100644 --- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c @@ -1336,32 +1336,22 @@ static void test_redir(struct test_sockmap_listen *skel, struct bpf_map *map, } }

-static void unix_redir_to_connected(int sotype, int sock_mapfd, - int verd_mapfd, enum redir_mode mode) +static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1, + int sock_mapfd, int verd_mapfd, enum redir_mode mode) { const char *log_prefix = redir_mode_str(mode); - int c0, c1, p0, p1; unsigned int pass; int err, n; - int sfd[2]; u32 key; char b;

zero_verdict_count(verd_mapfd);

- if (socketpair(AF_UNIX, sotype | SOCK_NONBLOCK, 0, sfd)) - return; - c0 = sfd[0], p0 = sfd[1]; - - if (socketpair(AF_UNIX, sotype | SOCK_NONBLOCK, 0, sfd)) - goto close0; - c1 = sfd[0], p1 = sfd[1]; - - err = add_to_sockmap(sock_mapfd, p0, p1); + err = add_to_sockmap(sock_mapfd, peer0, peer1); if (err) goto close;

- n = write(c1, "a", 1); + n = write(cli1, "a", 1); if (n < 0) FAIL_ERRNO("%s: write", log_prefix); if (n == 0) @@ -1376,16 +1366,34 @@ static void unix_redir_to_connected(int sotype, int sock_mapfd, if (pass != 1) FAIL("%s: want pass count 1, have %d", log_prefix, pass);

- n = recv_timeout(mode == REDIR_INGRESS ? p0 : c0, &b, 1, 0, IO_TIMEOUT_SEC); + n = recv_timeout(mode == REDIR_INGRESS ? peer0 : cli0, &b, 1, 0, IO_TIMEOUT_SEC); if (n < 0) FAIL_ERRNO("%s: recv_timeout", log_prefix); if (n == 0) FAIL("%s: incomplete recv", log_prefix);

close: - xclose(c1); - xclose(p1); -close0: + xclose(cli1); + xclose(peer1); +} + +static void unix_redir_to_connected(int sotype, int sock_mapfd, + int verd_mapfd, enum redir_mode mode) +{ + int c0, c1, p0, p1; + int sfd[2]; + + if (socketpair(AF_UNIX, sotype | SOCK_NONBLOCK, 0, sfd)) + return; + c0 = sfd[0], p0 = sfd[1]; + + if (socketpair(AF_UNIX, sotype | SOCK_NONBLOCK, 0, sfd)) + goto close; + c1 = sfd[0], p1 = sfd[1]; + + pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, verd_mapfd, mode); + +close: xclose(c0); xclose(p0); } @@ -1661,51 +1669,19 @@ static int inet_socketpair(int family, int type, int *s, int *c) static void udp_redir_to_connected(int family, int sock_mapfd, int verd_mapfd, enum redir_mode mode) { - const char *log_prefix = redir_mode_str(mode); int c0, c1, p0, p1; - unsigned int pass; - int err, n; - u32 key; - char b; - - zero_verdict_count(verd_mapfd); + int err;

err = inet_socketpair(family, SOCK_DGRAM, &p0, &c0); if (err) return; err = inet_socketpair(family, SOCK_DGRAM, &p1, &c1); if (err) - goto close_cli0; - - err = add_to_sockmap(sock_mapfd, p0, p1); - if (err) - goto close_cli1; - - n = write(c1, "a", 1); - if (n < 0) - FAIL_ERRNO("%s: write", log_prefix); - if (n == 0) - FAIL("%s: incomplete write", log_prefix); - if (n < 1) - goto close_cli1; - - key = SK_PASS; - err = xbpf_map_lookup_elem(verd_mapfd, &key, &pass); - if (err) - goto close_cli1; - if (pass != 1) - FAIL("%s: want pass count 1, have %d", log_prefix, pass); + goto close;

- n = recv_timeout(mode == REDIR_INGRESS ? p0 : c0, &b, 1, 0, IO_TIMEOUT_SEC); - if (n < 0) - FAIL_ERRNO("%s: recv_timeout", log_prefix); - if (n == 0) - FAIL("%s: incomplete recv", log_prefix); + pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, verd_mapfd, mode);

-close_cli1: - xclose(c1); - xclose(p1); -close_cli0: +close: xclose(c0); xclose(p0); } @@ -1747,15 +1723,9 @@ static void test_udp_redir(struct test_sockmap_listen *skel, struct bpf_map *map static void inet_unix_redir_to_connected(int family, int type, int sock_mapfd, int verd_mapfd, enum redir_mode mode) { - const char *log_prefix = redir_mode_str(mode); int c0, c1, p0, p1; - unsigned int pass; - int err, n; int sfd[2]; - u32 key; - char b; - - zero_verdict_count(verd_mapfd); + int err;

if (socketpair(AF_UNIX, SOCK_DGRAM | SOCK_NONBLOCK, 0, sfd)) return; @@ -1765,34 +1735,8 @@ static void inet_unix_redir_to_connected(int family, int type, int sock_mapfd, if (err) goto close;

- err = add_to_sockmap(sock_mapfd, p0, p1); - if (err) - goto close_cli1; - - n = write(c1, "a", 1); - if (n < 0) - FAIL_ERRNO("%s: write", log_prefix); - if (n == 0) - FAIL("%s: incomplete write", log_prefix); - if (n < 1) - goto close_cli1; + pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, verd_mapfd, mode);

- key = SK_PASS; - err = xbpf_map_lookup_elem(verd_mapfd, &key, &pass); - if (err) - goto close_cli1; - if (pass != 1) - FAIL("%s: want pass count 1, have %d", log_prefix, pass); - - n = recv_timeout(mode == REDIR_INGRESS ? p0 : c0, &b, 1, 0, IO_TIMEOUT_SEC); - if (n < 0) - FAIL_ERRNO("%s: recv_timeout", log_prefix); - if (n == 0) - FAIL("%s: incomplete recv", log_prefix); - -close_cli1: - xclose(c1); - xclose(p1); close: xclose(c0); xclose(p0); @@ -1827,56 +1771,23 @@ static void inet_unix_skb_redir_to_connected(struct test_sockmap_listen *skel, static void unix_inet_redir_to_connected(int family, int type, int sock_mapfd, int verd_mapfd, enum redir_mode mode) { - const char *log_prefix = redir_mode_str(mode); int c0, c1, p0, p1; - unsigned int pass; - int err, n; int sfd[2]; - u32 key; - char b; - - zero_verdict_count(verd_mapfd); + int err;

err = inet_socketpair(family, SOCK_DGRAM, &p0, &c0); if (err) return;

if (socketpair(AF_UNIX, SOCK_DGRAM | SOCK_NONBLOCK, 0, sfd)) - goto close_cli0; - c1 = sfd[0], p1 = sfd[1]; - - err = add_to_sockmap(sock_mapfd, p0, p1); - if (err) - goto close; - - n = write(c1, "a", 1); - if (n < 0) - FAIL_ERRNO("%s: write", log_prefix); - if (n == 0) - FAIL("%s: incomplete write", log_prefix); - if (n < 1) goto close; + c1 = sfd[0], p1 = sfd[1];

- key = SK_PASS; - err = xbpf_map_lookup_elem(verd_mapfd, &key, &pass); - if (err) - goto close; - if (pass != 1) - FAIL("%s: want pass count 1, have %d", log_prefix, pass); - - n = recv_timeout(mode == REDIR_INGRESS ? p0 : c0, &b, 1, 0, IO_TIMEOUT_SEC); - if (n < 0) - FAIL_ERRNO("%s: recv_timeout", log_prefix); - if (n == 0) - FAIL("%s: incomplete recv", log_prefix); + pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, verd_mapfd, mode);

close: - xclose(c1); - xclose(p1); -close_cli0: xclose(c0); xclose(p0); - }

static void unix_inet_skb_redir_to_connected(struct test_sockmap_listen *skel,

-- 2.35.3

Martin KaFai Lau

6 Oct 6 Oct

5:18 a.m.

New subject: [PATCH bpf-next v2 2/2] selftests/bpf: Add pairs_redir_to_connected helper

On 10/5/23 12:21 AM, Geliang Tang wrote:

...

--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c @@ -1336,32 +1336,22 @@ static void test_redir(struct test_sockmap_listen *skel, struct bpf_map *map, } } -static void unix_redir_to_connected(int sotype, int sock_mapfd,
	       int verd_mapfd, enum redir_mode mode)
+static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
		     int sock_mapfd, int verd_mapfd, enum redir_mode mode)
{ const char *log_prefix = redir_mode_str(mode);
int c0, c1, p0, p1; unsigned int pass; int err, n;

int sfd[2]; u32 key; char b;

zero_verdict_count(verd_mapfd);
if (socketpair(AF_UNIX, sotype | SOCK_NONBLOCK, 0, sfd))
return;
c0 = sfd[0], p0 = sfd[1];

if (socketpair(AF_UNIX, sotype | SOCK_NONBLOCK, 0, sfd))
goto close0;
c1 = sfd[0], p1 = sfd[1];

err = add_to_sockmap(sock_mapfd, p0, p1);
err = add_to_sockmap(sock_mapfd, peer0, peer1); if (err) goto close;

n = write(c1, "a", 1);

n = write(cli1, "a", 1); if (n < 0) FAIL_ERRNO("%s: write", log_prefix); if (n == 0)

@@ -1376,16 +1366,34 @@ static void unix_redir_to_connected(int sotype, int sock_mapfd, if (pass != 1) FAIL("%s: want pass count 1, have %d", log_prefix, pass);

n = recv_timeout(mode == REDIR_INGRESS ? p0 : c0, &b, 1, 0, IO_TIMEOUT_SEC);

n = recv_timeout(mode == REDIR_INGRESS ? peer0 : cli0, &b, 1, 0, IO_TIMEOUT_SEC); if (n < 0) FAIL_ERRNO("%s: recv_timeout", log_prefix); if (n == 0) FAIL("%s: incomplete recv", log_prefix);

close:

xclose(c1);

xclose(p1);

-close0:

xclose(cli1);

xclose(peer1);

+}

+static void unix_redir_to_connected(int sotype, int sock_mapfd,
	       int verd_mapfd, enum redir_mode mode)
+{
int c0, c1, p0, p1;

int sfd[2];

if (socketpair(AF_UNIX, sotype | SOCK_NONBLOCK, 0, sfd))
return;
c0 = sfd[0], p0 = sfd[1];

if (socketpair(AF_UNIX, sotype | SOCK_NONBLOCK, 0, sfd))
goto close;
c1 = sfd[0], p1 = sfd[1];

pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, verd_mapfd, mode);
+close: xclose(c0); xclose(p0); } @@ -1661,51 +1669,19 @@ static int inet_socketpair(int family, int type, int *s, int *c) static void udp_redir_to_connected(int family, int sock_mapfd, int verd_mapfd, enum redir_mode mode) {

const char *log_prefix = redir_mode_str(mode); int c0, c1, p0, p1;

unsigned int pass;

int err, n;

u32 key;

char b;

zero_verdict_count(verd_mapfd);

int err;

err = inet_socketpair(family, SOCK_DGRAM, &p0, &c0); if (err) return; err = inet_socketpair(family, SOCK_DGRAM, &p1, &c1); if (err)
goto close_cli0;
err = add_to_sockmap(sock_mapfd, p0, p1);

if (err)
goto close_cli1;
n = write(c1, "a", 1);

if (n < 0)
FAIL_ERRNO("%s: write", log_prefix);
if (n == 0)
FAIL("%s: incomplete write", log_prefix);
if (n < 1)
goto close_cli1;
key = SK_PASS;

err = xbpf_map_lookup_elem(verd_mapfd, &key, &pass);

if (err)
goto close_cli1;
if (pass != 1)
FAIL("%s: want pass count 1, have %d", log_prefix, pass);
goto close;
n = recv_timeout(mode == REDIR_INGRESS ? p0 : c0, &b, 1, 0, IO_TIMEOUT_SEC);

if (n < 0)
FAIL_ERRNO("%s: recv_timeout", log_prefix);
if (n == 0)
FAIL("%s: incomplete recv", log_prefix);
pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, verd_mapfd, mode);

-close_cli1:

xclose(c1);

xclose(p1);

-close_cli0: +close: xclose(c0); xclose(p0);

Patch 1 is applied. Thanks.

In patch 2, the xclose() here is confusing after this change. It is also inconsistent from how other tests in sockmap_listen.c is doing it. c0/p0 and c1/p1 are opened here but only c0/p0 is closed here and c1/p1 is closed in the pairs_redir_to_connected() above instead.

Jakub Sitnicki

13 Oct 13 Oct

10:40 a.m.

New subject: [PATCH bpf-next v2 2/2] selftests/bpf: Add pairs_redir_to_connected helper

On Thu, Oct 05, 2023 at 10:18 PM -07, Martin KaFai Lau wrote:

...

On 10/5/23 12:21 AM, Geliang Tang wrote:

...
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c @@ -1336,32 +1336,22 @@ static void test_redir(struct test_sockmap_listen *skel, struct bpf_map *map, } } -static void unix_redir_to_connected(int sotype, int sock_mapfd,
	       int verd_mapfd, enum redir_mode mode)
+static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
		     int sock_mapfd, int verd_mapfd, enum redir_mode mode)
{ const char *log_prefix = redir_mode_str(mode);
int c0, c1, p0, p1; unsigned int pass; int err, n;

int sfd[2]; u32 key; char b; zero_verdict_count(verd_mapfd);

if (socketpair(AF_UNIX, sotype | SOCK_NONBLOCK, 0, sfd))
return;
c0 = sfd[0], p0 = sfd[1];

if (socketpair(AF_UNIX, sotype | SOCK_NONBLOCK, 0, sfd))
goto close0;
c1 = sfd[0], p1 = sfd[1];

err = add_to_sockmap(sock_mapfd, p0, p1);
err = add_to_sockmap(sock_mapfd, peer0, peer1); if (err) goto close;

n = write(c1, "a", 1);

n = write(cli1, "a", 1); if (n < 0) FAIL_ERRNO("%s: write", log_prefix); if (n == 0)

@@ -1376,16 +1366,34 @@ static void unix_redir_to_connected(int sotype, int sock_mapfd, if (pass != 1) FAIL("%s: want pass count 1, have %d", log_prefix, pass);

n = recv_timeout(mode == REDIR_INGRESS ? p0 : c0, &b, 1, 0,

IO_TIMEOUT_SEC);

n = recv_timeout(mode == REDIR_INGRESS ? peer0 : cli0, &b, 1, 0, IO_TIMEOUT_SEC); if (n < 0) FAIL_ERRNO("%s: recv_timeout", log_prefix); if (n == 0) FAIL("%s: incomplete recv", log_prefix); close:

xclose(c1);

xclose(p1);

-close0:

xclose(cli1);

xclose(peer1);

+}

+static void unix_redir_to_connected(int sotype, int sock_mapfd,
	       int verd_mapfd, enum redir_mode mode)
+{
int c0, c1, p0, p1;

int sfd[2];

if (socketpair(AF_UNIX, sotype | SOCK_NONBLOCK, 0, sfd))
return;
c0 = sfd[0], p0 = sfd[1];

if (socketpair(AF_UNIX, sotype | SOCK_NONBLOCK, 0, sfd))
goto close;
c1 = sfd[0], p1 = sfd[1];

pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, verd_mapfd, mode);
+close: xclose(c0); xclose(p0); } @@ -1661,51 +1669,19 @@ static int inet_socketpair(int family, int type, int *s, int *c) static void udp_redir_to_connected(int family, int sock_mapfd, int verd_mapfd, enum redir_mode mode) {

const char *log_prefix = redir_mode_str(mode); int c0, c1, p0, p1;

unsigned int pass;

int err, n;

u32 key;

char b;

zero_verdict_count(verd_mapfd);

int err; err = inet_socketpair(family, SOCK_DGRAM, &p0, &c0); if (err) return; err = inet_socketpair(family, SOCK_DGRAM, &p1, &c1); if (err)
goto close_cli0;
err = add_to_sockmap(sock_mapfd, p0, p1);

if (err)
goto close_cli1;
n = write(c1, "a", 1);

if (n < 0)
FAIL_ERRNO("%s: write", log_prefix);
if (n == 0)
FAIL("%s: incomplete write", log_prefix);
if (n < 1)
goto close_cli1;
key = SK_PASS;

err = xbpf_map_lookup_elem(verd_mapfd, &key, &pass);

if (err)
goto close_cli1;
if (pass != 1)
FAIL("%s: want pass count 1, have %d", log_prefix, pass);
goto close;
n = recv_timeout(mode == REDIR_INGRESS ? p0 : c0, &b, 1, 0,
IO_TIMEOUT_SEC);
if (n < 0)
FAIL_ERRNO("%s: recv_timeout", log_prefix);
if (n == 0)
FAIL("%s: incomplete recv", log_prefix);
pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, verd_mapfd, mode); -close_cli1:

xclose(c1);

xclose(p1);

-close_cli0: +close: xclose(c0); xclose(p0);
Patch 1 is applied. Thanks.

In patch 2, the xclose() here is confusing after this change. It is also inconsistent from how other tests in sockmap_listen.c is doing it. c0/p0 and c1/p1 are opened here but only c0/p0 is closed here and c1/p1 is closed in the pairs_redir_to_connected() above instead.

I agree with Martin.

The refactoring idea itself is nice. But, I also find it surprising that newly extracted pairs_redir_to_connected helper takes ownership of cli1 & peer1 FDs.

For me, ideally, we would start using the __cleanup__ attribute, like we do now in the kernel code [1], to auto-close FDs on return. That would make error recovery and resource release less verbose.

[1] https://elixir.bootlin.com/linux/v6.5/source/include/linux/cleanup.h

patchwork-bot+netdevbpf＠kernel.org

6 Oct 6 Oct

5:10 a.m.

Hello:

This series was applied to bpf/bpf-next.git (master) by Martin KaFai Lau martin.lau@kernel.org:

On Thu, 5 Oct 2023 15:21:50 +0800 you wrote:

...

v2:

rename c0/c1 to cli0/cli1, p0/p1 to peer0/perr1 as Daniel suggested.

Two cleanups for sockmap_listen selftests: enable a kconfig and add a new helper.

Geliang Tang (2): selftests/bpf: Enable CONFIG_VSOCKETS in config selftests/bpf: Add pairs_redir_to_connected helper

[...]

Here is the summary with links: - [bpf-next,v2,1/2] selftests/bpf: Enable CONFIG_VSOCKETS in config https://git.kernel.org/bpf/bpf-next/c/d549854bc58f - [bpf-next,v2,2/2] selftests/bpf: Add pairs_redir_to_connected helper (no matching commit)

You are awesome, thank you!

-- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html

437

days inactive

445

days old

linux-kselftest-mirror@lists.linaro.org

5 comments

participants

tags (0)

participants (4)

Geliang Tang
Jakub Sitnicki
Martin KaFai Lau
patchwork-bot+netdevbpf＠kernel.org