[PATCH bpf-next v2 0/5] bpf: allow any port in bpf

netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH bpf-next v2 0/5] bpf: allow any port in bpf_bind helper
@ 2020-05-05 20:27 Stanislav Fomichev
  2020-05-05 20:27 ` [PATCH bpf-next v2 1/5] selftests/bpf: generalize helpers to control background listener Stanislav Fomichev
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Stanislav Fomichev @ 2020-05-05 20:27 UTC (permalink / raw)
  To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev, Andrey Ignatov

We want to have a tighter control on what ports we bind to in
the BPF_CGROUP_INET{4,6}_CONNECT hooks even if it means
connect() becomes slightly more expensive.

The series goes like this:
1. selftests: move existing helpers that make it easy to create
   listener threads into common test_progs part
2. selftests: make sure the helpers above don't get stuck forever
   in case the tests fails
3. do small refactoring of __inet{,6}_bind() flags to make it easy
   to extend them with the additional flags
4. remove the restriction on port being zero in bpf_bind() helper;
   add new bind flag to prevent POST_BIND hook from being called
5. selftests: move some common functionality into network_helpers

Cc: Andrey Ignatov <rdna@fb.com>

Stanislav Fomichev (5):
  selftests/bpf: generalize helpers to control background listener
  selftests/bpf: adopt accept_timeout from sockmap_listen
  net: refactor arguments of inet{,6}_bind
  bpf: allow any port in bpf_bind helper
  selftests/bpf: move existing common networking parts into
    network_helpers

 include/net/inet_common.h                     |   8 +-
 include/net/ipv6_stubs.h                      |   2 +-
 include/uapi/linux/bpf.h                      |   9 +-
 net/core/filter.c                             |  16 +-
 net/ipv4/af_inet.c                            |  20 +-
 net/ipv6/af_inet6.c                           |  22 +-
 tools/include/uapi/linux/bpf.h                |   9 +-
 tools/testing/selftests/bpf/Makefile          |   2 +-
 tools/testing/selftests/bpf/network_helpers.c | 208 ++++++++++++++++++
 tools/testing/selftests/bpf/network_helpers.h |  44 ++++
 .../bpf/prog_tests/connect_force_port.c       | 104 +++++++++
 .../selftests/bpf/prog_tests/fexit_bpf2bpf.c  |   1 +
 .../selftests/bpf/prog_tests/flow_dissector.c |   1 +
 .../prog_tests/flow_dissector_load_bytes.c    |   1 +
 .../selftests/bpf/prog_tests/global_data.c    |   1 +
 .../selftests/bpf/prog_tests/kfree_skb.c      |   1 +
 .../selftests/bpf/prog_tests/l4lb_all.c       |   1 +
 .../selftests/bpf/prog_tests/map_lock.c       |  14 ++
 .../selftests/bpf/prog_tests/pkt_access.c     |   1 +
 .../selftests/bpf/prog_tests/pkt_md_access.c  |   1 +
 .../selftests/bpf/prog_tests/prog_run_xattr.c |   1 +
 .../bpf/prog_tests/queue_stack_map.c          |   1 +
 .../selftests/bpf/prog_tests/signal_pending.c |   1 +
 .../selftests/bpf/prog_tests/skb_ctx.c        |   1 +
 .../selftests/bpf/prog_tests/sockmap_listen.c |  35 +--
 .../selftests/bpf/prog_tests/spinlock.c       |  14 ++
 .../selftests/bpf/prog_tests/tcp_rtt.c        | 116 +---------
 tools/testing/selftests/bpf/prog_tests/xdp.c  |   1 +
 .../bpf/prog_tests/xdp_adjust_tail.c          |   1 +
 .../selftests/bpf/prog_tests/xdp_bpf2bpf.c    |   1 +
 .../selftests/bpf/prog_tests/xdp_noinline.c   |   1 +
 .../selftests/bpf/progs/connect_force_port4.c |  28 +++
 .../selftests/bpf/progs/connect_force_port6.c |  28 +++
 tools/testing/selftests/bpf/test_progs.c      |  30 ---
 tools/testing/selftests/bpf/test_progs.h      |  23 --
 35 files changed, 510 insertions(+), 238 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/network_helpers.c
 create mode 100644 tools/testing/selftests/bpf/network_helpers.h
 create mode 100644 tools/testing/selftests/bpf/prog_tests/connect_force_port.c
 create mode 100644 tools/testing/selftests/bpf/progs/connect_force_port4.c
 create mode 100644 tools/testing/selftests/bpf/progs/connect_force_port6.c

-- 
2.26.2.526.g744177e7f7-goog

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH bpf-next v2 1/5] selftests/bpf: generalize helpers to control background listener
  2020-05-05 20:27 [PATCH bpf-next v2 0/5] bpf: allow any port in bpf_bind helper Stanislav Fomichev
@ 2020-05-05 20:27 ` Stanislav Fomichev
  2020-05-06  7:00   ` Martin KaFai Lau
  2020-05-05 20:27 ` [PATCH bpf-next v2 2/5] selftests/bpf: adopt accept_timeout from sockmap_listen Stanislav Fomichev
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Stanislav Fomichev @ 2020-05-05 20:27 UTC (permalink / raw)
  To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev, Andrey Ignatov

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_thread - start background INADDR_ANY thread
* stop_server_thread - stop the thread
* 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.

v2:
* put helpers into network_helpers.c (Andrii Nakryiko)

Cc: Andrey Ignatov <rdna@fb.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 tools/testing/selftests/bpf/Makefile          |   2 +-
 tools/testing/selftests/bpf/network_helpers.c | 164 ++++++++++++++++++
 tools/testing/selftests/bpf/network_helpers.h |  11 ++
 .../selftests/bpf/prog_tests/tcp_rtt.c        | 116 +------------
 4 files changed, 180 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..ee9386b033ed
--- /dev/null
+++ b/tools/testing/selftests/bpf/network_helpers.c
@@ -0,0 +1,164 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <errno.h>
+#include <pthread.h>
+#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 CHECK_FAIL(condition) ({					\
+	int __ret = !!(condition);					\
+	int __save_errno = errno;					\
+	if (__ret) {							\
+		fprintf(stdout, "%s:FAIL:%d\n", __func__, __LINE__);	\
+	}								\
+	errno = __save_errno;						\
+	__ret;								\
+})
+
+#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__)
+
+static int start_server(int family)
+{
+	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, SOCK_STREAM | 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;
+	}
+
+	return fd;
+}
+
+static pthread_mutex_t server_started_mtx = PTHREAD_MUTEX_INITIALIZER;
+static pthread_cond_t server_started = PTHREAD_COND_INITIALIZER;
+static volatile bool server_done;
+pthread_t server_tid;
+
+static void *server_thread(void *arg)
+{
+	struct sockaddr_storage addr;
+	socklen_t len = sizeof(addr);
+	int fd = *(int *)arg;
+	int client_fd;
+	int err;
+
+	err = listen(fd, 1);
+
+	pthread_mutex_lock(&server_started_mtx);
+	pthread_cond_signal(&server_started);
+	pthread_mutex_unlock(&server_started_mtx);
+
+	if (CHECK_FAIL(err < 0)) {
+		perror("Failed to listed on socket");
+		return ERR_PTR(err);
+	}
+
+	while (true) {
+		client_fd = accept(fd, (struct sockaddr *)&addr, &len);
+		if (client_fd == -1 && errno == EAGAIN) {
+			usleep(50);
+			continue;
+		}
+		break;
+	}
+	if (CHECK_FAIL(client_fd < 0)) {
+		perror("Failed to accept client");
+		return ERR_PTR(err);
+	}
+
+	while (!server_done)
+		usleep(50);
+
+	close(client_fd);
+
+	return NULL;
+}
+
+int start_server_thread(int family)
+{
+	int fd = start_server(family);
+
+	if (fd < 0)
+		return -1;
+
+	if (CHECK_FAIL(pthread_create(&server_tid, NULL, server_thread,
+				      (void *)&fd)))
+		goto err;
+
+	pthread_mutex_lock(&server_started_mtx);
+	pthread_cond_wait(&server_started, &server_started_mtx);
+	pthread_mutex_unlock(&server_started_mtx);
+
+	return fd;
+err:
+	close(fd);
+	return -1;
+}
+
+void stop_server_thread(int fd)
+{
+	void *server_res;
+
+	server_done = true;
+	CHECK_FAIL(pthread_join(server_tid, &server_res));
+	CHECK_FAIL(IS_ERR(server_res));
+	close(fd);
+}
+
+int connect_to_fd(int family, int server_fd)
+{
+	struct sockaddr_storage addr;
+	socklen_t len = sizeof(addr);
+	int fd;
+
+	fd = socket(family, 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 with family %d", family);
+		goto out;
+	}
+
+	return fd;
+
+out:
+	close(fd);
+	return -1;
+}
diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h
new file mode 100644
index 000000000000..1f3942160287
--- /dev/null
+++ b/tools/testing/selftests/bpf/network_helpers.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __NETWORK_HELPERS_H
+#define __NETWORK_HELPERS_H
+#include <sys/socket.h>
+#include <sys/types.h>
+
+int start_server_thread(int family);
+void stop_server_thread(int fd);
+int connect_to_fd(int family, int server_fd);
+
+#endif
diff --git a/tools/testing/selftests/bpf/prog_tests/tcp_rtt.c b/tools/testing/selftests/bpf/prog_tests/tcp_rtt.c
index e56b52ab41da..14bc0f3dc5c1 100644
--- a/tools/testing/selftests/bpf/prog_tests/tcp_rtt.c
+++ b/tools/testing/selftests/bpf/prog_tests/tcp_rtt.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
 #include "cgroup_helpers.h"
+#include "network_helpers.h"
 
 struct tcp_rtt_storage {
 	__u32 invoked;
@@ -87,34 +88,6 @@ static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 invoked,
 	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)
 {
@@ -145,7 +118,7 @@ static int run_test(int cgroup_fd, int server_fd)
 		goto close_bpf_object;
 	}
 
