diff mbox series

[bpf-next,6/8] selftests/bpf: test BPF_SOCK_OPS_RTT_CB

Message ID 20190701204821.44230-7-sdf@google.com
State Changes Requested
Delegated to: BPF Maintainers
Headers show
Series bpf: TCP RTT sock_ops bpf callback | expand

Commit Message

Stanislav Fomichev July 1, 2019, 8:48 p.m. UTC 
Make sure the callback is invoked for syn-ack and data packet.

Cc: Eric Dumazet <edumazet@google.com>
Cc: Priyaranjan Jha <priyarjha@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
Signed-off-by: Stanislav Fomichev <sdf@google.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

Comments

Y Song July 1, 2019, 11:40 p.m. UTC | #1 
On Mon, Jul 1, 2019 at 1:49 PM Stanislav Fomichev <sdf@google.com> wrote:
>
> Make sure the callback is invoked for syn-ack and data packet.
>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Priyaranjan Jha <priyarjha@google.com>
> Cc: Yuchung Cheng <ycheng@google.com>
> Cc: Soheil Hassas Yeganeh <soheil@google.com>
> Signed-off-by: Stanislav Fomichev <sdf@google.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;
> +};
> +
> +struct bpf_map_def SEC("maps") socket_storage_map = {
> +       .type = BPF_MAP_TYPE_SK_STORAGE,
> +       .key_size = sizeof(int),
> +       .value_size = sizeof(struct tcp_rtt_storage),
> +       .map_flags = BPF_F_NO_PREALLOC,
> +};
> +BPF_ANNOTATE_KV_PAIR(socket_storage_map, int, struct tcp_rtt_storage);
> +
> +SEC("sockops")
> +int _sockops(struct bpf_sock_ops *ctx)
> +{
> +       struct tcp_rtt_storage *storage;
> +       struct bpf_tcp_sock *tcp_sk;
> +       int op = (int) ctx->op;
> +       struct bpf_sock *sk;
> +
> +       sk = ctx->sk;
> +       if (!sk)
> +               return 1;
> +
> +       storage = bpf_sk_storage_get(&socket_storage_map, sk, 0,
> +                                    BPF_SK_STORAGE_GET_F_CREATE);
> +       if (!storage)
> +               return 1;
> +
> +       if (op == BPF_SOCK_OPS_TCP_CONNECT_CB) {
> +               bpf_sock_ops_cb_flags_set(ctx, BPF_SOCK_OPS_RTT_CB_FLAG);
> +               return 1;
> +       }
> +
> +       if (op != BPF_SOCK_OPS_RTT_CB)
> +               return 1;
> +
> +       tcp_sk = bpf_tcp_sock(sk);
> +       if (!tcp_sk)
> +               return 1;
> +
> +       storage->invoked++;
> +
> +       storage->dsack_dups = tcp_sk->dsack_dups;
> +       storage->delivered = tcp_sk->delivered;
> +       storage->delivered_ce = tcp_sk->delivered_ce;
> +       storage->icsk_retransmits = tcp_sk->icsk_retransmits;
> +
> +       return 1;
> +}
> diff --git a/tools/testing/selftests/bpf/test_tcp_rtt.c b/tools/testing/selftests/bpf/test_tcp_rtt.c
> new file mode 100644
> index 000000000000..413fd8514adc
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/test_tcp_rtt.c
> @@ -0,0 +1,253 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <error.h>
> +#include <errno.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <sys/socket.h>
> +#include <netinet/in.h>
> +#include <pthread.h>
> +
> +#include <linux/filter.h>
> +#include <bpf/bpf.h>
> +#include <bpf/libbpf.h>
> +
> +#include "bpf_rlimit.h"
> +#include "bpf_util.h"
> +#include "cgroup_helpers.h"
> +
> +#define CG_PATH                                "/tcp_rtt"
> +
> +struct tcp_rtt_storage {
> +       __u32 invoked;
> +       __u32 dsack_dups;
> +       __u32 delivered;
> +       __u32 delivered_ce;
> +       __u32 icsk_retransmits;
> +};
> +
> +static void send_byte(int fd)
> +{
> +       char b = 0x55;
> +
> +       if (write(fd, &b, sizeof(b)) != 1)
> +               error(1, errno, "Failed to send single byte");
> +}
> +
> +static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 invoked,
> +                    __u32 dsack_dups, __u32 delivered, __u32 delivered_ce,
> +                    __u32 icsk_retransmits)
> +{
> +       int err = 0;
> +       struct tcp_rtt_storage val;
> +
> +       if (bpf_map_lookup_elem(map_fd, &client_fd, &val) < 0)
> +               error(1, errno, "Failed to read socket storage");
> +
> +       if (val.invoked != invoked) {
> +               log_err("%s: unexpected bpf_tcp_sock.invoked %d != %d",
> +                       msg, val.invoked, invoked);
> +               err++;
> +       }
> +
> +       if (val.dsack_dups != dsack_dups) {
> +               log_err("%s: unexpected bpf_tcp_sock.dsack_dups %d != %d",
> +                       msg, val.dsack_dups, dsack_dups);
> +               err++;
> +       }
> +
> +       if (val.delivered != delivered) {
> +               log_err("%s: unexpected bpf_tcp_sock.delivered %d != %d",
> +                       msg, val.delivered, delivered);
> +               err++;
> +       }
> +
> +       if (val.delivered_ce != delivered_ce) {
> +               log_err("%s: unexpected bpf_tcp_sock.delivered_ce %d != %d",
> +                       msg, val.delivered_ce, delivered_ce);
> +               err++;
> +       }
> +
> +       if (val.icsk_retransmits != icsk_retransmits) {
> +               log_err("%s: unexpected bpf_tcp_sock.icsk_retransmits %d != %d",
> +                       msg, val.icsk_retransmits, icsk_retransmits);
> +               err++;
> +       }
> +
> +       return err;
> +}
> +
> +static int connect_to_server(int server_fd)
> +{
> +       struct sockaddr_storage addr;
> +       socklen_t len = sizeof(addr);
> +       int fd;
> +
> +       fd = socket(AF_INET, SOCK_STREAM, 0);
> +       if (fd < 0) {
> +               log_err("Failed to create client socket");
> +               return -1;
> +       }
> +
> +       if (getsockname(server_fd, (struct sockaddr *)&addr, &len)) {
> +               log_err("Failed to get server addr");
> +               goto out;
> +       }
> +
> +       if (connect(fd, (const struct sockaddr *)&addr, len) < 0) {
> +               log_err("Fail to connect to server");
> +               goto out;
> +       }
> +
> +       return fd;
> +
> +out:
> +       close(fd);
> +       return -1;
> +}
> +
> +static int run_test(int cgroup_fd, int server_fd)
> +{
> +       struct bpf_prog_load_attr attr = {
> +               .prog_type = BPF_PROG_TYPE_SOCK_OPS,
> +               .file = "./tcp_rtt.o",
> +               .expected_attach_type = BPF_CGROUP_SOCK_OPS,
> +       };
> +       struct bpf_program *prog;
> +       struct bpf_object *obj;
> +       struct bpf_map *map;
> +       int client_fd;
> +       int ignored;
> +       int map_fd;
> +       int err;
> +
> +       err = bpf_prog_load_xattr(&attr, &obj, &ignored);
> +       if (err) {
> +               log_err("Failed to load BPF object");
> +               return -1;
> +       }

The third argument of bpf_prog_load_xattr is prog_fd.
If you have it, you do not need the below retrieving prog_fd
by bpf_program__fd(prog).

> +
> +       map = bpf_map__next(NULL, obj);
> +       map_fd = bpf_map__fd(map);
> +
> +       prog = bpf_program__next(NULL, obj);
> +       err = bpf_prog_attach(bpf_program__fd(prog), cgroup_fd,
> +                             BPF_CGROUP_SOCK_OPS, 0);
> +       if (err) {
> +               log_err("Failed to attach BPF program");
> +               goto close_bpf_object;
> +       }
> +
> +       client_fd = connect_to_server(server_fd);
> +       if (client_fd < 0) {
> +               err = -1;
> +               goto close_bpf_object;
> +       }
> +
> +       err += verify_sk(map_fd, client_fd, "syn-ack",
> +                        /*invoked=*/1,
> +                        /*dsack_dups=*/0,
> +                        /*delivered=*/1,
> +                        /*delivered_ce=*/0,
> +                        /*icsk_retransmits=*/0);
> +
> +       send_byte(client_fd);
> +
> +       err += verify_sk(map_fd, client_fd, "first payload byte",
> +                        /*invoked=*/2,
> +                        /*dsack_dups=*/0,
> +                        /*delivered=*/2,
> +                        /*delivered_ce=*/0,
> +                        /*icsk_retransmits=*/0);
> +
> +       close(client_fd);
> +
> +close_bpf_object:
> +       bpf_object__close(obj);
> +       return err;
> +}
> +
> +static int start_server(void)
> +{
> +       struct sockaddr_in addr = {
> +               .sin_family = AF_INET,
> +               .sin_addr.s_addr = htonl(INADDR_LOOPBACK),
> +       };
> +       int fd;
> +
> +       fd = socket(AF_INET, SOCK_STREAM, 0);
> +       if (fd < 0) {
> +               log_err("Failed to create server socket");
> +               return -1;
> +       }
> +
> +       if (bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) < 0) {
> +               log_err("Failed to bind socket");
> +               close(fd);
> +               return -1;
> +       }
> +
> +       return fd;
> +}
> +
> +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.

