| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAH3MdRUSaqHmuu1cx5ckPvWYSDot8uwHCbgMRp26kKtkQ10N=w@mail.gmail.com>
Date: Mon, 1 Jul 2019 17:26:28 -0700
From: Y Song <ys114321@...il.com>
To: Stanislav Fomichev <sdf@...ichev.me>
Cc: Stanislav Fomichev <sdf@...gle.com>,
netdev <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
David Miller <davem@...emloft.net>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Eric Dumazet <edumazet@...gle.com>,
Priyaranjan Jha <priyarjha@...gle.com>,
Yuchung Cheng <ycheng@...gle.com>,
Soheil Hassas Yeganeh <soheil@...gle.com>
Subject: Re: [PATCH bpf-next 6/8] selftests/bpf: test BPF_SOCK_OPS_RTT_CB
On Mon, Jul 1, 2019 at 5:07 PM Stanislav Fomichev <sdf@...ichev.me> wrote:
>
> On 07/01, Y Song wrote:
> > On Mon, Jul 1, 2019 at 1:49 PM Stanislav Fomichev <sdf@...gle.com> wrote:
> > >
> > > Make sure the callback is invoked for syn-ack and data packet.
> > >
> > > Cc: Eric Dumazet <edumazet@...gle.com>
> > > Cc: Priyaranjan Jha <priyarjha@...gle.com>
> > > Cc: Yuchung Cheng <ycheng@...gle.com>
> > > Cc: Soheil Hassas Yeganeh <soheil@...gle.com>
> > > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> > > ---
> > > tools/testing/selftests/bpf/Makefile | 3 +-
> > > tools/testing/selftests/bpf/progs/tcp_rtt.c | 61 +++++
> > > tools/testing/selftests/bpf/test_tcp_rtt.c | 253 ++++++++++++++++++++
> > > 3 files changed, 316 insertions(+), 1 deletion(-)
> > > create mode 100644 tools/testing/selftests/bpf/progs/tcp_rtt.c
> > > create mode 100644 tools/testing/selftests/bpf/test_tcp_rtt.c
> > >
> > > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > > index de1754a8f5fe..2620406a53ec 100644
> > > --- a/tools/testing/selftests/bpf/Makefile
> > > +++ b/tools/testing/selftests/bpf/Makefile
> > > @@ -27,7 +27,7 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
> > > test_cgroup_storage test_select_reuseport test_section_names \
> > > test_netcnt test_tcpnotify_user test_sock_fields test_sysctl test_hashmap \
> > > test_btf_dump test_cgroup_attach xdping test_sockopt test_sockopt_sk \
> > > - test_sockopt_multi
> > > + test_sockopt_multi test_tcp_rtt
> > >
> > > BPF_OBJ_FILES = $(patsubst %.c,%.o, $(notdir $(wildcard progs/*.c)))
> > > TEST_GEN_FILES = $(BPF_OBJ_FILES)
> > > @@ -107,6 +107,7 @@ $(OUTPUT)/test_cgroup_attach: cgroup_helpers.c
> > > $(OUTPUT)/test_sockopt: cgroup_helpers.c
> > > $(OUTPUT)/test_sockopt_sk: cgroup_helpers.c
> > > $(OUTPUT)/test_sockopt_multi: cgroup_helpers.c
> > > +$(OUTPUT)/test_tcp_rtt: cgroup_helpers.c
> > >
> > > .PHONY: force
> > >
> > > diff --git a/tools/testing/selftests/bpf/progs/tcp_rtt.c b/tools/testing/selftests/bpf/progs/tcp_rtt.c
> > > new file mode 100644
> > > index 000000000000..233bdcb1659e
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/bpf/progs/tcp_rtt.c
> > > @@ -0,0 +1,61 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +#include <linux/bpf.h>
> > > +#include "bpf_helpers.h"
> > > +
> > > +char _license[] SEC("license") = "GPL";
> > > +__u32 _version SEC("version") = 1;
> > > +
> > > +struct tcp_rtt_storage {
> > > + __u32 invoked;
> > > + __u32 dsack_dups;
> > > + __u32 delivered;
> > > + __u32 delivered_ce;
> > > + __u32 icsk_retransmits;
> > > +};
[...]
> > > +
> > > +static void *server_thread(void *arg)
> > > +{
> > > + struct sockaddr_storage addr;
> > > + socklen_t len = sizeof(addr);
> > > + int fd = *(int *)arg;
> > > + int client_fd;
> > > +
> > > + if (listen(fd, 1) < 0)
> > > + error(1, errno, "Failed to listed on socket");
> >
> > The error() here only reports the error, right? In case of error,
> > should the control jumps to the end of this function and return?
> > The same for several error() calls below.
> No, error() calls exit(), so the whole process should die. Do you think
> it's better to gracefully handle that with pthread_join?
Thanks for explanation of error() semantics.
test_tcp_rtt is a standalone a program, so exiting
with a meaningful error message is fine to me. No need to change then.
>
> > > +
> > > + client_fd = accept(fd, (struct sockaddr *)&addr, &len);
> > > + if (client_fd < 0)
> > > + error(1, errno, "Failed to accept client");
> > > +
> > > + if (accept(fd, (struct sockaddr *)&addr, &len) >= 0)
> > > + error(1, errno, "Unexpected success in second accept");
> >
> > What is the purpose of this second default to-be-failed accept() call?
> So the server_thread waits here for the next client (that never arrives)
> and doesn't exit and call close(client_fd). I can add a comment here to
> clarify. Alternatively, I can just drop close(client_fd) and let
> the thread exit. WDYT?
Adding a comment to explain should be good enough. Thanks!
>
> > > +
> > > + close(client_fd);
> > > +
> > > + return NULL;
> > > +}
> > > +
> > > +int main(int args, char **argv)
> > > +{
> > > + int server_fd, cgroup_fd;
> > > + int err = EXIT_SUCCESS;
> > > + pthread_t tid;
> > > +
> > > + if (setup_cgroup_environment())
> > > + goto cleanup_obj;
> > > +
> > > + cgroup_fd = create_and_get_cgroup(CG_PATH);
> > > + if (cgroup_fd < 0)
> > > + goto cleanup_cgroup_env;
> > > +
> > > + if (join_cgroup(CG_PATH))
> > > + goto cleanup_cgroup;
> > > +
> > > + server_fd = start_server();
> > > + if (server_fd < 0) {
> > > + err = EXIT_FAILURE;
> > > + goto cleanup_cgroup;
> > > + }
> > > +
> > > + pthread_create(&tid, NULL, server_thread, (void *)&server_fd);
> > > +
> > > + if (run_test(cgroup_fd, server_fd))
> > > + err = EXIT_FAILURE;
> > > +
> > > + close(server_fd);
> > > +
> > > + printf("test_sockopt_sk: %s\n",
> > > + err == EXIT_SUCCESS ? "PASSED" : "FAILED");
> > > +
> > > +cleanup_cgroup:
> > > + close(cgroup_fd);
> > > +cleanup_cgroup_env:
> > > + cleanup_cgroup_environment();
> > > +cleanup_obj:
> > > + return err;
> > > +}
> > > --
> > > 2.22.0.410.gd8fdbe21b5-goog
> > >
Powered by blists - more mailing lists