-	client_fd = connect_to_server(server_fd);
+	client_fd = connect_to_fd(AF_INET, server_fd);
 	if (client_fd < 0) {
 		err = -1;
 		goto close_bpf_object;
@@ -180,103 +153,22 @@ static int run_test(int cgroup_fd, int server_fd)
 	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 | SOCK_NONBLOCK, 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 pthread_mutex_t server_started_mtx = PTHREAD_MUTEX_INITIALIZER;
-static pthread_cond_t server_started = PTHREAD_COND_INITIALIZER;
-static volatile bool server_done = false;
-
-static void *server_thread(void *arg)
-{
-	struct sockaddr_storage addr;
-	socklen_t len = sizeof(addr);
-	int fd = *(int *)arg;
-	int client_fd;
-	int err;
-
-	err = listen(fd, 1);
-
-	pthread_mutex_lock(&server_started_mtx);
-	pthread_cond_signal(&server_started);
-	pthread_mutex_unlock(&server_started_mtx);
-
-	if (CHECK_FAIL(err < 0)) {
-		perror("Failed to listed on socket");
-		return ERR_PTR(err);
-	}
-
-	while (true) {
-		client_fd = accept(fd, (struct sockaddr *)&addr, &len);
-		if (client_fd == -1 && errno == EAGAIN) {
-			usleep(50);
-			continue;
-		}
-		break;
-	}
-	if (CHECK_FAIL(client_fd < 0)) {
-		perror("Failed to accept client");
-		return ERR_PTR(err);
-	}
-
-	while (!server_done)
-		usleep(50);
-
-	close(client_fd);
-
-	return NULL;
-}
-
 void test_tcp_rtt(void)
 {
 	int server_fd, cgroup_fd;
-	pthread_t tid;
-	void *server_res;
 
 	cgroup_fd = test__join_cgroup("/tcp_rtt");
 	if (CHECK_FAIL(cgroup_fd < 0))
 		return;
 
-	server_fd = start_server();
+	server_fd = start_server_thread(AF_INET);
 	if (CHECK_FAIL(server_fd < 0))
 		goto close_cgroup_fd;
 
-	if (CHECK_FAIL(pthread_create(&tid, NULL, server_thread,
-				      (void *)&server_fd)))
-		goto close_server_fd;
-
-	pthread_mutex_lock(&server_started_mtx);
-	pthread_cond_wait(&server_started, &server_started_mtx);
-	pthread_mutex_unlock(&server_started_mtx);
-
 	CHECK_FAIL(run_test(cgroup_fd, server_fd));
 
-	server_done = true;
-	CHECK_FAIL(pthread_join(tid, &server_res));
-	CHECK_FAIL(IS_ERR(server_res));
+	stop_server_thread(server_fd);
 
-close_server_fd:
-	close(server_fd);
 close_cgroup_fd:
 	close(cgroup_fd);
 }
-- 
2.26.2.526.g744177e7f7-goog


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf-next v2 1/5] selftests/bpf: generalize helpers to control background listener
  2020-05-05 20:27 ` [PATCH bpf-next v2 1/5] selftests/bpf: generalize helpers to control background listener Stanislav Fomichev
@ 2020-05-06  7:00   ` Martin KaFai Lau
  2020-05-06 16:28     ` sdf
  0 siblings, 1 reply; 14+ messages in thread
From: Martin KaFai Lau @ 2020-05-06  7:00 UTC (permalink / raw)
  To: Stanislav Fomichev; +Cc: netdev, bpf, davem, ast, daniel, Andrey Ignatov

On Tue, May 05, 2020 at 01:27:26PM -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_thread - start background INADDR_ANY thread
> * stop_server_thread - stop the thread
> * connect_to_fd - connect to the server identified by fd
> 
> These will be used in the next commit.
The refactoring itself looks fine.

If I read it correctly, it is a simple connect() test.
I am not sure a thread is even needed.  accept() is also unnecessary.
Can all be done in one thread?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf-next v2 1/5] selftests/bpf: generalize helpers to control background listener
  2020-05-06  7:00   ` Martin KaFai Lau
@ 2020-05-06 16:28     ` sdf
  2020-05-06 17:48       ` Martin KaFai Lau
  0 siblings, 1 reply; 14+ messages in thread
From: sdf @ 2020-05-06 16:28 UTC (permalink / raw)
  To: Martin KaFai Lau; +Cc: netdev, bpf, davem, ast, daniel, Andrey Ignatov

On 05/06, Martin KaFai Lau wrote:
> On Tue, May 05, 2020 at 01:27:26PM -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_thread - start background INADDR_ANY thread
> > * stop_server_thread - stop the thread
> > * connect_to_fd - connect to the server identified by fd
> >
> > These will be used in the next commit.
> The refactoring itself looks fine.

> If I read it correctly, it is a simple connect() test.
> I am not sure a thread is even needed.  accept() is also unnecessary.
> Can all be done in one thread?
I'm looking at the socket address after connection is established (to
verify that the port is the one we were supposed to be using), so
I fail to understand how accept() is unnecessary. Care to clarify?

I thought about doing a "listen() > non-blocking connect() > accept()"
in a single thread instead of background thread, but then decided that
it's better to reuse existing helpers and do proper connection instead
of writing all this new code.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf-next v2 1/5] selftests/bpf: generalize helpers to control background listener
  2020-05-06 16:28     ` sdf
@ 2020-05-06 17:48       ` Martin KaFai Lau
  2020-05-06 18:09         ` sdf
  0 siblings, 1 reply; 14+ messages in thread
From: Martin KaFai Lau @ 2020-05-06 17:48 UTC (permalink / raw)
  To: sdf; +Cc: netdev, bpf, davem, ast, daniel, Andrey Ignatov

On Wed, May 06, 2020 at 09:28:02AM -0700, sdf@google.com wrote:
> On 05/06, Martin KaFai Lau wrote:
> > On Tue, May 05, 2020 at 01:27:26PM -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_thread - start background INADDR_ANY thread
> > > * stop_server_thread - stop the thread
> > > * connect_to_fd - connect to the server identified by fd
> > >
> > > These will be used in the next commit.
> > The refactoring itself looks fine.
> 
> > If I read it correctly, it is a simple connect() test.
> > I am not sure a thread is even needed.  accept() is also unnecessary.
> > Can all be done in one thread?
> I'm looking at the socket address after connection is established (to
If I read it correctly, it is checking the local address (getsockname())
of the client's connect-ed() fd instead of the server's accept-ed() fd.

> verify that the port is the one we were supposed to be using), so
> I fail to understand how accept() is unnecessary. Care to clarify?
> 
> I thought about doing a "listen() > non-blocking connect() > accept()"
It should not need non-blocking connect().
The client connect() (3WHS) can still finish before the server side
accept() is called.  If the test does not need the accept-ed() fd,
then calling it or not is optional.

Just took a quick look, sk_assign.c and test_sock_addr.c could be
good examples.  They use SO_RCVTIMEO/SO_SNDTIMEO for timeout also.

> in a single thread instead of background thread, but then decided that
> it's better to reuse existing helpers and do proper connection instead
> of writing all this new code.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf-next v2 1/5] selftests/bpf: generalize helpers to control background listener
  2020-05-06 17:48       ` Martin KaFai Lau
@ 2020-05-06 18:09         ` sdf
  0 siblings, 0 replies; 14+ messages in thread
From: sdf @ 2020-05-06 18:09 UTC (permalink / raw)
  To: Martin KaFai Lau; +Cc: netdev, bpf, davem, ast, daniel, Andrey Ignatov

On 05/06, Martin KaFai Lau wrote:
> On Wed, May 06, 2020 at 09:28:02AM -0700, sdf@google.com wrote:
> > On 05/06, Martin KaFai Lau wrote:
> > > On Tue, May 05, 2020 at 01:27:26PM -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_thread - start background INADDR_ANY thread
> > > > * stop_server_thread - stop the thread
> > > > * connect_to_fd - connect to the server identified by fd
> > > >
> > > > These will be used in the next commit.
> > > The refactoring itself looks fine.
> >
> > > If I read it correctly, it is a simple connect() test.
> > > I am not sure a thread is even needed.  accept() is also unnecessary.
> > > Can all be done in one thread?
> > I'm looking at the socket address after connection is established (to
> If I read it correctly, it is checking the local address (getsockname())
> of the client's connect-ed() fd instead of the server's accept-ed() fd.

> > verify that the port is the one we were supposed to be using), so
> > I fail to understand how accept() is unnecessary. Care to clarify?
> >
> > I thought about doing a "listen() > non-blocking connect() > accept()"
> It should not need non-blocking connect().
> The client connect() (3WHS) can still finish before the server side
> accept() is called.  If the test does not need the accept-ed() fd,
> then calling it or not is optional.
Ah, I see what you're saying, in this case I can expose extra
helper from network_helpers to do only socket+bind part for
the server. Thanks for the explanation!

> Just took a quick look, sk_assign.c and test_sock_addr.c could be
> good examples.  They use SO_RCVTIMEO/SO_SNDTIMEO for timeout also.

> > in a single thread instead of background thread, but then decided that
> > it's better to reuse existing helpers and do proper connection instead
> > of writing all this new code.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH bpf-next v2 2/5] selftests/bpf: adopt accept_timeout from sockmap_listen
  2020-05-05 20:27 [PATCH bpf-next v2 0/5] bpf: allow any port in bpf_bind helper Stanislav Fomichev
  2020-05-05 20:27 ` [PATCH bpf-next v2 1/5] selftests/bpf: generalize helpers to control background listener Stanislav Fomichev
@ 2020-05-05 20:27 ` Stanislav Fomichev
  2020-05-05 20:27 ` [PATCH bpf-next v2 3/5] net: refactor arguments of inet{,6}_bind Stanislav Fomichev
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: Stanislav Fomichev @ 2020-05-05 20:27 UTC (permalink / raw)
  To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev, Andrey Ignatov

Move accept_timeout and recv_timeout to the common place so they
can be reused by the other tests. Switch to accept_timeout() in
test_progs instead of doing while loop around accept().

This prevents the tests that use start_server_thread/stop_server_thread
from being stuck when the error occurs.

Cc: Andrey Ignatov <rdna@fb.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 tools/testing/selftests/bpf/network_helpers.c | 43 +++++++++++++++----
 tools/testing/selftests/bpf/network_helpers.h |  4 ++
 .../selftests/bpf/prog_tests/sockmap_listen.c | 35 +--------------
 3 files changed, 40 insertions(+), 42 deletions(-)

diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
index ee9386b033ed..30ef0c7edebf 100644
--- a/tools/testing/selftests/bpf/network_helpers.c
+++ b/tools/testing/selftests/bpf/network_helpers.c
@@ -82,14 +82,7 @@ static void *server_thread(void *arg)
 		return ERR_PTR(err);
 	}
 
-	while (true) {
-		client_fd = accept(fd, (struct sockaddr *)&addr, &len);
-		if (client_fd == -1 && errno == EAGAIN) {
-			usleep(50);
-			continue;
-		}
-		break;
-	}
+	client_fd = accept_timeout(fd, (struct sockaddr *)&addr, &len, 3);
 	if (CHECK_FAIL(client_fd < 0)) {
 		perror("Failed to accept client");
 		return ERR_PTR(err);
@@ -162,3 +155,37 @@ int connect_to_fd(int family, int server_fd)
 	close(fd);
 	return -1;
 }