> +
> +       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?

> +
> +       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
>
Stanislav Fomichev July 2, 2019, 12:07 a.m. UTC | #2 
On 07/01, Y Song wrote:
> On Mon, Jul 1, 2019 at 1:49 PM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > Make sure the callback is invoked for syn-ack and data packet.
> >
> > Cc: Eric Dumazet <edumazet@google.com>
> > Cc: Priyaranjan Jha <priyarjha@google.com>
> > Cc: Yuchung Cheng <ycheng@google.com>
> > Cc: Soheil Hassas Yeganeh <soheil@google.com>
> > Signed-off-by: Stanislav Fomichev <sdf@google.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;
> > +};
> > +
> > +struct bpf_map_def SEC("maps") socket_storage_map = {
> > +       .type = BPF_MAP_TYPE_SK_STORAGE,
> > +       .key_size = sizeof(int),
> > +       .value_size = sizeof(struct tcp_rtt_storage),
> > +       .map_flags = BPF_F_NO_PREALLOC,
> > +};
> > +BPF_ANNOTATE_KV_PAIR(socket_storage_map, int, struct tcp_rtt_storage);
> > +
> > +SEC("sockops")
> > +int _sockops(struct bpf_sock_ops *ctx)
> > +{
> > +       struct tcp_rtt_storage *storage;
> > +       struct bpf_tcp_sock *tcp_sk;
> > +       int op = (int) ctx->op;
> > +       struct bpf_sock *sk;
> > +
> > +       sk = ctx->sk;
> > +       if (!sk)
> > +               return 1;
> > +
> > +       storage = bpf_sk_storage_get(&socket_storage_map, sk, 0,
> > +                                    BPF_SK_STORAGE_GET_F_CREATE);
> > +       if (!storage)
> > +               return 1;
> > +
> > +       if (op == BPF_SOCK_OPS_TCP_CONNECT_CB) {
> > +               bpf_sock_ops_cb_flags_set(ctx, BPF_SOCK_OPS_RTT_CB_FLAG);
> > +               return 1;
> > +       }
> > +
> > +       if (op != BPF_SOCK_OPS_RTT_CB)
> > +               return 1;
> > +
> > +       tcp_sk = bpf_tcp_sock(sk);
> > +       if (!tcp_sk)
> > +               return 1;
> > +
> > +       storage->invoked++;
> > +
> > +       storage->dsack_dups = tcp_sk->dsack_dups;
> > +       storage->delivered = tcp_sk->delivered;
> > +       storage->delivered_ce = tcp_sk->delivered_ce;
> > +       storage->icsk_retransmits = tcp_sk->icsk_retransmits;
> > +
> > +       return 1;
> > +}
> > diff --git a/tools/testing/selftests/bpf/test_tcp_rtt.c b/tools/testing/selftests/bpf/test_tcp_rtt.c
> > new file mode 100644
> > index 000000000000..413fd8514adc
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/test_tcp_rtt.c
> > @@ -0,0 +1,253 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <error.h>
> > +#include <errno.h>
> > +#include <stdio.h>
> > +#include <unistd.h>
> > +#include <sys/types.h>
> > +#include <sys/socket.h>
> > +#include <netinet/in.h>
> > +#include <pthread.h>
> > +
> > +#include <linux/filter.h>
> > +#include <bpf/bpf.h>
> > +#include <bpf/libbpf.h>
> > +
> > +#include "bpf_rlimit.h"
> > +#include "bpf_util.h"
> > +#include "cgroup_helpers.h"
> > +
> > +#define CG_PATH                                "/tcp_rtt"
> > +
> > +struct tcp_rtt_storage {
> > +       __u32 invoked;
> > +       __u32 dsack_dups;
> > +       __u32 delivered;
> > +       __u32 delivered_ce;
> > +       __u32 icsk_retransmits;
> > +};
> > +
> > +static void send_byte(int fd)
> > +{
> > +       char b = 0x55;
> > +
> > +       if (write(fd, &b, sizeof(b)) != 1)
> > +               error(1, errno, "Failed to send single byte");
> > +}
> > +
> > +static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 invoked,
> > +                    __u32 dsack_dups, __u32 delivered, __u32 delivered_ce,
> > +                    __u32 icsk_retransmits)
> > +{
> > +       int err = 0;
> > +       struct tcp_rtt_storage val;
> > +
> > +       if (bpf_map_lookup_elem(map_fd, &client_fd, &val) < 0)
> > +               error(1, errno, "Failed to read socket storage");
> > +
> > +       if (val.invoked != invoked) {
> > +               log_err("%s: unexpected bpf_tcp_sock.invoked %d != %d",
> > +                       msg, val.invoked, invoked);
> > +               err++;
> > +       }
> > +
> > +       if (val.dsack_dups != dsack_dups) {
> > +               log_err("%s: unexpected bpf_tcp_sock.dsack_dups %d != %d",
> > +                       msg, val.dsack_dups, dsack_dups);
> > +               err++;
> > +       }
> > +
> > +       if (val.delivered != delivered) {
> > +               log_err("%s: unexpected bpf_tcp_sock.delivered %d != %d",
> > +                       msg, val.delivered, delivered);
> > +               err++;
> > +       }
> > +
> > +       if (val.delivered_ce != delivered_ce) {
> > +               log_err("%s: unexpected bpf_tcp_sock.delivered_ce %d != %d",
> > +                       msg, val.delivered_ce, delivered_ce);
> > +               err++;
> > +       }
> > +
> > +       if (val.icsk_retransmits != icsk_retransmits) {
> > +               log_err("%s: unexpected bpf_tcp_sock.icsk_retransmits %d != %d",
> > +                       msg, val.icsk_retransmits, icsk_retransmits);
> > +               err++;
> > +       }
> > +
> > +       return err;
> > +}
> > +
> > +static int connect_to_server(int server_fd)
> > +{
> > +       struct sockaddr_storage addr;
> > +       socklen_t len = sizeof(addr);
> > +       int fd;
> > +
> > +       fd = socket(AF_INET, SOCK_STREAM, 0);
> > +       if (fd < 0) {
> > +               log_err("Failed to create client socket");
> > +               return -1;
> > +       }
> > +
> > +       if (getsockname(server_fd, (struct sockaddr *)&addr, &len)) {
> > +               log_err("Failed to get server addr");
> > +               goto out;
> > +       }
> > +
> > +       if (connect(fd, (const struct sockaddr *)&addr, len) < 0) {
> > +               log_err("Fail to connect to server");
> > +               goto out;
> > +       }
> > +
> > +       return fd;
> > +
> > +out:
> > +       close(fd);
> > +       return -1;
> > +}
> > +
> > +static int run_test(int cgroup_fd, int server_fd)
> > +{
> > +       struct bpf_prog_load_attr attr = {
> > +               .prog_type = BPF_PROG_TYPE_SOCK_OPS,
> > +               .file = "./tcp_rtt.o",
> > +               .expected_attach_type = BPF_CGROUP_SOCK_OPS,
> > +       };
> > +       struct bpf_program *prog;
> > +       struct bpf_object *obj;
> > +       struct bpf_map *map;
> > +       int client_fd;
> > +       int ignored;
> > +       int map_fd;
> > +       int err;
> > +
> > +       err = bpf_prog_load_xattr(&attr, &obj, &ignored);
> > +       if (err) {
> > +               log_err("Failed to load BPF object");
> > +               return -1;
> > +       }
> 
> The third argument of bpf_prog_load_xattr is prog_fd.
> If you have it, you do not need the below retrieving prog_fd
> by bpf_program__fd(prog).
Ack. I think I copy-pasted it from my other test where
I have multiple progs in the object file and attach them
manually by name. Will s/ignored/prog_fd/ in the v2.

> > +
> > +       map = bpf_map__next(NULL, obj);
> > +       map_fd = bpf_map__fd(map);
> > +
> > +       prog = bpf_program__next(NULL, obj);
> > +       err = bpf_prog_attach(bpf_program__fd(prog), cgroup_fd,
> > +                             BPF_CGROUP_SOCK_OPS, 0);
> > +       if (err) {
> > +               log_err("Failed to attach BPF program");
> > +               goto close_bpf_object;
> > +       }
> > +
> > +       client_fd = connect_to_server(server_fd);
> > +       if (client_fd < 0) {
> > +               err = -1;
> > +               goto close_bpf_object;
> > +       }
> > +
> > +       err += verify_sk(map_fd, client_fd, "syn-ack",
> > +                        /*invoked=*/1,
> > +                        /*dsack_dups=*/0,
> > +                        /*delivered=*/1,
> > +                        /*delivered_ce=*/0,
> > +                        /*icsk_retransmits=*/0);
> > +
> > +       send_byte(client_fd);
> > +
> > +       err += verify_sk(map_fd, client_fd, "first payload byte",
> > +                        /*invoked=*/2,
> > +                        /*dsack_dups=*/0,
> > +                        /*delivered=*/2,
> > +                        /*delivered_ce=*/0,
> > +                        /*icsk_retransmits=*/0);
> > +
> > +       close(client_fd);
> > +
> > +close_bpf_object:
> > +       bpf_object__close(obj);
> > +       return err;
> > +}
> > +
> > +static int start_server(void)
> > +{
> > +       struct sockaddr_in addr = {
> > +               .sin_family = AF_INET,
> > +               .sin_addr.s_addr = htonl(INADDR_LOOPBACK),
> > +       };
> > +       int fd;
> > +
> > +       fd = socket(AF_INET, SOCK_STREAM, 0);
> > +       if (fd < 0) {
> > +               log_err("Failed to create server socket");
> > +               return -1;
> > +       }
> > +
> > +       if (bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) < 0) {
> > +               log_err("Failed to bind socket");
> > +               close(fd);
> > +               return -1;
> > +       }
> > +
> > +       return fd;
> > +}
> > +
> > +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?

