From: Martin KaFai Lau <martin.lau@linux.dev>
To: Aditi Ghag <aditi.ghag@isovalent.com>
Cc: sdf@google.com, bpf@vger.kernel.org
Subject: Re: [PATCH v7 bpf-next 08/10] selftests/bpf: Test bpf_sock_destroy
Date: Thu, 4 May 2023 17:24:05 -0700 [thread overview]
Message-ID: <7a375393-ca58-6dd2-67eb-0eb77b430384@linux.dev> (raw)
In-Reply-To: <20230503225351.3700208-9-aditi.ghag@isovalent.com>
On 5/3/23 3:53 PM, Aditi Ghag wrote:
> The test cases for destroying sockets mirror the intended usages of the
> bpf_sock_destroy kfunc using iterators.
>
> The destroy helpers set `ECONNABORTED` error code that we can validate in
> the test code with client sockets. But UDP sockets have an overriding error
> code from the disconnect called during abort, so the error code the
> validation is only done for TCP sockets.
>
> Signed-off-by: Aditi Ghag <aditi.ghag@isovalent.com>
> ---
> .../selftests/bpf/prog_tests/sock_destroy.c | 215 ++++++++++++++++++
> .../selftests/bpf/progs/sock_destroy_prog.c | 145 ++++++++++++
> 2 files changed, 360 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/sock_destroy.c
> create mode 100644 tools/testing/selftests/bpf/progs/sock_destroy_prog.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/sock_destroy.c b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
> new file mode 100644
> index 000000000000..d5f76731b4a3
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
> @@ -0,0 +1,215 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <test_progs.h>
> +#include <bpf/bpf_endian.h>
> +
> +#include "sock_destroy_prog.skel.h"
> +#include "network_helpers.h"
> +
> +#define TEST_NS "sock_destroy_netns"
> +
> +static void start_iter_sockets(struct bpf_program *prog)
> +{
> + struct bpf_link *link;
> + char buf[50] = {};
> + int iter_fd, len;
> +
> + link = bpf_program__attach_iter(prog, NULL);
> + if (!ASSERT_OK_PTR(link, "attach_iter"))
> + return;
> +
> + iter_fd = bpf_iter_create(bpf_link__fd(link));
> + if (!ASSERT_GE(iter_fd, 0, "create_iter"))
> + goto free_link;
> +
> + while ((len = read(iter_fd, buf, sizeof(buf))) > 0)
> + ;
> + ASSERT_GE(len, 0, "read");
> +
> + close(iter_fd);
> +
> +free_link:
> + bpf_link__destroy(link);
> +}
> +
> +static void test_tcp_client(struct sock_destroy_prog *skel)
> +{
> + int serv = -1, clien = -1, n;
> +
> + serv = start_server(AF_INET6, SOCK_STREAM, NULL, 0, 0);
> + if (!ASSERT_GE(serv, 0, "start_server"))
> + goto cleanup;
> +
> + clien = connect_to_fd(serv, 0);
> + if (!ASSERT_GE(clien, 0, "connect_to_fd"))
> + goto cleanup;
> +
> + serv = accept(serv, NULL, NULL);
The original serv fd is over-written and lost.
> + if (!ASSERT_GE(serv, 0, "serv accept"))
> + goto cleanup;
> +
> + n = send(clien, "t", 1, 0);
> + if (!ASSERT_EQ(n, 1, "client send"))
> + goto cleanup;
> +
> + /* Run iterator program that destroys connected client sockets. */
> + start_iter_sockets(skel->progs.iter_tcp6_client);
> +
> + n = send(clien, "t", 1, 0);
> + if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
> + goto cleanup;
> + ASSERT_EQ(errno, ECONNABORTED, "error code on destroyed socket");
> +
> +cleanup:
> + if (clien != -1)
> + close(clien);
> + if (serv != -1)
> + close(serv);
> +}
> +
> +static void test_tcp_server(struct sock_destroy_prog *skel)
> +{
> + int serv = -1, clien = -1, n, serv_port;
> +
> + serv = start_server(AF_INET6, SOCK_STREAM, NULL, 0, 0);
> + if (!ASSERT_GE(serv, 0, "start_server"))
> + goto cleanup;
> + serv_port = get_socket_local_port(serv);
> + if (!ASSERT_GE(serv_port, 0, "get_sock_local_port"))
> + goto cleanup;
> + skel->bss->serv_port = (__be16) serv_port;
> +
> + clien = connect_to_fd(serv, 0);
> + if (!ASSERT_GE(clien, 0, "connect_to_fd"))
> + goto cleanup;
> +
> + serv = accept(serv, NULL, NULL);
Same here.
> + if (!ASSERT_GE(serv, 0, "serv accept"))
> + goto cleanup;
> +
> + n = send(clien, "t", 1, 0);
> + if (!ASSERT_EQ(n, 1, "client send"))
> + goto cleanup;
> +
> + /* Run iterator program that destroys server sockets. */
> + start_iter_sockets(skel->progs.iter_tcp6_server);
> +
> + n = send(clien, "t", 1, 0);
> + if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
> + goto cleanup;
> + ASSERT_EQ(errno, ECONNRESET, "error code on destroyed socket");
> +
> +cleanup:
> + if (clien != -1)
> + close(clien);
> + if (serv != -1)
> + close(serv);
> +}
> +
> +static void test_udp_client(struct sock_destroy_prog *skel)
> +{
> + int serv = -1, clien = -1, n = 0;
> +
> + serv = start_server(AF_INET6, SOCK_DGRAM, NULL, 0, 0);
> + if (!ASSERT_GE(serv, 0, "start_server"))
> + goto cleanup;
> +
> + clien = connect_to_fd(serv, 0);
> + if (!ASSERT_GE(clien, 0, "connect_to_fd"))
> + goto cleanup;
> +
> + n = send(clien, "t", 1, 0);
> + if (!ASSERT_EQ(n, 1, "client send"))
> + goto cleanup;
> +
> + /* Run iterator program that destroys sockets. */
> + start_iter_sockets(skel->progs.iter_udp6_client);
> +
> + n = send(clien, "t", 1, 0);
> + if (!ASSERT_LT(n, 0, "client_send on destroyed socket"))
> + goto cleanup;
> + /* UDP sockets have an overriding error code after they are disconnected,
> + * so we don't check for ECONNABORTED error code.
> + */
> +
> +cleanup:
> + if (clien != -1)
> + close(clien);
> + if (serv != -1)
> + close(serv);
> +}
> +
> +static void test_udp_server(struct sock_destroy_prog *skel)
> +{
> + int *listen_fds = NULL, n, i, serv_port;
> + unsigned int num_listens = 5;
> + char buf[1];
> +
> + /* Start reuseport servers. */
> + listen_fds = start_reuseport_server(AF_INET6, SOCK_DGRAM,
> + "::1", 0, 0, num_listens);
> + if (!ASSERT_OK_PTR(listen_fds, "start_reuseport_server"))
> + goto cleanup;
> + serv_port = get_socket_local_port(listen_fds[0]);
> + if (!ASSERT_GE(serv_port, 0, "get_sock_local_port"))
> + goto cleanup;
> + skel->bss->serv_port = (__be16) serv_port;
> +
> + /* Run iterator program that destroys server sockets. */
> + start_iter_sockets(skel->progs.iter_udp6_server);
> +
> + for (i = 0; i < num_listens; ++i) {
> + n = read(listen_fds[i], buf, sizeof(buf));
> + if (!ASSERT_EQ(n, -1, "read") ||
> + !ASSERT_EQ(errno, ECONNABORTED, "error code on destroyed socket"))
> + break;
> + }
> + ASSERT_EQ(i, num_listens, "server socket");
> +
> +cleanup:
> + free_fds(listen_fds, num_listens);
> +}
> +
> +void test_sock_destroy(void)
> +{
> + struct sock_destroy_prog *skel;
> + struct nstoken *nstoken;
This does need a '= NULL;' now after consolidating to one "cleanup" label. Even
the v6 patch needs a ' = NULL;' considering the SYS() below may do a goto also,
so I was incorrect on my v6 comment and stand to be corrected by the compiler:
https://github.com/kernel-patches/bpf/actions/runs/4877289517/jobs/8701781601
> + int cgroup_fd;
> +
> + skel = sock_destroy_prog__open_and_load();
> + if (!ASSERT_OK_PTR(skel, "skel_open"))
> + return;
> +
> + cgroup_fd = test__join_cgroup("/sock_destroy");
> + if (!ASSERT_GE(cgroup_fd, 0, "join_cgroup"))
> + goto cleanup;
> +
> + skel->links.sock_connect = bpf_program__attach_cgroup(
> + skel->progs.sock_connect, cgroup_fd);
> + if (!ASSERT_OK_PTR(skel->links.sock_connect, "prog_attach"))
> + goto cleanup;
> +
> + SYS(cleanup, "ip netns add %s", TEST_NS);
> + SYS(cleanup, "ip -net %s link set dev lo up", TEST_NS);
> +
> + nstoken = open_netns(TEST_NS);
> + if (!ASSERT_OK_PTR(nstoken, "open_netns"))
> + goto cleanup;
> +
> + if (test__start_subtest("tcp_client"))
> + test_tcp_client(skel);
> + if (test__start_subtest("tcp_server"))
> + test_tcp_server(skel);
> + if (test__start_subtest("udp_client"))
> + test_udp_client(skel);
> + if (test__start_subtest("udp_server"))
> + test_udp_server(skel);
> +
> +
> +cleanup:
> + if (nstoken)
> + close_netns(nstoken);
> + SYS_NOFAIL("ip netns del " TEST_NS " &> /dev/null");
> + if (cgroup_fd >= 0)
> + close(cgroup_fd);
> + sock_destroy_prog__destroy(skel);
> +}
next prev parent reply other threads:[~2023-05-05 0:24 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-03 22:53 [PATCH v7 bpf-next 00/10] bpf: Add socket destroy capability Aditi Ghag
2023-05-03 22:53 ` [PATCH v7 bpf-next 01/10] bpf: tcp: Avoid taking fast sock lock in iterator Aditi Ghag
2023-05-03 22:53 ` [PATCH v7 bpf-next 02/10] udp: seq_file: Helper function to match socket attributes Aditi Ghag
2023-05-03 22:53 ` [PATCH v7 bpf-next 03/10] bpf: udp: Encapsulate logic to get udp table Aditi Ghag
2023-05-03 22:53 ` [PATCH v7 bpf-next 04/10] udp: seq_file: Remove bpf_seq_afinfo from udp_iter_state Aditi Ghag
2023-05-04 1:15 ` kernel test robot
2023-05-04 1:25 ` Aditi Ghag
2023-05-04 10:37 ` kernel test robot
2023-05-03 22:53 ` [PATCH v7 bpf-next 05/10] bpf: udp: Implement batching for sockets iterator Aditi Ghag
2023-05-03 22:53 ` [PATCH v7 bpf-next 06/10] bpf: Add bpf_sock_destroy kfunc Aditi Ghag
2023-05-05 0:13 ` Martin KaFai Lau
2023-05-05 18:49 ` Martin KaFai Lau
2023-05-05 20:05 ` Alexei Starovoitov
2023-05-03 22:53 ` [PATCH v7 bpf-next 07/10] selftests/bpf: Add helper to get port using getsockname Aditi Ghag
2023-05-04 17:33 ` Stanislav Fomichev
2023-05-03 22:53 ` [PATCH v7 bpf-next 08/10] selftests/bpf: Test bpf_sock_destroy Aditi Ghag
2023-05-05 0:24 ` Martin KaFai Lau [this message]
2023-05-03 22:53 ` [PATCH v7 bpf-next 09/10] bpf: Add a kfunc filter function to 'struct btf_kfunc_id_set' Aditi Ghag
2023-05-05 0:28 ` Martin KaFai Lau
2023-05-03 22:53 ` [PATCH v7 bpf-next 10/10] selftests/bpf: Extend bpf_sock_destroy tests Aditi Ghag
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7a375393-ca58-6dd2-67eb-0eb77b430384@linux.dev \
--to=martin.lau@linux.dev \
--cc=aditi.ghag@isovalent.com \
--cc=bpf@vger.kernel.org \
--cc=sdf@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.