+
+static int poll_read(int fd, unsigned int timeout_sec)
+{
+	struct timeval timeout = { .tv_sec = timeout_sec };
+	fd_set rfds;
+	int r;
+
+	FD_ZERO(&rfds);
+	FD_SET(fd, &rfds);
+
+	r = select(fd + 1, &rfds, NULL, NULL, &timeout);
+	if (r == 0)
+		errno = ETIME;
+
+	return r == 1 ? 0 : -1;
+}
+
+int accept_timeout(int fd, struct sockaddr *addr, socklen_t *len,
+		   unsigned int timeout_sec)
+{
+	if (poll_read(fd, timeout_sec))
+		return -1;
+
+	return accept(fd, addr, len);
+}
+
+int recv_timeout(int fd, void *buf, size_t len, int flags,
+		 unsigned int timeout_sec)
+{
+	if (poll_read(fd, timeout_sec))
+		return -1;
+
+	return recv(fd, buf, len, flags);
+}
diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h
index 1f3942160287..74e2c40667a2 100644
--- a/tools/testing/selftests/bpf/network_helpers.h
+++ b/tools/testing/selftests/bpf/network_helpers.h
@@ -7,5 +7,9 @@
 int start_server_thread(int family);
 void stop_server_thread(int fd);
 int connect_to_fd(int family, int server_fd);
+int accept_timeout(int fd, struct sockaddr *addr, socklen_t *len,
+		   unsigned int timeout_sec);
+int recv_timeout(int fd, void *buf, size_t len, int flags,
+		 unsigned int timeout_sec);
 
 #endif
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index d7d65a700799..c2a78d8a110e 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -24,6 +24,7 @@
 
 #include "bpf_util.h"
 #include "test_progs.h"
+#include "network_helpers.h"
 #include "test_sockmap_listen.skel.h"
 
 #define IO_TIMEOUT_SEC 30
@@ -195,40 +196,6 @@
 		__ret;                                                         \
 	})
 
-static int poll_read(int fd, unsigned int timeout_sec)
-{
-	struct timeval timeout = { .tv_sec = timeout_sec };
-	fd_set rfds;
-	int r;
-
-	FD_ZERO(&rfds);
-	FD_SET(fd, &rfds);
-
-	r = select(fd + 1, &rfds, NULL, NULL, &timeout);
-	if (r == 0)
-		errno = ETIME;
-
-	return r == 1 ? 0 : -1;
-}
-
-static int accept_timeout(int fd, struct sockaddr *addr, socklen_t *len,
-			  unsigned int timeout_sec)
-{
-	if (poll_read(fd, timeout_sec))
-		return -1;
-
-	return accept(fd, addr, len);
-}
-
-static int recv_timeout(int fd, void *buf, size_t len, int flags,
-			unsigned int timeout_sec)
-{
-	if (poll_read(fd, timeout_sec))
-		return -1;
-
-	return recv(fd, buf, len, flags);
-}
-
 static void init_addr_loopback4(struct sockaddr_storage *ss, socklen_t *len)
 {
 	struct sockaddr_in *addr4 = memset(ss, 0, sizeof(*ss));
-- 
2.26.2.526.g744177e7f7-goog


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH bpf-next v2 3/5] net: refactor arguments of inet{,6}_bind
  2020-05-05 20:27 [PATCH bpf-next v2 0/5] bpf: allow any port in bpf_bind helper Stanislav Fomichev
  2020-05-05 20:27 ` [PATCH bpf-next v2 1/5] selftests/bpf: generalize helpers to control background listener Stanislav Fomichev
  2020-05-05 20:27 ` [PATCH bpf-next v2 2/5] selftests/bpf: adopt accept_timeout from sockmap_listen Stanislav Fomichev
