netdev - Re: [PATCH bpf-next v4 1/4] selftests/bpf: generalize helpers to control background listener

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20200508071931.hmj6c6ux3kixzug6@kafai-mbp.dhcp.thefacebook.com>
Date:   Fri, 8 May 2020 00:19:31 -0700
From:   Martin KaFai Lau <kafai@...com>
To:     Stanislav Fomichev <sdf@...gle.com>
CC:     <netdev@...r.kernel.org>, <bpf@...r.kernel.org>,
        <davem@...emloft.net>, <ast@...nel.org>, <daniel@...earbox.net>,
        Andrey Ignatov <rdna@...com>,
        Jakub Sitnicki <jakub@...udflare.com>
Subject: Re: [PATCH bpf-next v4 1/4] selftests/bpf: generalize helpers to
 control background listener

On Thu, May 07, 2020 at 12:12:12PM -0700, Stanislav Fomichev wrote:
> Move the following routines that let us start a background listener
> thread and connect to a server by fd to the test_prog:
> * start_server - socket+bind+listen
> * connect_to_fd - connect to the server identified by fd
> 
> These will be used in the next commit.
> 
> Also, extend these helpers to support AF_INET6 and accept the family
> as an argument.
> 
> v4:
> * export extra helper to start server without a thread (Martin KaFai Lau)
> * tcp_rtt is no longer starting background thread (Martin KaFai Lau)
> 
> v2:
> * put helpers into network_helpers.c (Andrii Nakryiko)
> 
> Cc: Andrey Ignatov <rdna@...com>
> Cc: Martin KaFai Lau <kafai@...com>
> Cc: Jakub Sitnicki <jakub@...udflare.com>
> Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> ---
>  tools/testing/selftests/bpf/Makefile          |   2 +-
>  tools/testing/selftests/bpf/network_helpers.c |  86 +++++++++++++
>  tools/testing/selftests/bpf/network_helpers.h |  10 ++
>  .../selftests/bpf/prog_tests/tcp_rtt.c        | 116 +-----------------
>  4 files changed, 101 insertions(+), 113 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/network_helpers.c
>  create mode 100644 tools/testing/selftests/bpf/network_helpers.h
> 
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 3d942be23d09..8f25966b500b 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -354,7 +354,7 @@ endef
>  TRUNNER_TESTS_DIR := prog_tests
>  TRUNNER_BPF_PROGS_DIR := progs
>  TRUNNER_EXTRA_SOURCES := test_progs.c cgroup_helpers.c trace_helpers.c	\
> -			 flow_dissector_load.h
> +			 network_helpers.c flow_dissector_load.h
>  TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read				\
>  		       $(wildcard progs/btf_dump_test_case_*.c)
>  TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE
> diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
> new file mode 100644
> index 000000000000..8ea2b045452e
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/network_helpers.c
> @@ -0,0 +1,86 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <errno.h>
> +#include <pthread.h>
No longer needed.

> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <linux/err.h>
> +#include <linux/in.h>
> +#include <linux/in6.h>
> +
> +#include "network_helpers.h"
> +
> +#define clean_errno() (errno == 0 ? "None" : strerror(errno))
> +#define log_err(MSG, ...) fprintf(stderr, "(%s:%d: errno: %s) " MSG "\n", \
> +	__FILE__, __LINE__, clean_errno(), ##__VA_ARGS__)
> +
> +int start_server(int family, int type)
> +{
> +	struct sockaddr_storage addr = {};
> +	socklen_t len;
> +	int fd;
> +
> +	if (family == AF_INET) {
> +		struct sockaddr_in *sin = (void *)&addr;
> +
> +		sin->sin_family = AF_INET;
> +		len = sizeof(*sin);
> +	} else {
> +		struct sockaddr_in6 *sin6 = (void *)&addr;
> +
> +		sin6->sin6_family = AF_INET6;
> +		len = sizeof(*sin6);
> +	}
> +
> +	fd = socket(family, type | SOCK_NONBLOCK, 0);
> +	if (fd < 0) {
> +		log_err("Failed to create server socket");
> +		return -1;
> +	}
> +
> +	if (bind(fd, (const struct sockaddr *)&addr, len) < 0) {
> +		log_err("Failed to bind socket");
> +		close(fd);
> +		return -1;
> +	}
> +
> +	if (type == SOCK_STREAM) {
> +		if (listen(fd, 1) < 0) {
> +			log_err("Failed to listed on socket");
> +			close(fd);
> +			return -1;
> +		}
> +	}
> +
> +	return fd;
> +}
> +
> +int connect_to_fd(int family, int type, int server_fd)
> +{
> +	struct sockaddr_storage addr;
> +	socklen_t len = sizeof(addr);
> +	int fd;
> +
> +	fd = socket(family, type, 0);
> +	if (fd < 0) {
> +		log_err("Failed to create client socket");
> +		return -1;
> +	}
> +
Please also set a connect timeout.  Snippet from sk_assign.c:

const struct timeval timeo_sec = { .tv_sec = 3 };
const size_t timeo_optlen = sizeof(timeo_sec);

setsockopt(fd, SOL_SOCKET, SO_SNDTIMEO, &timeo_sec, timeo_optlen);

Other than that,

Acked-by: Martin KaFai Lau <kafai@...com>

> +	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 with family %d", family);
> +		goto out;
> +	}
> +
> +	return fd;
> +
> +out:
> +	close(fd);
> +	return -1;
> +}