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  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]
Date:   Wed, 1 Jul 2020 15:45:00 -0700
From:   Yonghong Song <yhs@...com>
To:     Martin KaFai Lau <kafai@...com>, <bpf@...r.kernel.org>
CC:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, <kernel-team@...com>,
        <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next 1/2] bpf: selftests: A few improvements to
 network_helpers.c



On 7/1/20 12:46 PM, Martin KaFai Lau wrote:
> This patch makes a few changes to the network_helpers.c
> 
> 1) Enforce SO_RCVTIMEO and SO_SNDTIMEO
>     This patch enforces timeout to the network fds through setsockopt
>     SO_RCVTIMEO and SO_SNDTIMEO.
> 
>     It will remove the need for SOCK_NONBLOCK that requires a more demanding
>     timeout logic with epoll/select, e.g. epoll_create, epoll_ctrl, and
>     then epoll_wait for timeout.
> 
>     That removes the need for connect_wait() from the
>     cgroup_skb_sk_lookup.c. The needed change is made in
>     cgroup_skb_sk_lookup.c.
> 
> 2) start_server():
>     Add optional addr_str and port to start_server().
>     That removes the need of the start_server_with_port().  The caller
>     can pass addr_str==NULL and/or port==0.
> 
>     "int timeout_ms" is also added to control the timeout
>     on the "accept(listen_fd)".
> 
> 3) connect_to_fd(): Fully use the server_fd.
>     The server sock address has already been obtained from
>     getsockname(server_fd).  The sockaddr includes the family,
>     so the "int family" arg is redundant.
> 
>     Since the server address is obtained from server_fd,  there
>     is little reason not to get the server's socket type from the
>     server_fd also.  getsockopt(server_fd) can be used to do that,
>     so "int type" arg is also removed.
> 
>     "int timeout_ms" is added.
> 
> 4) connect_fd_to_fd():
>     "int timeout_ms" is added.
>     Some code is also refactored to connect_fd_to_addr() which is
>     shared with connect_to_fd().
> 
> 5) Preserve errno:
>     Some callers need to check errno, e.g. cgroup_skb_sk_lookup.c.
>     Make changes to do it more consistently in save_errno_close()
>     and log_err().
> 
> Signed-off-by: Martin KaFai Lau <kafai@...com>

LGTM. Ack with a few nits below.
Acked-by: Yonghong Song <yhs@...com>

> ---
>   tools/testing/selftests/bpf/network_helpers.c | 157 +++++++++++-------
>   tools/testing/selftests/bpf/network_helpers.h |   9 +-
>   .../bpf/prog_tests/cgroup_skb_sk_lookup.c     |  12 +-
>   .../bpf/prog_tests/connect_force_port.c       |  10 +-
>   .../bpf/prog_tests/load_bytes_relative.c      |   4 +-
>   .../selftests/bpf/prog_tests/tcp_rtt.c        |   4 +-
>   6 files changed, 110 insertions(+), 86 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
> index e36dd1a1780d..1a371d3eca7d 100644
> --- a/tools/testing/selftests/bpf/network_helpers.c
> +++ b/tools/testing/selftests/bpf/network_helpers.c
> @@ -7,8 +7,6 @@
>   
>   #include <arpa/inet.h>
>   
> -#include <sys/epoll.h>
> -
>   #include <linux/err.h>
>   #include <linux/in.h>
>   #include <linux/in6.h>
> @@ -17,8 +15,13 @@
>   #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__)
> +#define log_err(MSG, ...) ({						\
> +			int save = errno;				\

Typicallty, the variables used inside macro started with __, e.g.,
__save, to avoie shadowing.