@ 2020-05-05 20:27 ` Stanislav Fomichev
  2020-05-05 20:27 ` [PATCH bpf-next v2 4/5] bpf: allow any port in bpf_bind helper Stanislav Fomichev
  2020-05-05 20:27 ` [PATCH bpf-next v2 5/5] selftests/bpf: move existing common networking parts into network_helpers Stanislav Fomichev
  4 siblings, 0 replies; 14+ messages in thread
From: Stanislav Fomichev @ 2020-05-05 20:27 UTC (permalink / raw)
  To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev, Andrey Ignatov

The intent is to add an additional bind parameter in the next commit.
Instead of adding another argument, let's convert all existing
flag arguments into an extendable bit field.

No functional changes.

Cc: Andrey Ignatov <rdna@fb.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 include/net/inet_common.h |  6 +++++-
 include/net/ipv6_stubs.h  |  2 +-
 net/core/filter.c         |  6 ++++--
 net/ipv4/af_inet.c        | 10 +++++-----
 net/ipv6/af_inet6.c       | 10 +++++-----
 5 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/include/net/inet_common.h b/include/net/inet_common.h
index ae2ba897675c..c38f4f7d660a 100644
--- a/include/net/inet_common.h
+++ b/include/net/inet_common.h
@@ -35,8 +35,12 @@ int inet_shutdown(struct socket *sock, int how);
 int inet_listen(struct socket *sock, int backlog);
 void inet_sock_destruct(struct sock *sk);
 int inet_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len);
+/* Don't allocate port at this moment, defer to connect. */
+#define BIND_FORCE_ADDRESS_NO_PORT	(1 << 0)
+/* Grab and release socket lock. */
+#define BIND_WITH_LOCK			(1 << 1)
 int __inet_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len,
-		bool force_bind_address_no_port, bool with_lock);
+		u32 flags);
 int inet_getname(struct socket *sock, struct sockaddr *uaddr,
 		 int peer);
 int inet_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg);
diff --git a/include/net/ipv6_stubs.h b/include/net/ipv6_stubs.h
index a5f7c12c326a..6e622dd3122e 100644
--- a/include/net/ipv6_stubs.h
+++ b/include/net/ipv6_stubs.h
@@ -63,7 +63,7 @@ extern const struct ipv6_stub *ipv6_stub __read_mostly;
 /* A stub used by bpf helpers. Similarly ugly as ipv6_stub */
 struct ipv6_bpf_stub {
 	int (*inet6_bind)(struct sock *sk, struct sockaddr *uaddr, int addr_len,
-			  bool force_bind_address_no_port, bool with_lock);
+			  u32 flags);
 	struct sock *(*udp6_lib_lookup)(struct net *net,
 				     const struct in6_addr *saddr, __be16 sport,
 				     const struct in6_addr *daddr, __be16 dport,
diff --git a/net/core/filter.c b/net/core/filter.c
index dfaf5df13722..fa9ddab5dd1f 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4538,7 +4538,8 @@ BPF_CALL_3(bpf_bind, struct bpf_sock_addr_kern *, ctx, struct sockaddr *, addr,
 			return err;
 		if (((struct sockaddr_in *)addr)->sin_port != htons(0))
 			return err;
-		return __inet_bind(sk, addr, addr_len, true, false);
+		return __inet_bind(sk, addr, addr_len,
+				   BIND_FORCE_ADDRESS_NO_PORT);
 #if IS_ENABLED(CONFIG_IPV6)
 	} else if (addr->sa_family == AF_INET6) {
 		if (addr_len < SIN6_LEN_RFC2133)
@@ -4548,7 +4549,8 @@ BPF_CALL_3(bpf_bind, struct bpf_sock_addr_kern *, ctx, struct sockaddr *, addr,
 		/* ipv6_bpf_stub cannot be NULL, since it's called from
 		 * bpf_cgroup_inet6_connect hook and ipv6 is already loaded
 		 */
-		return ipv6_bpf_stub->inet6_bind(sk, addr, addr_len, true, false);
+		return ipv6_bpf_stub->inet6_bind(sk, addr, addr_len,
+						 BIND_FORCE_ADDRESS_NO_PORT);
 #endif /* CONFIG_IPV6 */
 	}
 #endif /* CONFIG_INET */
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 6177c4ba0037..68e74b1b0f26 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -450,12 +450,12 @@ int inet_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 	if (err)
 		return err;
 
-	return __inet_bind(sk, uaddr, addr_len, false, true);
+	return __inet_bind(sk, uaddr, addr_len, BIND_WITH_LOCK);
 }
 EXPORT_SYMBOL(inet_bind);
 
 int __inet_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len,
-		bool force_bind_address_no_port, bool with_lock)
+		u32 flags)
 {
 	struct sockaddr_in *addr = (struct sockaddr_in *)uaddr;
 	struct inet_sock *inet = inet_sk(sk);
@@ -506,7 +506,7 @@ int __inet_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len,
 	 *      would be illegal to use them (multicast/broadcast) in
 	 *      which case the sending device address is used.
 	 */
-	if (with_lock)
+	if (flags & BIND_WITH_LOCK)
 		lock_sock(sk);
 
 	/* Check these errors (active socket, double bind). */
@@ -520,7 +520,7 @@ int __inet_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len,
 
 	/* Make sure we are allowed to bind here. */
 	if (snum || !(inet->bind_address_no_port ||
-		      force_bind_address_no_port)) {
+		      (flags & BIND_FORCE_ADDRESS_NO_PORT))) {
 		if (sk->sk_prot->get_port(sk, snum)) {
 			inet->inet_saddr = inet->inet_rcv_saddr = 0;
 			err = -EADDRINUSE;
@@ -543,7 +543,7 @@ int __inet_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len,
 	sk_dst_reset(sk);
 	err = 0;
 out_release_sock:
-	if (with_lock)
+	if (flags & BIND_WITH_LOCK)
 		release_sock(sk);
 out:
 	return err;
diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index 345baa0a754f..552c2592b81c 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -273,7 +273,7 @@ static int inet6_create(struct net *net, struct socket *sock, int protocol,
 }
 
 static int __inet6_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len,
-			bool force_bind_address_no_port, bool with_lock)
+			u32 flags)
 {
 	struct sockaddr_in6 *addr = (struct sockaddr_in6 *)uaddr;
 	struct inet_sock *inet = inet_sk(sk);
@@ -297,7 +297,7 @@ static int __inet6_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len,
 	    !ns_capable(net->user_ns, CAP_NET_BIND_SERVICE))
 		return -EACCES;
 
-	if (with_lock)
+	if (flags & BIND_WITH_LOCK)
 		lock_sock(sk);
 
 	/* Check these errors (active socket, double bind). */
@@ -400,7 +400,7 @@ static int __inet6_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len,
 
 	/* Make sure we are allowed to bind here. */
 	if (snum || !(inet->bind_address_no_port ||
-		      force_bind_address_no_port)) {
+		      (flags & BIND_FORCE_ADDRESS_NO_PORT))) {
 		if (sk->sk_prot->get_port(sk, snum)) {
 			sk->sk_ipv6only = saved_ipv6only;
 			inet_reset_saddr(sk);
@@ -423,7 +423,7 @@ static int __inet6_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len,
 	inet->inet_dport = 0;
 	inet->inet_daddr = 0;
 out:
-	if (with_lock)
+	if (flags & BIND_WITH_LOCK)
 		release_sock(sk);
 	return err;
 out_unlock:
@@ -451,7 +451,7 @@ int inet6_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 	if (err)
 		return err;
 
-	return __inet6_bind(sk, uaddr, addr_len, false, true);
+	return __inet6_bind(sk, uaddr, addr_len, BIND_WITH_LOCK);
 }
 EXPORT_SYMBOL(inet6_bind);
 
-- 
2.26.2.526.g744177e7f7-goog


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH bpf-next v2 4/5] bpf: allow any port in bpf_bind helper
  2020-05-05 20:27 [PATCH bpf-next v2 0/5] bpf: allow any port in bpf_bind helper Stanislav Fomichev
                   ` (2 preceding siblings ...)
  2020-05-05 20:27 ` [PATCH bpf-next v2 3/5] net: refactor arguments of inet{,6}_bind Stanislav Fomichev
@ 2020-05-05 20:27 ` Stanislav Fomichev
  2020-05-05 20:33   ` Andrey Ignatov
  2020-05-06  6:23   ` Martin KaFai Lau
  2020-05-05 20:27 ` [PATCH bpf-next v2 5/5] selftests/bpf: move existing common networking parts into network_helpers Stanislav Fomichev
  4 siblings, 2 replies; 14+ messages in thread
From: Stanislav Fomichev @ 2020-05-05 20:27 UTC (permalink / raw)
  To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev, Andrey Ignatov

We want to have a tighter control on what ports we bind to in
the BPF_CGROUP_INET{4,6}_CONNECT hooks even if it means
connect() becomes slightly more expensive. The expensive part
comes from the fact that we now need to call inet_csk_get_port()
that verifies that the port is not used and allocates an entry
in the hash table for it.

Since we can't rely on "snum || !bind_address_no_port" to prevent
us from calling POST_BIND hook anymore, let's add another bind flag
to indicate that the call site is BPF program.

v2:
* Update documentation (Andrey Ignatov)
* Pass BIND_FORCE_ADDRESS_NO_PORT conditionally (Andrey Ignatov)

Cc: Andrey Ignatov <rdna@fb.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 include/net/inet_common.h                     |   2 +
 include/uapi/linux/bpf.h                      |   9 +-
 net/core/filter.c                             |  18 ++-
 net/ipv4/af_inet.c                            |  10 +-
 net/ipv6/af_inet6.c                           |  12 +-
 tools/include/uapi/linux/bpf.h                |   9 +-
 .../bpf/prog_tests/connect_force_port.c       | 104 ++++++++++++++++++
 .../selftests/bpf/progs/connect_force_port4.c |  28 +++++
 .../selftests/bpf/progs/connect_force_port6.c |  28 +++++
 9 files changed, 192 insertions(+), 28 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/connect_force_port.c
 create mode 100644 tools/testing/selftests/bpf/progs/connect_force_port4.c
 create mode 100644 tools/testing/selftests/bpf/progs/connect_force_port6.c

diff --git a/include/net/inet_common.h b/include/net/inet_common.h
index c38f4f7d660a..cb2818862919 100644
--- a/include/net/inet_common.h
+++ b/include/net/inet_common.h
@@ -39,6 +39,8 @@ int inet_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len);
 #define BIND_FORCE_ADDRESS_NO_PORT	(1 << 0)
 /* Grab and release socket lock. */
 #define BIND_WITH_LOCK			(1 << 1)
+/* Called from BPF program. */
+#define BIND_FROM_BPF			(1 << 2)
 int __inet_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len,
 		u32 flags);
 int inet_getname(struct socket *sock, struct sockaddr *uaddr,
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index b3643e27e264..14b5518a3d5b 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1994,10 +1994,11 @@ union bpf_attr {
  *
  * 		This helper works for IPv4 and IPv6, TCP and UDP sockets. The
  * 		domain (*addr*\ **->sa_family**) must be **AF_INET** (or
- * 		**AF_INET6**). Looking for a free port to bind to can be
- * 		expensive, therefore binding to port is not permitted by the
- * 		helper: *addr*\ **->sin_port** (or **sin6_port**, respectively)
- * 		must be set to zero.
+ * 		**AF_INET6**). It's advised to pass zero port (**sin_port**
+ * 		or **sin6_port**) which triggers IP_BIND_ADDRESS_NO_PORT-like
+ * 		behavior and lets the kernel reuse the same source port
+ * 		as long as 4-tuple is unique. Passing non-zero port might
+ * 		lead to degraded performance.
  * 	Return
  * 		0 on success, or a negative error in case of failure.
  *
diff --git a/net/core/filter.c b/net/core/filter.c
index fa9ddab5dd1f..da0634979f53 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4525,32 +4525,28 @@ BPF_CALL_3(bpf_bind, struct bpf_sock_addr_kern *, ctx, struct sockaddr *, addr,
 {
 #ifdef CONFIG_INET
 	struct sock *sk = ctx->sk;
+	u32 flags = BIND_FROM_BPF;
 	int err;
 
-	/* Binding to port can be expensive so it's prohibited in the helper.
-	 * Only binding to IP is supported.
-	 */
 	err = -EINVAL;
 	if (addr_len < offsetofend(struct sockaddr, sa_family))
 		return err;
 	if (addr->sa_family == AF_INET) {
 		if (addr_len < sizeof(struct sockaddr_in))
 			return err;
-		if (((struct sockaddr_in *)addr)->sin_port != htons(0))
-			return err;
-		return __inet_bind(sk, addr, addr_len,
-				   BIND_FORCE_ADDRESS_NO_PORT);
+		if (((struct sockaddr_in *)addr)->sin_port == htons(0))
+			flags |= BIND_FORCE_ADDRESS_NO_PORT;
+		return __inet_bind(sk, addr, addr_len, flags);
 #if IS_ENABLED(CONFIG_IPV6)
 	} else if (addr->sa_family == AF_INET6) {
 		if (addr_len < SIN6_LEN_RFC2133)
 			return err;
-		if (((struct sockaddr_in6 *)addr)->sin6_port != htons(0))
-			return err;
+		if (((struct sockaddr_in6 *)addr)->sin6_port == htons(0))
+			flags |= BIND_FORCE_ADDRESS_NO_PORT;
 		/* ipv6_bpf_stub cannot be NULL, since it's called from
 		 * bpf_cgroup_inet6_connect hook and ipv6 is already loaded
 		 */
-		return ipv6_bpf_stub->inet6_bind(sk, addr, addr_len,
-						 BIND_FORCE_ADDRESS_NO_PORT);
+		return ipv6_bpf_stub->inet6_bind(sk, addr, addr_len, flags);
 #endif /* CONFIG_IPV6 */
 	}
 #endif /* CONFIG_INET */
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 68e74b1b0f26..fcf0d12a407a 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -526,10 +526,12 @@ int __inet_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len,
 			err = -EADDRINUSE;
 			goto out_release_sock;
 		}
-		err = BPF_CGROUP_RUN_PROG_INET4_POST_BIND(sk);
-		if (err) {
-			inet->inet_saddr = inet->inet_rcv_saddr = 0;
-			goto out_release_sock;
+		if (!(flags & BIND_FROM_BPF)) {
+			err = BPF_CGROUP_RUN_PROG_INET4_POST_BIND(sk);
+			if (err) {
+				inet->inet_saddr = inet->inet_rcv_saddr = 0;
+				goto out_release_sock;
+			}
 		}
 	}
 
diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index 552c2592b81c..771a462a8322 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -407,11 +407,13 @@ static int __inet6_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len,
 			err = -EADDRINUSE;
 			goto out;
 		}
-		err = BPF_CGROUP_RUN_PROG_INET6_POST_BIND(sk);
-		if (err) {
-			sk->sk_ipv6only = saved_ipv6only;
-			inet_reset_saddr(sk);
-			goto out;
+		if (!(flags & BIND_FROM_BPF)) {
+			err = BPF_CGROUP_RUN_PROG_INET6_POST_BIND(sk);
+			if (err) {
+				sk->sk_ipv6only = saved_ipv6only;
+				inet_reset_saddr(sk);
+				goto out;
+			}
 		}
 	}
 
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index b3643e27e264..14b5518a3d5b 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1994,10 +1994,11 @@ union bpf_attr {
  *
  * 		This helper works for IPv4 and IPv6, TCP and UDP sockets. The
  * 		domain (*addr*\ **->sa_family**) must be **AF_INET** (or
- * 		**AF_INET6**). Looking for a free port to bind to can be
- * 		expensive, therefore binding to port is not permitted by the
- * 		helper: *addr*\ **->sin_port** (or **sin6_port**, respectively)
- * 		must be set to zero.
+ * 		**AF_INET6**). It's advised to pass zero port (**sin_port**
+ * 		or **sin6_port**) which triggers IP_BIND_ADDRESS_NO_PORT-like
+ * 		behavior and lets the kernel reuse the same source port
+ * 		as long as 4-tuple is unique. Passing non-zero port might
+ * 		lead to degraded performance.
  * 	Return
  * 		0 on success, or a negative error in case of failure.
  *
diff --git a/tools/testing/selftests/bpf/prog_tests/connect_force_port.c b/tools/testing/selftests/bpf/prog_tests/connect_force_port.c
new file mode 100644
index 000000000000..97104e6410b6
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/connect_force_port.c
@@ -0,0 +1,104 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <test_progs.h>
+#include "cgroup_helpers.h"
+#include "network_helpers.h"
+
+static int verify_port(int family, int fd, int expected)
+{
+	struct sockaddr_storage addr;
+	socklen_t len = sizeof(addr);
+	__u16 port;
+
+
+	if (getsockname(fd, (struct sockaddr *)&addr, &len)) {
+		log_err("Failed to get server addr");
+		return -1;
+	}
+
+	if (family == AF_INET)
+		port = ((struct sockaddr_in *)&addr)->sin_port;
+	else
+		port = ((struct sockaddr_in6 *)&addr)->sin6_port;
+
+	if (ntohs(port) != expected) {
+		log_err("Unexpected port %d, expected %d", ntohs(port),
+			expected);
+		return -1;
+	}
+
+	return 0;
+}
+
+static int run_test(int cgroup_fd, int server_fd, int family)
+{
+	struct bpf_prog_load_attr attr = {
+		.prog_type = BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
+	};
+	struct bpf_object *obj;
+	int expected_port;
+	int prog_fd;
+	int err;
+	int fd;
+
+	if (family == AF_INET) {
+		attr.file = "./connect_force_port4.o";
+		attr.expected_attach_type = BPF_CGROUP_INET4_CONNECT;
+		expected_port = 22222;
+	} else {
+		attr.file = "./connect_force_port6.o";
+		attr.expected_attach_type = BPF_CGROUP_INET6_CONNECT;
+		expected_port = 22223;
+	}
+
+	err = bpf_prog_load_xattr(&attr, &obj, &prog_fd);
+	if (err) {
+		log_err("Failed to load BPF object");
+		return -1;
+	}
+
+	err = bpf_prog_attach(prog_fd, cgroup_fd, attr.expected_attach_type,
+			      0);
+	if (err) {
+		log_err("Failed to attach BPF program");
+		goto close_bpf_object;
+	}
+
+	fd = connect_to_fd(family, server_fd);
+	if (fd < 0) {
+		err = -1;
+		goto close_bpf_object;
+	}
+
+	err = verify_port(family, fd, expected_port);
+
+	close(fd);
+
+close_bpf_object:
+	bpf_object__close(obj);
+	return err;
+}
+
+void test_connect_force_port(void)
+{
+	int server_fd, cgroup_fd;
+
+	cgroup_fd = test__join_cgroup("/connect_force_port");
+	if (CHECK_FAIL(cgroup_fd < 0))
+		return;
+
+	server_fd = start_server_thread(AF_INET);
+	if (CHECK_FAIL(server_fd < 0))
+		goto close_cgroup_fd;
+	CHECK_FAIL(run_test(cgroup_fd, server_fd, AF_INET));
+	stop_server_thread(server_fd);
+
+	server_fd = start_server_thread(AF_INET6);
+	if (CHECK_FAIL(server_fd < 0))
+		goto close_cgroup_fd;
+	CHECK_FAIL(run_test(cgroup_fd, server_fd, AF_INET6));
+	stop_server_thread(server_fd);
+
+close_cgroup_fd:
+	close(cgroup_fd);
+}
diff --git a/tools/testing/selftests/bpf/progs/connect_force_port4.c b/tools/testing/selftests/bpf/progs/connect_force_port4.c
new file mode 100644
index 000000000000..1b8eb34b2db0
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/connect_force_port4.c
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <string.h>
+
+#include <linux/bpf.h>
+#include <linux/in.h>
+#include <linux/in6.h>
+#include <sys/socket.h>
+
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
+
+char _license[] SEC("license") = "GPL";
+int _version SEC("version") = 1;
+
+SEC("cgroup/connect4")
+int _connect4(struct bpf_sock_addr *ctx)
+{
+	struct sockaddr_in sa = {};
+
+	sa.sin_family = AF_INET;
+	sa.sin_port = bpf_htons(22222);
+	sa.sin_addr.s_addr = bpf_htonl(0x7f000001); /* 127.0.0.1 */
+
+	if (bpf_bind(ctx, (struct sockaddr *)&sa, sizeof(sa)) != 0)
+		return 0;
+
+	return 1;
+}
diff --git a/tools/testing/selftests/bpf/progs/connect_force_port6.c b/tools/testing/selftests/bpf/progs/connect_force_port6.c
new file mode 100644
index 000000000000..8cd1a9e81f64
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/connect_force_port6.c
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <string.h>
+
+#include <linux/bpf.h>
+#include <linux/in.h>
+#include <linux/in6.h>
+#include <sys/socket.h>
+
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
+
+char _license[] SEC("license") = "GPL";
+int _version SEC("version") = 1;
+
+SEC("cgroup/connect6")
+int _connect6(struct bpf_sock_addr *ctx)
+{
+	struct sockaddr_in6 sa = {};
+
+	sa.sin6_family = AF_INET;
+	sa.sin6_port = bpf_htons(22223);
+	sa.sin6_addr.s6_addr32[3] = bpf_htonl(1); /* ::1 */
+
+	if (bpf_bind(ctx, (struct sockaddr *)&sa, sizeof(sa)) != 0)
+		return 0;
+
+	return 1;
+}
-- 
2.26.2.526.g744177e7f7-goog


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf-next v2 4/5] bpf: allow any port in bpf_bind helper
  2020-05-05 20:27 ` [PATCH bpf-next v2 4/5] bpf: allow any port in bpf_bind helper Stanislav Fomichev