> > +
> > +       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?

> > +
> > +       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
> >
Y Song July 2, 2019, 12:26 a.m. UTC | #3 
On Mon, Jul 1, 2019 at 5:07 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> On 07/01, Y Song wrote:
> > On Mon, Jul 1, 2019 at 1:49 PM Stanislav Fomichev <sdf@google.com> wrote:
> > >
> > > Make sure the callback is invoked for syn-ack and data packet.
> > >
> > > Cc: Eric Dumazet <edumazet@google.com>
> > > Cc: Priyaranjan Jha <priyarjha@google.com>
> > > Cc: Yuchung Cheng <ycheng@google.com>
> > > Cc: Soheil Hassas Yeganeh <soheil@google.com>
> > > Signed-off-by: Stanislav Fomichev <sdf@google.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
> > >
diff mbox series

Patch

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;
+};
+
+struct bpf_map_def SEC("maps") socket_storage_map = {
+	.type = BPF_MAP_TYPE_SK_STORAGE,
+	.key_size = sizeof(int),
+	.value_size = sizeof(struct tcp_rtt_storage),
+	.map_flags = BPF_F_NO_PREALLOC,
+};
+BPF_ANNOTATE_KV_PAIR(socket_storage_map, int, struct tcp_rtt_storage);
+
+SEC("sockops")
+int _sockops(struct bpf_sock_ops *ctx)
+{
+	struct tcp_rtt_storage *storage;
+	struct bpf_tcp_sock *tcp_sk;
+	int op = (int) ctx->op;
+	struct bpf_sock *sk;
+
+	sk = ctx->sk;
+	if (!sk)
+		return 1;
+
+	storage = bpf_sk_storage_get(&socket_storage_map, sk, 0,
+				     BPF_SK_STORAGE_GET_F_CREATE);
+	if (!storage)
+		return 1;
+
+	if (op == BPF_SOCK_OPS_TCP_CONNECT_CB) {
+		bpf_sock_ops_cb_flags_set(ctx, BPF_SOCK_OPS_RTT_CB_FLAG);
+		return 1;
+	}
+
+	if (op != BPF_SOCK_OPS_RTT_CB)
+		return 1;
+
+	tcp_sk = bpf_tcp_sock(sk);
+	if (!tcp_sk)
+		return 1;
+
+	storage->invoked++;
+
+	storage->dsack_dups = tcp_sk->dsack_dups;
+	storage->delivered = tcp_sk->delivered;
+	storage->delivered_ce = tcp_sk->delivered_ce;
+	storage->icsk_retransmits = tcp_sk->icsk_retransmits;
+
+	return 1;
+}
diff --git a/tools/testing/selftests/bpf/test_tcp_rtt.c b/tools/testing/selftests/bpf/test_tcp_rtt.c
new file mode 100644
index 000000000000..413fd8514adc
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_tcp_rtt.c
@@ -0,0 +1,253 @@ 
+// SPDX-License-Identifier: GPL-2.0
+#include <error.h>
+#include <errno.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <netinet/in.h>
+#include <pthread.h>
+
+#include <linux/filter.h>
+#include <bpf/bpf.h>
+#include <bpf/libbpf.h>
+
+#include "bpf_rlimit.h"
+#include "bpf_util.h"
+#include "cgroup_helpers.h"
+
+#define CG_PATH                                "/tcp_rtt"
+
+struct tcp_rtt_storage {
+	__u32 invoked;
+	__u32 dsack_dups;
+	__u32 delivered;
+	__u32 delivered_ce;
+	__u32 icsk_retransmits;
+};
+
+static void send_byte(int fd)
+{
+	char b = 0x55;
+
+	if (write(fd, &b, sizeof(b)) != 1)
+		error(1, errno, "Failed to send single byte");
+}
+
+static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 invoked,
+		     __u32 dsack_dups, __u32 delivered, __u32 delivered_ce,
+		     __u32 icsk_retransmits)
+{
+	int err = 0;
+	struct tcp_rtt_storage val;
+
+	if (bpf_map_lookup_elem(map_fd, &client_fd, &val) < 0)
+		error(1, errno, "Failed to read socket storage");
+
+	if (val.invoked != invoked) {
+		log_err("%s: unexpected bpf_tcp_sock.invoked %d != %d",
+			msg, val.invoked, invoked);
+		err++;
+	}
+
+	if (val.dsack_dups != dsack_dups) {
+		log_err("%s: unexpected bpf_tcp_sock.dsack_dups %d != %d",
+			msg, val.dsack_dups, dsack_dups);
+		err++;
+	}
+
+	if (val.delivered != delivered) {
+		log_err("%s: unexpected bpf_tcp_sock.delivered %d != %d",
+			msg, val.delivered, delivered);
+		err++;
+	}
+
+	if (val.delivered_ce != delivered_ce) {
+		log_err("%s: unexpected bpf_tcp_sock.delivered_ce %d != %d",
+			msg, val.delivered_ce, delivered_ce);
+		err++;
+	}
+
+	if (val.icsk_retransmits != icsk_retransmits) {
+		log_err("%s: unexpected bpf_tcp_sock.icsk_retransmits %d != %d",
+			msg, val.icsk_retransmits, icsk_retransmits);
+		err++;
+	}
+
+	return err;
+}
+
+static int connect_to_server(int server_fd)
+{
+	struct sockaddr_storage addr;
+	socklen_t len = sizeof(addr);
+	int fd;
+
+	fd = socket(AF_INET, SOCK_STREAM, 0);
+	if (fd < 0) {
+		log_err("Failed to create client socket");
+		return -1;
+	}
+
+	if (getsockname(server_fd, (struct sockaddr *)&addr, &len)) {
+		log_err("Failed to get server addr");
+		goto out;
+	}
+
+	if (connect(fd, (const struct sockaddr *)&addr, len) < 0) {
+		log_err("Fail to connect to server");
+		goto out;
+	}
+
+	return fd;
+
+out:
+	close(fd);
+	return -1;
+}
+
+static int run_test(int cgroup_fd, int server_fd)
+{
+	struct bpf_prog_load_attr attr = {
+		.prog_type = BPF_PROG_TYPE_SOCK_OPS,
+		.file = "./tcp_rtt.o",
+		.expected_attach_type = BPF_CGROUP_SOCK_OPS,
+	};
+	struct bpf_program *prog;
+	struct bpf_object *obj;
+	struct bpf_map *map;
+	int client_fd;
+	int ignored;
+	int map_fd;
+	int err;
+
+	err = bpf_prog_load_xattr(&attr, &obj, &ignored);
+	if (err) {
+		log_err("Failed to load BPF object");
+		return -1;
+	}
+
+	map = bpf_map__next(NULL, obj);
+	map_fd = bpf_map__fd(map);
+
+	prog = bpf_program__next(NULL, obj);
+	err = bpf_prog_attach(bpf_program__fd(prog), cgroup_fd,
+			      BPF_CGROUP_SOCK_OPS, 0);
+	if (err) {
+		log_err("Failed to attach BPF program");
+		goto close_bpf_object;
+	}
+
+	client_fd = connect_to_server(server_fd);
+	if (client_fd < 0) {
+		err = -1;
+		goto close_bpf_object;
+	}
+
+	err += verify_sk(map_fd, client_fd, "syn-ack",
+			 /*invoked=*/1,
+			 /*dsack_dups=*/0,
+			 /*delivered=*/1,
+			 /*delivered_ce=*/0,
+			 /*icsk_retransmits=*/0);
+
+	send_byte(client_fd);
+
+	err += verify_sk(map_fd, client_fd, "first payload byte",
+			 /*invoked=*/2,
+			 /*dsack_dups=*/0,
+			 /*delivered=*/2,
+			 /*delivered_ce=*/0,
+			 /*icsk_retransmits=*/0);
+
+	close(client_fd);
+
+close_bpf_object:
+	bpf_object__close(obj);
+	return err;
+}
+
+static int start_server(void)
+{
+	struct sockaddr_in addr = {
+		.sin_family = AF_INET,
+		.sin_addr.s_addr = htonl(INADDR_LOOPBACK),
+	};
+	int fd;
+
+	fd = socket(AF_INET, SOCK_STREAM, 0);
+	if (fd < 0) {
+		log_err("Failed to create server socket");
+		return -1;
+	}
+
+	if (bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) < 0) {
+		log_err("Failed to bind socket");
+		close(fd);
+		return -1;
+	}
+
+	return fd;
+}
+
+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");
+
+	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");
+
+	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;
+}