> +			fprintf(stderr, "(%s:%d: errno: %s) " MSG "\n", \
> +				__FILE__, __LINE__, clean_errno(),	\
> +				##__VA_ARGS__);				\
> +			errno = save;					\
> +})
>   
>   struct ipv4_packet pkt_v4 = {
>   	.eth.h_proto = __bpf_constant_htons(ETH_P_IP),
> @@ -37,7 +40,34 @@ struct ipv6_packet pkt_v6 = {
>   	.tcp.doff = 5,
>   };
>   
> -int start_server_with_port(int family, int type, __u16 port)
> +static int settimeo(int fd, int timeout_ms)
> +{
> +	struct timeval timeout = { .tv_sec = 3 };
> +
> +	if (timeout_ms > 0) {
> +		timeout.tv_sec = timeout_ms / 1000;
> +		timeout.tv_usec = (timeout_ms % 1000) * 1000;
> +	}
> +
> +	if (setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &timeout,
> +		       sizeof(timeout))) {
> +		log_err("Failed to set SO_RCVTIMEO");
> +		return -1;
> +	}
> +
> +	if (setsockopt(fd, SOL_SOCKET, SO_SNDTIMEO, &timeout,
> +		       sizeof(timeout))) {
> +		log_err("Failed to set SO_SNDTIMEO");
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +#define save_errno_close(fd) ({ int save = errno; close(fd); errno = save; })
> +
> +int start_server(int family, int type, const char *addr_str, __u16 port,
> +		 int timeout_ms)

Looks like non-NULL addr_str is not passed in all existing cased.
I guess this is for your later use case. It would be good to clarify
in the commit message.

>   {
>   	struct sockaddr_storage addr = {};
>   	socklen_t len;
> @@ -48,120 +78,119 @@ int start_server_with_port(int family, int type, __u16 port)
>   
>   		sin->sin_family = AF_INET;
>   		sin->sin_port = htons(port);
> +		if (addr_str &&
> +		    inet_pton(AF_INET, addr_str, &sin->sin_addr) != 1) {
> +			log_err("inet_pton(AF_INET, %s)", addr_str);
> +			return -1;
> +		}
>   		len = sizeof(*sin);
>   	} else {
>   		struct sockaddr_in6 *sin6 = (void *)&addr;
>   
>   		sin6->sin6_family = AF_INET6;
>   		sin6->sin6_port = htons(port);
> +		if (addr_str &&
> +		    inet_pton(AF_INET6, addr_str, &sin6->sin6_addr) != 1) {
> +			log_err("inet_pton(AF_INET6, %s)", addr_str);
> +			return -1;
> +		}
>   		len = sizeof(*sin6);
>   	}
>   
> -	fd = socket(family, type | SOCK_NONBLOCK, 0);
> +	fd = socket(family, type, 0);
>   	if (fd < 0) {
>   		log_err("Failed to create server socket");
>   		return -1;
>   	}
>   
> +	if (settimeo(fd, timeout_ms))
> +		goto error_close;
> +
>   	if (bind(fd, (const struct sockaddr *)&addr, len) < 0) {
>   		log_err("Failed to bind socket");
> -		close(fd);
> -		return -1;
> +		goto error_close;
>   	}
>   
>   	if (type == SOCK_STREAM) {
>   		if (listen(fd, 1) < 0) {
>   			log_err("Failed to listed on socket");
> -			close(fd);
> -			return -1;
> +			goto error_close;
>   		}
>   	}
>   
>   	return fd;
> -}
>   
> -int start_server(int family, int type)
> -{
> -	return start_server_with_port(family, type, 0);
> +error_close:
> +	save_errno_close(fd);
> +	return -1;
>   }
>   
[...]
> diff --git a/tools/testing/selftests/bpf/prog_tests/connect_force_port.c b/tools/testing/selftests/bpf/prog_tests/connect_force_port.c
> index 17bbf76812ca..9229db2f5ca5 100644
> --- a/tools/testing/selftests/bpf/prog_tests/connect_force_port.c
> +++ b/tools/testing/selftests/bpf/prog_tests/connect_force_port.c
> @@ -114,7 +114,7 @@ static int run_test(int cgroup_fd, int server_fd, int family, int type)
>   		goto close_bpf_object;
>   	}
>   
> -	fd = connect_to_fd(family, type, server_fd);
> +	fd = connect_to_fd(server_fd, 0);
>   	if (fd < 0) {
>   		err = -1;
>   		goto close_bpf_object;
> @@ -137,25 +137,25 @@ void test_connect_force_port(void)
>   	if (CHECK_FAIL(cgroup_fd < 0))
>   		return;
>   
> -	server_fd = start_server_with_port(AF_INET, SOCK_STREAM, 60123);
> +	server_fd = start_server(AF_INET, SOCK_STREAM, NULL, 60123, 0);
>   	if (CHECK_FAIL(server_fd < 0))
>   		goto close_cgroup_fd;
>   	CHECK_FAIL(run_test(cgroup_fd, server_fd, AF_INET, SOCK_STREAM));
>   	close(server_fd);
>   
> -	server_fd = start_server_with_port(AF_INET6, SOCK_STREAM, 60124);
> +	server_fd = start_server(AF_INET6, SOCK_STREAM, NULL, 60124, 0);
>   	if (CHECK_FAIL(server_fd < 0))
>   		goto close_cgroup_fd;
>   	CHECK_FAIL(run_test(cgroup_fd, server_fd, AF_INET6, SOCK_STREAM));
>   	close(server_fd);
>   
> -	server_fd = start_server_with_port(AF_INET, SOCK_DGRAM, 60123);
> +	server_fd = start_server(AF_INET, SOCK_DGRAM, NULL, 60123, 0);
>   	if (CHECK_FAIL(server_fd < 0))
>   		goto close_cgroup_fd;
>   	CHECK_FAIL(run_test(cgroup_fd, server_fd, AF_INET, SOCK_DGRAM));
>   	close(server_fd);
>   
> -	server_fd = start_server_with_port(AF_INET6, SOCK_DGRAM, 60124);
> +	server_fd = start_server(AF_INET6, SOCK_DGRAM, NULL, 60124, 0);
>   	if (CHECK_FAIL(server_fd < 0))
>   		goto close_cgroup_fd;
>   	CHECK_FAIL(run_test(cgroup_fd, server_fd, AF_INET6, SOCK_DGRAM));
> diff --git a/tools/testing/selftests/bpf/prog_tests/load_bytes_relative.c b/tools/testing/selftests/bpf/prog_tests/load_bytes_relative.c
> index c1168e4a9036..5a2a689dbb68 100644
> --- a/tools/testing/selftests/bpf/prog_tests/load_bytes_relative.c
> +++ b/tools/testing/selftests/bpf/prog_tests/load_bytes_relative.c
> @@ -23,7 +23,7 @@ void test_load_bytes_relative(void)
>   	if (CHECK_FAIL(cgroup_fd < 0))
>   		return;
>   
> -	server_fd = start_server(AF_INET, SOCK_STREAM);
> +	server_fd = start_server(AF_INET, SOCK_STREAM, NULL, 0, 0);
>   	if (CHECK_FAIL(server_fd < 0))
>   		goto close_cgroup_fd;
>   
> @@ -49,7 +49,7 @@ void test_load_bytes_relative(void)
>   	if (CHECK_FAIL(err))
>   		goto close_bpf_object;
>   
> -	client_fd = connect_to_fd(AF_INET, SOCK_STREAM, server_fd);
> +	client_fd = connect_to_fd(server_fd, 0);
>   	if (CHECK_FAIL(client_fd < 0))
>   		goto close_bpf_object;
>   	close(client_fd);
> diff --git a/tools/testing/selftests/bpf/prog_tests/tcp_rtt.c b/tools/testing/selftests/bpf/prog_tests/tcp_rtt.c
> index 9013a0c01eed..d207e968e6b1 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tcp_rtt.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tcp_rtt.c
> @@ -118,7 +118,7 @@ static int run_test(int cgroup_fd, int server_fd)
>   		goto close_bpf_object;
>   	}
>   
> -	client_fd = connect_to_fd(AF_INET, SOCK_STREAM, server_fd);
> +	client_fd = connect_to_fd(server_fd, 0);
>   	if (client_fd < 0) {
>   		err = -1;
>   		goto close_bpf_object;
> @@ -161,7 +161,7 @@ void test_tcp_rtt(void)
>   	if (CHECK_FAIL(cgroup_fd < 0))
>   		return;
>   
> -	server_fd = start_server(AF_INET, SOCK_STREAM);
> +	server_fd = start_server(AF_INET, SOCK_STREAM, NULL, 0, 0);
>   	if (CHECK_FAIL(server_fd < 0))
>   		goto close_cgroup_fd;
>   
> 

Powered by blists - more mailing lists