@ 2020-05-05 20:33   ` Andrey Ignatov
  2020-05-06  6:23   ` Martin KaFai Lau
  1 sibling, 0 replies; 14+ messages in thread
From: Andrey Ignatov @ 2020-05-05 20:33 UTC (permalink / raw)
  To: Stanislav Fomichev; +Cc: netdev, bpf, davem, ast, daniel

Stanislav Fomichev <sdf@google.com> [Tue, 2020-05-05 13:27 -0700]:
> We want to have a tighter control on what ports we bind to in
> the BPF_CGROUP_INET{4,6}_CONNECT hooks even if it means
> connect() becomes slightly more expensive. The expensive part
> comes from the fact that we now need to call inet_csk_get_port()
> that verifies that the port is not used and allocates an entry
> in the hash table for it.
> 
> Since we can't rely on "snum || !bind_address_no_port" to prevent
> us from calling POST_BIND hook anymore, let's add another bind flag
> to indicate that the call site is BPF program.
> 
> v2:
> * Update documentation (Andrey Ignatov)
> * Pass BIND_FORCE_ADDRESS_NO_PORT conditionally (Andrey Ignatov)
> 
> Cc: Andrey Ignatov <rdna@fb.com>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>

Thanks. LGMT. 

Acked-by: Andrey Ignatov <rdna@fb.com>

-- 
Andrey Ignatov

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf-next v2 4/5] bpf: allow any port in bpf_bind helper
  2020-05-05 20:27 ` [PATCH bpf-next v2 4/5] bpf: allow any port in bpf_bind helper Stanislav Fomichev
  2020-05-05 20:33   ` Andrey Ignatov
@ 2020-05-06  6:23   ` Martin KaFai Lau
  2020-05-06 16:22     ` sdf
  1 sibling, 1 reply; 14+ messages in thread
From: Martin KaFai Lau @ 2020-05-06  6:23 UTC (permalink / raw)
  To: Stanislav Fomichev; +Cc: netdev, bpf, davem, ast, daniel, Andrey Ignatov

On Tue, May 05, 2020 at 01:27:29PM -0700, Stanislav Fomichev wrote:
> We want to have a tighter control on what ports we bind to in
> the BPF_CGROUP_INET{4,6}_CONNECT hooks even if it means
> connect() becomes slightly more expensive. The expensive part
> comes from the fact that we now need to call inet_csk_get_port()
> that verifies that the port is not used and allocates an entry
> in the hash table for it.
> 
> Since we can't rely on "snum || !bind_address_no_port" to prevent
> us from calling POST_BIND hook anymore, let's add another bind flag
> to indicate that the call site is BPF program.
> 
> v2:
> * Update documentation (Andrey Ignatov)
> * Pass BIND_FORCE_ADDRESS_NO_PORT conditionally (Andrey Ignatov)
> 
> Cc: Andrey Ignatov <rdna@fb.com>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
>  include/net/inet_common.h                     |   2 +
>  include/uapi/linux/bpf.h                      |   9 +-
>  net/core/filter.c                             |  18 ++-
>  net/ipv4/af_inet.c                            |  10 +-
>  net/ipv6/af_inet6.c                           |  12 +-
>  tools/include/uapi/linux/bpf.h                |   9 +-
>  .../bpf/prog_tests/connect_force_port.c       | 104 ++++++++++++++++++
>  .../selftests/bpf/progs/connect_force_port4.c |  28 +++++
>  .../selftests/bpf/progs/connect_force_port6.c |  28 +++++
>  9 files changed, 192 insertions(+), 28 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/connect_force_port.c
>  create mode 100644 tools/testing/selftests/bpf/progs/connect_force_port4.c
>  create mode 100644 tools/testing/selftests/bpf/progs/connect_force_port6.c
> 
> diff --git a/include/net/inet_common.h b/include/net/inet_common.h
> index c38f4f7d660a..cb2818862919 100644
> --- a/include/net/inet_common.h
> +++ b/include/net/inet_common.h
> @@ -39,6 +39,8 @@ int inet_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len);
>  #define BIND_FORCE_ADDRESS_NO_PORT	(1 << 0)
>  /* Grab and release socket lock. */
>  #define BIND_WITH_LOCK			(1 << 1)
> +/* Called from BPF program. */
> +#define BIND_FROM_BPF			(1 << 2)
>  int __inet_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len,
>  		u32 flags);
>  int inet_getname(struct socket *sock, struct sockaddr *uaddr,
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index b3643e27e264..14b5518a3d5b 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1994,10 +1994,11 @@ union bpf_attr {
>   *
>   * 		This helper works for IPv4 and IPv6, TCP and UDP sockets. The
>   * 		domain (*addr*\ **->sa_family**) must be **AF_INET** (or
> - * 		**AF_INET6**). Looking for a free port to bind to can be
> - * 		expensive, therefore binding to port is not permitted by the
> - * 		helper: *addr*\ **->sin_port** (or **sin6_port**, respectively)
> - * 		must be set to zero.
> + * 		**AF_INET6**). It's advised to pass zero port (**sin_port**
> + * 		or **sin6_port**) which triggers IP_BIND_ADDRESS_NO_PORT-like
> + * 		behavior and lets the kernel reuse the same source port
Reading "zero port" and "the same source port" together is confusing.

> + * 		as long as 4-tuple is unique. Passing non-zero port might
> + * 		lead to degraded performance.
Is the "degraded performance" also true for UDP?

>   * 	Return
>   * 		0 on success, or a negative error in case of failure.
>   *

[ ... ]

> diff --git a/tools/testing/selftests/bpf/prog_tests/connect_force_port.c b/tools/testing/selftests/bpf/prog_tests/connect_force_port.c
> new file mode 100644
> index 000000000000..97104e6410b6
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/connect_force_port.c
> @@ -0,0 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <test_progs.h>
> +#include "cgroup_helpers.h"
> +#include "network_helpers.h"
> +
> +static int verify_port(int family, int fd, int expected)
> +{
> +	struct sockaddr_storage addr;
> +	socklen_t len = sizeof(addr);
> +	__u16 port;
> +
> +
> +	if (getsockname(fd, (struct sockaddr *)&addr, &len)) {
> +		log_err("Failed to get server addr");
> +		return -1;
> +	}
> +
> +	if (family == AF_INET)
> +		port = ((struct sockaddr_in *)&addr)->sin_port;
> +	else
> +		port = ((struct sockaddr_in6 *)&addr)->sin6_port;
> +
> +	if (ntohs(port) != expected) {
> +		log_err("Unexpected port %d, expected %d", ntohs(port),
> +			expected);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int run_test(int cgroup_fd, int server_fd, int family)
> +{
> +	struct bpf_prog_load_attr attr = {
> +		.prog_type = BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
> +	};
> +	struct bpf_object *obj;
> +	int expected_port;
> +	int prog_fd;
> +	int err;
> +	int fd;
> +
> +	if (family == AF_INET) {
> +		attr.file = "./connect_force_port4.o";
> +		attr.expected_attach_type = BPF_CGROUP_INET4_CONNECT;
> +		expected_port = 22222;
> +	} else {
> +		attr.file = "./connect_force_port6.o";
> +		attr.expected_attach_type = BPF_CGROUP_INET6_CONNECT;
> +		expected_port = 22223;
> +	}
> +
> +	err = bpf_prog_load_xattr(&attr, &obj, &prog_fd);
> +	if (err) {
> +		log_err("Failed to load BPF object");
> +		return -1;
> +	}
> +
> +	err = bpf_prog_attach(prog_fd, cgroup_fd, attr.expected_attach_type,
> +			      0);
> +	if (err) {
> +		log_err("Failed to attach BPF program");
> +		goto close_bpf_object;
> +	}
> +
> +	fd = connect_to_fd(family, server_fd);
> +	if (fd < 0) {
> +		err = -1;
> +		goto close_bpf_object;
> +	}
> +
> +	err = verify_port(family, fd, expected_port);
> +
> +	close(fd);
> +
> +close_bpf_object:
> +	bpf_object__close(obj);
> +	return err;
> +}
> +
> +void test_connect_force_port(void)
> +{
> +	int server_fd, cgroup_fd;
> +
> +	cgroup_fd = test__join_cgroup("/connect_force_port");
> +	if (CHECK_FAIL(cgroup_fd < 0))
> +		return;
> +
> +	server_fd = start_server_thread(AF_INET);
> +	if (CHECK_FAIL(server_fd < 0))
> +		goto close_cgroup_fd;
> +	CHECK_FAIL(run_test(cgroup_fd, server_fd, AF_INET));
> +	stop_server_thread(server_fd);
> +
> +	server_fd = start_server_thread(AF_INET6);
> +	if (CHECK_FAIL(server_fd < 0))
> +		goto close_cgroup_fd;
> +	CHECK_FAIL(run_test(cgroup_fd, server_fd, AF_INET6));
> +	stop_server_thread(server_fd);
Thanks for testing both v6 and v4.

The UDP path should be tested also.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf-next v2 4/5] bpf: allow any port in bpf_bind helper
  2020-05-06  6:23   ` Martin KaFai Lau
@ 2020-05-06 16:22     ` sdf
  2020-05-06 18:09       ` Martin KaFai Lau
  0 siblings, 1 reply; 14+ messages in thread
From: sdf @ 2020-05-06 16:22 UTC (permalink / raw)
  To: Martin KaFai Lau; +Cc: netdev, bpf, davem, ast, daniel, Andrey Ignatov

On 05/05, Martin KaFai Lau wrote:
> On Tue, May 05, 2020 at 01:27:29PM -0700, Stanislav Fomichev wrote:
> > We want to have a tighter control on what ports we bind to in
> > the BPF_CGROUP_INET{4,6}_CONNECT hooks even if it means
> > connect() becomes slightly more expensive. The expensive part
> > comes from the fact that we now need to call inet_csk_get_port()
> > that verifies that the port is not used and allocates an entry
> > in the hash table for it.
> >
> > Since we can't rely on "snum || !bind_address_no_port" to prevent
> > us from calling POST_BIND hook anymore, let's add another bind flag
> > to indicate that the call site is BPF program.
> >
> > v2:
> > * Update documentation (Andrey Ignatov)
> > * Pass BIND_FORCE_ADDRESS_NO_PORT conditionally (Andrey Ignatov)
> >
> > Cc: Andrey Ignatov <rdna@fb.com>
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> >  include/net/inet_common.h                     |   2 +
> >  include/uapi/linux/bpf.h                      |   9 +-
> >  net/core/filter.c                             |  18 ++-
> >  net/ipv4/af_inet.c                            |  10 +-
> >  net/ipv6/af_inet6.c                           |  12 +-
> >  tools/include/uapi/linux/bpf.h                |   9 +-
> >  .../bpf/prog_tests/connect_force_port.c       | 104 ++++++++++++++++++
> >  .../selftests/bpf/progs/connect_force_port4.c |  28 +++++
> >  .../selftests/bpf/progs/connect_force_port6.c |  28 +++++
> >  9 files changed, 192 insertions(+), 28 deletions(-)
> >  create mode 100644  
> tools/testing/selftests/bpf/prog_tests/connect_force_port.c
> >  create mode 100644  
> tools/testing/selftests/bpf/progs/connect_force_port4.c
> >  create mode 100644  
> tools/testing/selftests/bpf/progs/connect_force_port6.c
> >
> > diff --git a/include/net/inet_common.h b/include/net/inet_common.h
> > index c38f4f7d660a..cb2818862919 100644
> > --- a/include/net/inet_common.h
> > +++ b/include/net/inet_common.h
> > @@ -39,6 +39,8 @@ int inet_bind(struct socket *sock, struct sockaddr  
> *uaddr, int addr_len);
> >  #define BIND_FORCE_ADDRESS_NO_PORT	(1 << 0)
> >  /* Grab and release socket lock. */
> >  #define BIND_WITH_LOCK			(1 << 1)
> > +/* Called from BPF program. */
> > +#define BIND_FROM_BPF			(1 << 2)
> >  int __inet_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len,
> >  		u32 flags);
> >  int inet_getname(struct socket *sock, struct sockaddr *uaddr,
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index b3643e27e264..14b5518a3d5b 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -1994,10 +1994,11 @@ union bpf_attr {
> >   *
> >   * 		This helper works for IPv4 and IPv6, TCP and UDP sockets. The
> >   * 		domain (*addr*\ **->sa_family**) must be **AF_INET** (or
> > - * 		**AF_INET6**). Looking for a free port to bind to can be
> > - * 		expensive, therefore binding to port is not permitted by the
> > - * 		helper: *addr*\ **->sin_port** (or **sin6_port**, respectively)
> > - * 		must be set to zero.
> > + * 		**AF_INET6**). It's advised to pass zero port (**sin_port**
> > + * 		or **sin6_port**) which triggers IP_BIND_ADDRESS_NO_PORT-like
> > + * 		behavior and lets the kernel reuse the same source port
> Reading "zero port" and "the same source port" together is confusing.
Ack, let me try rephrase it a bit to make it more clear.

> > + * 		as long as 4-tuple is unique. Passing non-zero port might
> > + * 		lead to degraded performance.
> Is the "degraded performance" also true for UDP?
I suppose everything that is "allocating" port at bind time can lead
to a faster port exhaustion, so UDP should be also affected.
Although, looking at udp_v4_get_port, it looks less involved than
its TCP counterpart.

> >   * 	Return
> >   * 		0 on success, or a negative error in case of failure.
> >   *

> [ ... ]

> > diff --git  
> a/tools/testing/selftests/bpf/prog_tests/connect_force_port.c  
> b/tools/testing/selftests/bpf/prog_tests/connect_force_port.c
> > new file mode 100644
> > index 000000000000..97104e6410b6
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/connect_force_port.c
> > @@ -0,0 +1,104 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <test_progs.h>
> > +#include "cgroup_helpers.h"
> > +#include "network_helpers.h"
> > +
> > +static int verify_port(int family, int fd, int expected)
> > +{
> > +	struct sockaddr_storage addr;
> > +	socklen_t len = sizeof(addr);
> > +	__u16 port;
> > +
> > +
> > +	if (getsockname(fd, (struct sockaddr *)&addr, &len)) {
> > +		log_err("Failed to get server addr");
> > +		return -1;
> > +	}
> > +
> > +	if (family == AF_INET)
> > +		port = ((struct sockaddr_in *)&addr)->sin_port;
> > +	else
> > +		port = ((struct sockaddr_in6 *)&addr)->sin6_port;
> > +
> > +	if (ntohs(port) != expected) {
> > +		log_err("Unexpected port %d, expected %d", ntohs(port),
> > +			expected);
> > +		return -1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int run_test(int cgroup_fd, int server_fd, int family)
> > +{
> > +	struct bpf_prog_load_attr attr = {
> > +		.prog_type = BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
> > +	};
> > +	struct bpf_object *obj;
> > +	int expected_port;
> > +	int prog_fd;
> > +	int err;
> > +	int fd;
> > +
> > +	if (family == AF_INET) {
> > +		attr.file = "./connect_force_port4.o";
> > +		attr.expected_attach_type = BPF_CGROUP_INET4_CONNECT;
> > +		expected_port = 22222;
> > +	} else {
> > +		attr.file = "./connect_force_port6.o";
> > +		attr.expected_attach_type = BPF_CGROUP_INET6_CONNECT;
> > +		expected_port = 22223;
> > +	}
> > +
> > +	err = bpf_prog_load_xattr(&attr, &obj, &prog_fd);
> > +	if (err) {
> > +		log_err("Failed to load BPF object");
> > +		return -1;
> > +	}
> > +
> > +	err = bpf_prog_attach(prog_fd, cgroup_fd, attr.expected_attach_type,
> > +			      0);
> > +	if (err) {
> > +		log_err("Failed to attach BPF program");
> > +		goto close_bpf_object;
> > +	}
> > +
> > +	fd = connect_to_fd(family, server_fd);
> > +	if (fd < 0) {
> > +		err = -1;
> > +		goto close_bpf_object;
> > +	}
> > +
> > +	err = verify_port(family, fd, expected_port);
> > +
> > +	close(fd);
> > +
> > +close_bpf_object:
> > +	bpf_object__close(obj);
> > +	return err;
> > +}
> > +
> > +void test_connect_force_port(void)
> > +{
> > +	int server_fd, cgroup_fd;
> > +
> > +	cgroup_fd = test__join_cgroup("/connect_force_port");
> > +	if (CHECK_FAIL(cgroup_fd < 0))
> > +		return;
> > +
> > +	server_fd = start_server_thread(AF_INET);
> > +	if (CHECK_FAIL(server_fd < 0))
> > +		goto close_cgroup_fd;
> > +	CHECK_FAIL(run_test(cgroup_fd, server_fd, AF_INET));
> > +	stop_server_thread(server_fd);
> > +
> > +	server_fd = start_server_thread(AF_INET6);
> > +	if (CHECK_FAIL(server_fd < 0))
> > +		goto close_cgroup_fd;
> > +	CHECK_FAIL(run_test(cgroup_fd, server_fd, AF_INET6));
> > +	stop_server_thread(server_fd);
> Thanks for testing both v6 and v4.

> The UDP path should be tested also.
Good point, will do!

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf-next v2 4/5] bpf: allow any port in bpf_bind helper
  2020-05-06 16:22     ` sdf
@ 2020-05-06 18:09       ` Martin KaFai Lau
  0 siblings, 0 replies; 14+ messages in thread
From: Martin KaFai Lau @ 2020-05-06 18:09 UTC (permalink / raw)
  To: sdf; +Cc: netdev, bpf, davem, ast, daniel, Andrey Ignatov

On Wed, May 06, 2020 at 09:22:45AM -0700, sdf@google.com wrote:
> On 05/05, Martin KaFai Lau wrote:
> > On Tue, May 05, 2020 at 01:27:29PM -0700, Stanislav Fomichev wrote:
> > > We want to have a tighter control on what ports we bind to in
> > > the BPF_CGROUP_INET{4,6}_CONNECT hooks even if it means
> > > connect() becomes slightly more expensive. The expensive part
> > > comes from the fact that we now need to call inet_csk_get_port()
> > > that verifies that the port is not used and allocates an entry
> > > in the hash table for it.
> > >
> > > Since we can't rely on "snum || !bind_address_no_port" to prevent
> > > us from calling POST_BIND hook anymore, let's add another bind flag
> > > to indicate that the call site is BPF program.
> > >
> > > v2:
> > > * Update documentation (Andrey Ignatov)
> > > * Pass BIND_FORCE_ADDRESS_NO_PORT conditionally (Andrey Ignatov)
> > >
> > > Cc: Andrey Ignatov <rdna@fb.com>
> > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > > ---
> > >  include/net/inet_common.h                     |   2 +
> > >  include/uapi/linux/bpf.h                      |   9 +-
> > >  net/core/filter.c                             |  18 ++-
> > >  net/ipv4/af_inet.c                            |  10 +-
> > >  net/ipv6/af_inet6.c                           |  12 +-
> > >  tools/include/uapi/linux/bpf.h                |   9 +-
> > >  .../bpf/prog_tests/connect_force_port.c       | 104 ++++++++++++++++++
> > >  .../selftests/bpf/progs/connect_force_port4.c |  28 +++++
> > >  .../selftests/bpf/progs/connect_force_port6.c |  28 +++++
> > >  9 files changed, 192 insertions(+), 28 deletions(-)
> > >  create mode 100644
> > tools/testing/selftests/bpf/prog_tests/connect_force_port.c
> > >  create mode 100644
> > tools/testing/selftests/bpf/progs/connect_force_port4.c
> > >  create mode 100644
> > tools/testing/selftests/bpf/progs/connect_force_port6.c
> > >
> > > diff --git a/include/net/inet_common.h b/include/net/inet_common.h
> > > index c38f4f7d660a..cb2818862919 100644
> > > --- a/include/net/inet_common.h
> > > +++ b/include/net/inet_common.h
> > > @@ -39,6 +39,8 @@ int inet_bind(struct socket *sock, struct sockaddr
> > *uaddr, int addr_len);
> > >  #define BIND_FORCE_ADDRESS_NO_PORT	(1 << 0)
> > >  /* Grab and release socket lock. */
> > >  #define BIND_WITH_LOCK			(1 << 1)
> > > +/* Called from BPF program. */
> > > +#define BIND_FROM_BPF			(1 << 2)
> > >  int __inet_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len,
> > >  		u32 flags);
> > >  int inet_getname(struct socket *sock, struct sockaddr *uaddr,
> > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > index b3643e27e264..14b5518a3d5b 100644
> > > --- a/include/uapi/linux/bpf.h
> > > +++ b/include/uapi/linux/bpf.h
> > > @@ -1994,10 +1994,11 @@ union bpf_attr {
> > >   *
> > >   * 		This helper works for IPv4 and IPv6, TCP and UDP sockets. The
> > >   * 		domain (*addr*\ **->sa_family**) must be **AF_INET** (or
> > > - * 		**AF_INET6**). Looking for a free port to bind to can be
> > > - * 		expensive, therefore binding to port is not permitted by the
> > > - * 		helper: *addr*\ **->sin_port** (or **sin6_port**, respectively)
> > > - * 		must be set to zero.
> > > + * 		**AF_INET6**). It's advised to pass zero port (**sin_port**
> > > + * 		or **sin6_port**) which triggers IP_BIND_ADDRESS_NO_PORT-like
> > > + * 		behavior and lets the kernel reuse the same source port
> > Reading "zero port" and "the same source port" together is confusing.
> Ack, let me try rephrase it a bit to make it more clear.
> 
> > > + * 		as long as 4-tuple is unique. Passing non-zero port might
> > > + * 		lead to degraded performance.
> > Is the "degraded performance" also true for UDP?
> I suppose everything that is "allocating" port at bind time can lead
> to a faster port exhaustion, so UDP should be also affected.
> Although, looking at udp_v4_get_port, it looks less involved than
> its TCP counterpart.
I was mostly curious after taking a quick look in both TCP and UDP.
It does not affect the patch here.  May be something for
optimization later.

From skimming the codes,
I was thinking UDP should be more or less the same since
they have to go through get_port() eventually.

TCP will be degraded from inet_hash_connect() [which seems
to be using the ehash and 4 tuples] to get_port().

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH bpf-next v2 5/5] selftests/bpf: move existing common networking parts into network_helpers
  2020-05-05 20:27 [PATCH bpf-next v2 0/5] bpf: allow any port in bpf_bind helper Stanislav Fomichev
                   ` (3 preceding siblings ...)
  2020-05-05 20:27 ` [PATCH bpf-next v2 4/5] bpf: allow any port in bpf_bind helper Stanislav Fomichev
@ 2020-05-05 20:27 ` Stanislav Fomichev
  4 siblings, 0 replies; 14+ messages in thread
From: Stanislav Fomichev @ 2020-05-05 20:27 UTC (permalink / raw)
  To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev

1. Move pkt_v4 and pkt_v6 into network_helpers and adjust the users.
2. Copy-paste spin_lock_thread into two tests that use it.

Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 tools/testing/selftests/bpf/network_helpers.c | 17 +++++++++++
 tools/testing/selftests/bpf/network_helpers.h | 29 ++++++++++++++++++
 .../selftests/bpf/prog_tests/fexit_bpf2bpf.c  |  1 +
 .../selftests/bpf/prog_tests/flow_dissector.c |  1 +
 .../prog_tests/flow_dissector_load_bytes.c    |  1 +
 .../selftests/bpf/prog_tests/global_data.c    |  1 +
 .../selftests/bpf/prog_tests/kfree_skb.c      |  1 +
 .../selftests/bpf/prog_tests/l4lb_all.c       |  1 +
 .../selftests/bpf/prog_tests/map_lock.c       | 14 +++++++++
 .../selftests/bpf/prog_tests/pkt_access.c     |  1 +
 .../selftests/bpf/prog_tests/pkt_md_access.c  |  1 +
 .../selftests/bpf/prog_tests/prog_run_xattr.c |  1 +
 .../bpf/prog_tests/queue_stack_map.c          |  1 +
 .../selftests/bpf/prog_tests/signal_pending.c |  1 +
 .../selftests/bpf/prog_tests/skb_ctx.c        |  1 +
 .../selftests/bpf/prog_tests/spinlock.c       | 14 +++++++++
 tools/testing/selftests/bpf/prog_tests/xdp.c  |  1 +
 .../bpf/prog_tests/xdp_adjust_tail.c          |  1 +
 .../selftests/bpf/prog_tests/xdp_bpf2bpf.c    |  1 +
 .../selftests/bpf/prog_tests/xdp_noinline.c   |  1 +
 tools/testing/selftests/bpf/test_progs.c      | 30 -------------------
 tools/testing/selftests/bpf/test_progs.h      | 23 --------------
 22 files changed, 90 insertions(+), 53 deletions(-)

diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
index 30ef0c7edebf..159ca9225596 100644
--- a/tools/testing/selftests/bpf/network_helpers.c
+++ b/tools/testing/selftests/bpf/network_helpers.c
@@ -25,6 +25,23 @@
 #define log_err(MSG, ...) fprintf(stderr, "(%s:%d: errno: %s) " MSG "\n", \
 	__FILE__, __LINE__, clean_errno(), ##__VA_ARGS__)
 
+struct ipv4_packet pkt_v4 = {
+	.eth.h_proto = __bpf_constant_htons(ETH_P_IP),
+	.iph.ihl = 5,
+	.iph.protocol = IPPROTO_TCP,
+	.iph.tot_len = __bpf_constant_htons(MAGIC_BYTES),
+	.tcp.urg_ptr = 123,
+	.tcp.doff = 5,
+};
+
+struct ipv6_packet pkt_v6 = {
+	.eth.h_proto = __bpf_constant_htons(ETH_P_IPV6),
+	.iph.nexthdr = IPPROTO_TCP,
+	.iph.payload_len = __bpf_constant_htons(MAGIC_BYTES),
+	.tcp.urg_ptr = 123,
+	.tcp.doff = 5,
+};
+
 static int start_server(int family)
 {
 	struct sockaddr_storage addr = {};
diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h
index 74e2c40667a2..205123b53a23 100644
--- a/tools/testing/selftests/bpf/network_helpers.h
+++ b/tools/testing/selftests/bpf/network_helpers.h
@@ -3,6 +3,35 @@
 #define __NETWORK_HELPERS_H
 #include <sys/socket.h>
 #include <sys/types.h>
+#include <linux/types.h>
+typedef __u16 __sum16;
+#include <linux/if_ether.h>
+#include <linux/if_packet.h>
+#include <linux/ip.h>
+#include <linux/ipv6.h>
+#include <netinet/tcp.h>
+#include <bpf/bpf_endian.h>
+
+#define MAGIC_VAL 0x1234
+#define NUM_ITER 100000
+#define VIP_NUM 5
+#define MAGIC_BYTES 123
+
+/* ipv4 test vector */
+struct ipv4_packet {
+	struct ethhdr eth;
+	struct iphdr iph;
+	struct tcphdr tcp;
+} __packed;
+extern struct ipv4_packet pkt_v4;
+
+/* ipv6 test vector */
+struct ipv6_packet {
+	struct ethhdr eth;
+	struct ipv6hdr iph;
+	struct tcphdr tcp;
+} __packed;
+extern struct ipv6_packet pkt_v6;
 
 int start_server_thread(int family);
 void stop_server_thread(int fd);
diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
index c2642517e1d8..a895bfed55db 100644
--- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
+++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (c) 2019 Facebook */
 #include <test_progs.h>
+#include <network_helpers.h>
 
 static void test_fexit_bpf2bpf_common(const char *obj_file,
 				      const char *target_obj_file,
diff --git a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
index 92563898867c..2301c4d3ecec 100644
--- a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
+++ b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
+#include <network_helpers.h>
 #include <error.h>
 #include <linux/if.h>
 #include <linux/if_tun.h>
diff --git a/tools/testing/selftests/bpf/prog_tests/flow_dissector_load_bytes.c b/tools/testing/selftests/bpf/prog_tests/flow_dissector_load_bytes.c
index dc5ef155ec28..0e8a4d2f023d 100644
--- a/tools/testing/selftests/bpf/prog_tests/flow_dissector_load_bytes.c
+++ b/tools/testing/selftests/bpf/prog_tests/flow_dissector_load_bytes.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
+#include <network_helpers.h>
 
 void test_flow_dissector_load_bytes(void)
 {
diff --git a/tools/testing/selftests/bpf/prog_tests/global_data.c b/tools/testing/selftests/bpf/prog_tests/global_data.c
index c680926fce73..e3cb62b0a110 100644
--- a/tools/testing/selftests/bpf/prog_tests/global_data.c
+++ b/tools/testing/selftests/bpf/prog_tests/global_data.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
+#include <network_helpers.h>
 
 static void test_global_data_number(struct bpf_object *obj, __u32 duration)
 {
diff --git a/tools/testing/selftests/bpf/prog_tests/kfree_skb.c b/tools/testing/selftests/bpf/prog_tests/kfree_skb.c
index 7507c8f689bc..42c3a3103c26 100644
--- a/tools/testing/selftests/bpf/prog_tests/kfree_skb.c
+++ b/tools/testing/selftests/bpf/prog_tests/kfree_skb.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
+#include <network_helpers.h>
 
 struct meta {
 	int ifindex;
diff --git a/tools/testing/selftests/bpf/prog_tests/l4lb_all.c b/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
index eaf64595be88..c2d373e294bb 100644
--- a/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
+++ b/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
+#include <network_helpers.h>
 
 static void test_l4lb(const char *file)
 {
diff --git a/tools/testing/selftests/bpf/prog_tests/map_lock.c b/tools/testing/selftests/bpf/prog_tests/map_lock.c
index 8f91f1881d11..ce17b1ed8709 100644
--- a/tools/testing/selftests/bpf/prog_tests/map_lock.c
+++ b/tools/testing/selftests/bpf/prog_tests/map_lock.c
@@ -1,5 +1,19 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
+#include <network_helpers.h>
+
+static void *spin_lock_thread(void *arg)
+{
+	__u32 duration, retval;
+	int err, prog_fd = *(u32 *) arg;
+
+	err = bpf_prog_test_run(prog_fd, 10000, &pkt_v4, sizeof(pkt_v4),
+				NULL, NULL, &retval, &duration);
+	CHECK(err || retval, "",
+	      "err %d errno %d retval %d duration %d\n",
+	      err, errno, retval, duration);
+	pthread_exit(arg);
+}
 
 static void *parallel_map_access(void *arg)
 {
diff --git a/tools/testing/selftests/bpf/prog_tests/pkt_access.c b/tools/testing/selftests/bpf/prog_tests/pkt_access.c
index a2537dfa899c..44b514fabccd 100644
--- a/tools/testing/selftests/bpf/prog_tests/pkt_access.c
+++ b/tools/testing/selftests/bpf/prog_tests/pkt_access.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
+#include <network_helpers.h>
 
 void test_pkt_access(void)
 {
diff --git a/tools/testing/selftests/bpf/prog_tests/pkt_md_access.c b/tools/testing/selftests/bpf/prog_tests/pkt_md_access.c
index 5f7aea605019..939015cd6dba 100644
--- a/tools/testing/selftests/bpf/prog_tests/pkt_md_access.c
+++ b/tools/testing/selftests/bpf/prog_tests/pkt_md_access.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
+#include <network_helpers.h>
 
 void test_pkt_md_access(void)
 {
diff --git a/tools/testing/selftests/bpf/prog_tests/prog_run_xattr.c b/tools/testing/selftests/bpf/prog_tests/prog_run_xattr.c
index 5dd89b941f53..dde2b7ae7bc9 100644
--- a/tools/testing/selftests/bpf/prog_tests/prog_run_xattr.c
+++ b/tools/testing/selftests/bpf/prog_tests/prog_run_xattr.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
+#include <network_helpers.h>
 
 void test_prog_run_xattr(void)
 {
diff --git a/tools/testing/selftests/bpf/prog_tests/queue_stack_map.c b/tools/testing/selftests/bpf/prog_tests/queue_stack_map.c
index faccc66f4e39..f47e7b1cb32c 100644
--- a/tools/testing/selftests/bpf/prog_tests/queue_stack_map.c
+++ b/tools/testing/selftests/bpf/prog_tests/queue_stack_map.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
+#include <network_helpers.h>
 
 enum {
 	QUEUE,
diff --git a/tools/testing/selftests/bpf/prog_tests/signal_pending.c b/tools/testing/selftests/bpf/prog_tests/signal_pending.c
index 996e808f43a2..dfcbddcbe4d3 100644
--- a/tools/testing/selftests/bpf/prog_tests/signal_pending.c
+++ b/tools/testing/selftests/bpf/prog_tests/signal_pending.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
+#include <network_helpers.h>
 
 static void sigalrm_handler(int s) {}
 static struct sigaction sigalrm_action = {
diff --git a/tools/testing/selftests/bpf/prog_tests/skb_ctx.c b/tools/testing/selftests/bpf/prog_tests/skb_ctx.c
index 4538bd08203f..7021b92af313 100644
--- a/tools/testing/selftests/bpf/prog_tests/skb_ctx.c
+++ b/tools/testing/selftests/bpf/prog_tests/skb_ctx.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
+#include <network_helpers.h>
 
 void test_skb_ctx(void)
 {
diff --git a/tools/testing/selftests/bpf/prog_tests/spinlock.c b/tools/testing/selftests/bpf/prog_tests/spinlock.c
index 1ae00cd3174e..7577a77a4c4c 100644
--- a/tools/testing/selftests/bpf/prog_tests/spinlock.c
+++ b/tools/testing/selftests/bpf/prog_tests/spinlock.c
@@ -1,5 +1,19 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
+#include <network_helpers.h>
+
+static void *spin_lock_thread(void *arg)
+{
+	__u32 duration, retval;
+	int err, prog_fd = *(u32 *) arg;
+
+	err = bpf_prog_test_run(prog_fd, 10000, &pkt_v4, sizeof(pkt_v4),
+				NULL, NULL, &retval, &duration);
+	CHECK(err || retval, "",
+	      "err %d errno %d retval %d duration %d\n",
+	      err, errno, retval, duration);
+	pthread_exit(arg);
+}
 
 void test_spinlock(void)
 {
diff --git a/tools/testing/selftests/bpf/prog_tests/xdp.c b/tools/testing/selftests/bpf/prog_tests/xdp.c
index dcb5ecac778e..48921ff74850 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
+#include <network_helpers.h>
 
 void test_xdp(void)
 {
diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
index 3744196d7cba..6c8ca1c93f9b 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
+#include <network_helpers.h>
 
 void test_xdp_adjust_tail(void)
 {
diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/xdp_bpf2bpf.c
index a0f688c37023..2c6c570b21f8 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_bpf2bpf.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_bpf2bpf.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
+#include <network_helpers.h>
 #include <net/if.h>
 #include "test_xdp.skel.h"
 #include "test_xdp_bpf2bpf.skel.h"
diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c b/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c
index c9404e6b226e..f284f72158ef 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
+#include <network_helpers.h>
 
 void test_xdp_noinline(void)
 {
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 93970ec1c9e9..0f411fdc4f6d 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -222,23 +222,6 @@ int test__join_cgroup(const char *path)
 	return fd;
 }
 
-struct ipv4_packet pkt_v4 = {
-	.eth.h_proto = __bpf_constant_htons(ETH_P_IP),
-	.iph.ihl = 5,
-	.iph.protocol = IPPROTO_TCP,
-	.iph.tot_len = __bpf_constant_htons(MAGIC_BYTES),
-	.tcp.urg_ptr = 123,
-	.tcp.doff = 5,
-};
-
-struct ipv6_packet pkt_v6 = {
-	.eth.h_proto = __bpf_constant_htons(ETH_P_IPV6),
-	.iph.nexthdr = IPPROTO_TCP,
-	.iph.payload_len = __bpf_constant_htons(MAGIC_BYTES),
-	.tcp.urg_ptr = 123,
-	.tcp.doff = 5,
-};
-
 int bpf_find_map(const char *test, struct bpf_object *obj, const char *name)
 {
 	struct bpf_map *map;
@@ -358,19 +341,6 @@ int extract_build_id(char *build_id, size_t size)
 	return -1;
 }
 
-void *spin_lock_thread(void *arg)
-{
-	__u32 duration, retval;
-	int err, prog_fd = *(u32 *) arg;
-
-	err = bpf_prog_test_run(prog_fd, 10000, &pkt_v4, sizeof(pkt_v4),
-				NULL, NULL, &retval, &duration);
-	CHECK(err || retval, "",
-	      "err %d errno %d retval %d duration %d\n",
-	      err, errno, retval, duration);
-	pthread_exit(arg);
-}
-
 /* extern declarations for test funcs */
 #define DEFINE_TEST(name) extern void test_##name(void);
 #include <prog_tests/tests.h>
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index 10188cc8e9e0..83287c76332b 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -87,24 +87,6 @@ extern void test__skip(void);
 extern void test__fail(void);
 extern int test__join_cgroup(const char *path);
 
-#define MAGIC_BYTES 123
-
-/* ipv4 test vector */
-struct ipv4_packet {
-	struct ethhdr eth;
-	struct iphdr iph;
-	struct tcphdr tcp;
-} __packed;
-extern struct ipv4_packet pkt_v4;
-
-/* ipv6 test vector */
-struct ipv6_packet {
-	struct ethhdr eth;
-	struct ipv6hdr iph;
-	struct tcphdr tcp;
-} __packed;
-extern struct ipv6_packet pkt_v6;
-
 #define PRINT_FAIL(format...)                                                  \
 	({                                                                     \
 		test__fail();                                                  \
@@ -143,10 +125,6 @@ extern struct ipv6_packet pkt_v6;
 #define CHECK_ATTR(condition, tag, format...) \
 	_CHECK(condition, tag, tattr.duration, format)
 
-#define MAGIC_VAL 0x1234
-#define NUM_ITER 100000
-#define VIP_NUM 5
-
 static inline __u64 ptr_to_u64(const void *ptr)
 {
 	return (__u64) (unsigned long) ptr;
@@ -156,7 +134,6 @@ int bpf_find_map(const char *test, struct bpf_object *obj, const char *name);
 int compare_map_keys(int map1_fd, int map2_fd);
 int compare_stack_ips(int smap_fd, int amap_fd, int stack_trace_len);
 int extract_build_id(char *build_id, size_t size);
-void *spin_lock_thread(void *arg);
 
 #ifdef __x86_64__
 #define SYS_NANOSLEEP_KPROBE_NAME "__x64_sys_nanosleep"
-- 
2.26.2.526.g744177e7f7-goog


^ permalink raw reply related	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-05-06 18:09 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-05-05 20:27 [PATCH bpf-next v2 0/5] bpf: allow any port in bpf_bind helper Stanislav Fomichev
2020-05-05 20:27 ` [PATCH bpf-next v2 1/5] selftests/bpf: generalize helpers to control background listener Stanislav Fomichev
2020-05-06  7:00   ` Martin KaFai Lau
2020-05-06 16:28     ` sdf
2020-05-06 17:48       ` Martin KaFai Lau
2020-05-06 18:09         ` sdf
2020-05-05 20:27 ` [PATCH bpf-next v2 2/5] selftests/bpf: adopt accept_timeout from sockmap_listen Stanislav Fomichev
2020-05-05 20:27 ` [PATCH bpf-next v2 3/5] net: refactor arguments of inet{,6}_bind Stanislav Fomichev
2020-05-05 20:27 ` [PATCH bpf-next v2 4/5] bpf: allow any port in bpf_bind helper Stanislav Fomichev
2020-05-05 20:33   ` Andrey Ignatov
2020-05-06  6:23   ` Martin KaFai Lau
2020-05-06 16:22     ` sdf
2020-05-06 18:09       ` Martin KaFai Lau
2020-05-05 20:27 ` [PATCH bpf-next v2 5/5] selftests/bpf: move existing common networking parts into network_helpers Stanislav Fomichev

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).