diff mbox series

[v1,bpf-next,5/8] selftests/bpf: Factor out load_path and defines from test_sock_addr

Message ID 20240329191907.1808635-6-jrife@google.com
State New
Headers show
Series selftests/bpf: Add sockaddr tests for kernel networking | expand

Commit Message

Jordan Rife March 29, 2024, 7:18 p.m. UTC 
Factor out useful defines and load_path() from the test_sock_addr self
test to use them in sock_addr_kern.

Signed-off-by: Jordan Rife <jrife@google.com>
---
 tools/testing/selftests/bpf/Makefile          | 34 +++++----
 .../testing/selftests/bpf/sock_addr_helpers.c | 46 +++++++++++
 .../testing/selftests/bpf/sock_addr_helpers.h | 44 +++++++++++
 tools/testing/selftests/bpf/test_sock_addr.c  | 76 +++----------------
 4 files changed, 120 insertions(+), 80 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/sock_addr_helpers.c
 create mode 100644 tools/testing/selftests/bpf/sock_addr_helpers.h

Comments

Martin KaFai Lau April 2, 2024, 11:14 p.m. UTC | #1 
On 3/29/24 12:18 PM, Jordan Rife wrote:
> diff --git a/tools/testing/selftests/bpf/sock_addr_helpers.c b/tools/testing/selftests/bpf/sock_addr_helpers.c
> new file mode 100644
> index 0000000000000..ff2eb09870f16
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/sock_addr_helpers.c
> @@ -0,0 +1,46 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +
> +#include <bpf/bpf.h>
> +#include <bpf/libbpf.h>
> +
> +#include "cgroup_helpers.h"
> +#include "sock_addr_helpers.h"
> +#include "testing_helpers.h"
> +
> +int load_path(const char *path, enum bpf_attach_type attach_type,
> +	      bool expect_reject)
> +{
> +	struct bpf_object *obj;
> +	struct bpf_program *prog;
> +	int err;
> +
> +	obj = bpf_object__open_file(path, NULL);

Although it works, it is heading to the opposite direction by reusing things 
from the older test_sock_addr.c.

test_sock_addr.c should have been moved to the test_progs. It is not run by bpf 
CI and bits get rotten [e.g. the bug fix in patch 8]. There is also old practice 
like bpf_object__open_file() should have been replaced with the skeleton 
__open_and_load() instead of refactoring it out to create new use cases.

The newer prog_tests/sock_addr.c was created when adding AF_UNIX support. It has 
a very similar setup as the older test_sock_addr.c and the intention was to 
finally retire test_sock_addr.c. e.g. It also has "load_fn loadfn" but is done 
with skeleton, the program is also attached to cgroup...etc.

Instead of adding a new sock_addr_kern.c in patch 7, it probably will be easier 
to add the kernel socket tests into the existing prog_tests/sock_addr.c.

Also setup the netns and veth in the prog_tests/sock_addr.c instead of calling 
out the test_sock_addr.sh (which should also go away eventually), there are 
examples in prog_tests/ (e.g. mptcp.c).

> +	err = libbpf_get_error(obj);
> +	if (err) {
> +		log_err(">>> Opening BPF object (%s) error.\n", path);
> +		return -1;
> +	}
> +
> +	prog = bpf_object__next_program(obj, NULL);
> +	if (!prog)
> +		goto err_out;
> +
> +	bpf_program__set_type(prog, BPF_PROG_TYPE_CGROUP_SOCK_ADDR);
> +	bpf_program__set_expected_attach_type(prog, attach_type);
> +	bpf_program__set_flags(prog, testing_prog_flags());
> +
> +	err = bpf_object__load(obj);
> +	if (err) {
> +		if (!expect_reject)
> +			log_err(">>> Loading program (%s) error.\n", path);
> +		goto err_out;
> +	}
> +
> +	return bpf_program__fd(prog);
> +err_out:
> +	bpf_object__close(obj);
> +	return -1;
> +}
Jordan Rife April 3, 2024, 12:05 a.m. UTC | #2 
> The newer prog_tests/sock_addr.c was created when adding AF_UNIX support. It has
> a very similar setup as the older test_sock_addr.c and the intention was to
> finally retire test_sock_addr.c. e.g. It also has "load_fn loadfn" but is done
> with skeleton, the program is also attached to cgroup...etc.

Thanks, I wasn't sure what the preferred direction would be here. I
saw some discussion
in the AF_UNIX support patch series about prog_tests being the
preferred approach and
there being plans to migrate the old test_sock_addr.c tests to the new format.

I suspect the changes mentioned here (load done with skeleton, etc.)
and helpers needed
to support the tests in my patch series may also be helpful in
retiring test_sock_addr.c.

> Instead of adding a new sock_addr_kern.c in patch 7, it probably will be easier
> to add the kernel socket tests into the existing prog_tests/sock_addr.c.

Originally it felt awkward to try to merge these tests with
prog_tests/sock_addr.c, because
while the goals are similar the flow is different with
loading/unloading a kernel module to
drive socket operations. I'll revisit this and see if I can merge
everything into
prog_tests/sock_addr.c. Switching to kfuncs as mentioned earlier might
make this simpler
as well.

> Also setup the netns and veth in the prog_tests/sock_addr.c instead of calling
> out the test_sock_addr.sh (which should also go away eventually), there are
> examples in prog_tests/ (e.g. mptcp.c).

Ack. Sure thing.

-Jordan
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index b5d02ff724957..ccc1c11559a45 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -291,6 +291,7 @@  TEST_GEN_PROGS_EXTENDED += $(TRUNNER_BPFTOOL)
 
 $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED): $(BPFOBJ)
 
+SOCK_ADDR_HELPERS := $(OUTPUT)/sock_addr_helpers.o
 TESTING_HELPERS	:= $(OUTPUT)/testing_helpers.o
 CGROUP_HELPERS	:= $(OUTPUT)/cgroup_helpers.o
 UNPRIV_HELPERS  := $(OUTPUT)/unpriv_helpers.o
@@ -301,7 +302,7 @@  CAP_HELPERS	:= $(OUTPUT)/cap_helpers.o
 $(OUTPUT)/test_dev_cgroup: $(CGROUP_HELPERS) $(TESTING_HELPERS)
 $(OUTPUT)/test_skb_cgroup_id_user: $(CGROUP_HELPERS) $(TESTING_HELPERS)
 $(OUTPUT)/test_sock: $(CGROUP_HELPERS) $(TESTING_HELPERS)
-$(OUTPUT)/test_sock_addr: $(CGROUP_HELPERS) $(TESTING_HELPERS)
+$(OUTPUT)/test_sock_addr: $(CGROUP_HELPERS) $(TESTING_HELPERS) $(SOCK_ADDR_HELPERS)
 $(OUTPUT)/test_sockmap: $(CGROUP_HELPERS) $(TESTING_HELPERS)
 $(OUTPUT)/test_tcpnotify_user: $(CGROUP_HELPERS) $(TESTING_HELPERS) $(TRACE_HELPERS)
 $(OUTPUT)/get_cgroup_id_user: $(CGROUP_HELPERS) $(TESTING_HELPERS)
@@ -628,21 +629,22 @@  endef
 # Define test_progs test runner.
 TRUNNER_TESTS_DIR := prog_tests
 TRUNNER_BPF_PROGS_DIR := progs
-TRUNNER_EXTRA_SOURCES := test_progs.c		\
-			 cgroup_helpers.c	\
-			 trace_helpers.c	\
-			 network_helpers.c	\
-			 testing_helpers.c	\
-			 btf_helpers.c		\
-			 cap_helpers.c		\
-			 unpriv_helpers.c 	\
-			 netlink_helpers.c	\
-			 test_loader.c		\
-			 xsk.c			\
-			 disasm.c		\
-			 json_writer.c 		\
-			 flow_dissector_load.h	\
-			 ip_check_defrag_frags.h
+TRUNNER_EXTRA_SOURCES := test_progs.c		 \
+			 cgroup_helpers.c	 \
+			 trace_helpers.c	 \
+			 network_helpers.c	 \
+			 testing_helpers.c	 \
+			 btf_helpers.c		 \
+			 cap_helpers.c		 \
+			 unpriv_helpers.c 	 \
+			 netlink_helpers.c	 \
+			 test_loader.c		 \
+			 xsk.c			 \
+			 disasm.c		 \
+			 json_writer.c 		 \
+			 flow_dissector_load.h	 \
+			 ip_check_defrag_frags.h \
+			 sock_addr_helpers.c
 TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko	\
 		       $(OUTPUT)/bpf_test_no_cfi.ko			\
 		       $(OUTPUT)/sock_addr_testmod.ko	\
diff --git a/tools/testing/selftests/bpf/sock_addr_helpers.c b/tools/testing/selftests/bpf/sock_addr_helpers.c
new file mode 100644
index 0000000000000..ff2eb09870f16
--- /dev/null
+++ b/tools/testing/selftests/bpf/sock_addr_helpers.c
@@ -0,0 +1,46 @@ 
+// SPDX-License-Identifier: GPL-2.0
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+#include <bpf/bpf.h>
+#include <bpf/libbpf.h>
+
+#include "cgroup_helpers.h"
+#include "sock_addr_helpers.h"
+#include "testing_helpers.h"
+
+int load_path(const char *path, enum bpf_attach_type attach_type,
+	      bool expect_reject)
+{
+	struct bpf_object *obj;
+	struct bpf_program *prog;
+	int err;
+
+	obj = bpf_object__open_file(path, NULL);
+	err = libbpf_get_error(obj);
+	if (err) {
+		log_err(">>> Opening BPF object (%s) error.\n", path);
+		return -1;
+	}
+
+	prog = bpf_object__next_program(obj, NULL);
+	if (!prog)
+		goto err_out;
+
+	bpf_program__set_type(prog, BPF_PROG_TYPE_CGROUP_SOCK_ADDR);
+	bpf_program__set_expected_attach_type(prog, attach_type);
+	bpf_program__set_flags(prog, testing_prog_flags());
+
+	err = bpf_object__load(obj);
+	if (err) {
+		if (!expect_reject)
+			log_err(">>> Loading program (%s) error.\n", path);
+		goto err_out;
+	}
+
+	return bpf_program__fd(prog);
+err_out:
+	bpf_object__close(obj);
+	return -1;
+}
diff --git a/tools/testing/selftests/bpf/sock_addr_helpers.h b/tools/testing/selftests/bpf/sock_addr_helpers.h
new file mode 100644
index 0000000000000..cb17579075143
--- /dev/null
+++ b/tools/testing/selftests/bpf/sock_addr_helpers.h
@@ -0,0 +1,44 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __SOCK_ADDR_HELPERS_H
+#define __SOCK_ADDR_HELPERS_H
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+#include <bpf/bpf.h>
+#include <bpf/libbpf.h>
+
+#define CONNECT4_PROG_PATH	"./connect4_prog.bpf.o"
+#define CONNECT6_PROG_PATH	"./connect6_prog.bpf.o"
+#define CONNECTUN_PROG_PATH	"./connect_unix_prog.bpf.o"
+#define SENDMSG4_PROG_PATH	"./sendmsg4_prog.bpf.o"
+#define SENDMSG6_PROG_PATH	"./sendmsg6_prog.bpf.o"
+#define RECVMSG4_PROG_PATH	"./recvmsg4_prog.bpf.o"
+#define RECVMSG6_PROG_PATH	"./recvmsg6_prog.bpf.o"
+#define BIND4_PROG_PATH		"./bind4_prog.bpf.o"
+#define BIND6_PROG_PATH		"./bind6_prog.bpf.o"
+
+#define SERV4_IP		"192.168.1.254"
+#define SERV4_REWRITE_IP	"127.0.0.1"
+#define SRC4_IP			"172.16.0.1"
+#define SRC4_REWRITE_IP		"127.0.0.4"
+#define SERV4_PORT		4040
+#define SERV4_REWRITE_PORT	4444
+
+#define SERV6_IP		"face:b00c:1234:5678::abcd"
+#define SERV6_REWRITE_IP	"::1"
+#define SERV6_V4MAPPED_IP	"::ffff:192.168.0.4"
+#define SRC6_IP			"::1"
+#define SRC6_REWRITE_IP		"::6"
+#define WILDCARD6_IP		"::"
+#define SERV6_PORT		6060
+#define SERV6_REWRITE_PORT	6666
+
+#define SERVUN_ADDRESS         "bpf_cgroup_unix_test"
+#define SERVUN_REWRITE_ADDRESS "bpf_cgroup_unix_test_rewrite"
+
+int load_path(const char *path, enum bpf_attach_type attach_type,
+	      bool expected_reject);
+
+#endif
diff --git a/tools/testing/selftests/bpf/test_sock_addr.c b/tools/testing/selftests/bpf/test_sock_addr.c
index 80c42583f5977..89c106832f9a4 100644
--- a/tools/testing/selftests/bpf/test_sock_addr.c
+++ b/tools/testing/selftests/bpf/test_sock_addr.c
@@ -21,37 +21,13 @@ 
 #include "cgroup_helpers.h"
 #include "testing_helpers.h"
 #include "bpf_util.h"
+#include "sock_addr_helpers.h"
 
 #ifndef ENOTSUPP
 # define ENOTSUPP 524
 #endif
 
 #define CG_PATH	"/foo"
-#define CONNECT4_PROG_PATH	"./connect4_prog.bpf.o"
-#define CONNECT6_PROG_PATH	"./connect6_prog.bpf.o"
-#define SENDMSG4_PROG_PATH	"./sendmsg4_prog.bpf.o"
-#define SENDMSG6_PROG_PATH	"./sendmsg6_prog.bpf.o"
-#define RECVMSG4_PROG_PATH	"./recvmsg4_prog.bpf.o"
-#define RECVMSG6_PROG_PATH	"./recvmsg6_prog.bpf.o"
-#define BIND4_PROG_PATH		"./bind4_prog.bpf.o"
-#define BIND6_PROG_PATH		"./bind6_prog.bpf.o"
-
-#define SERV4_IP		"192.168.1.254"
-#define SERV4_REWRITE_IP	"127.0.0.1"
-#define SRC4_IP			"172.16.0.1"
-#define SRC4_REWRITE_IP		"127.0.0.4"
-#define SERV4_PORT		4040
-#define SERV4_REWRITE_PORT	4444
-
-#define SERV6_IP		"face:b00c:1234:5678::abcd"
-#define SERV6_REWRITE_IP	"::1"
-#define SERV6_V4MAPPED_IP	"::ffff:192.168.0.4"
-#define SRC6_IP			"::1"
-#define SRC6_REWRITE_IP		"::6"
-#define WILDCARD6_IP		"::"
-#define SERV6_PORT		6060
-#define SERV6_REWRITE_PORT	6666
-
 #define INET_NTOP_BUF	40
 
 struct sock_addr_test;
@@ -661,58 +637,30 @@  static int load_insns(const struct sock_addr_test *test,
 	return ret;
 }
 
-static int load_path(const struct sock_addr_test *test, const char *path)
+static int ld_path(const struct sock_addr_test *test, const char *path)
 {
-	struct bpf_object *obj;
-	struct bpf_program *prog;
-	int err;
-
-	obj = bpf_object__open_file(path, NULL);
-	err = libbpf_get_error(obj);
-	if (err) {
-		log_err(">>> Opening BPF object (%s) error.\n", path);
-		return -1;
-	}
-
-	prog = bpf_object__next_program(obj, NULL);
-	if (!prog)
-		goto err_out;
-
-	bpf_program__set_type(prog, BPF_PROG_TYPE_CGROUP_SOCK_ADDR);
-	bpf_program__set_expected_attach_type(prog, test->expected_attach_type);
-	bpf_program__set_flags(prog, testing_prog_flags());
-
-	err = bpf_object__load(obj);
-	if (err) {
-		if (test->expected_result != LOAD_REJECT)
-			log_err(">>> Loading program (%s) error.\n", path);
-		goto err_out;
-	}
-
-	return bpf_program__fd(prog);
-err_out:
-	bpf_object__close(obj);
-	return -1;
+	return load_path(path, test->expected_attach_type,
+			 test->expected_result == LOAD_REJECT);
 }
 
 static int bind4_prog_load(const struct sock_addr_test *test)
 {
-	return load_path(test, BIND4_PROG_PATH);
+	return ld_path(test, BIND4_PROG_PATH);
 }
 
 static int bind6_prog_load(const struct sock_addr_test *test)
 {
-	return load_path(test, BIND6_PROG_PATH);
+	return ld_path(test, BIND6_PROG_PATH);
 }
 
 static int connect4_prog_load(const struct sock_addr_test *test)
 {
-	return load_path(test, CONNECT4_PROG_PATH);
+	return ld_path(test, CONNECT4_PROG_PATH);
 }
 
 static int connect6_prog_load(const struct sock_addr_test *test)
 {
-	return load_path(test, CONNECT6_PROG_PATH);
+	return ld_path(test, CONNECT6_PROG_PATH);
 }
 
 static int xmsg_ret_only_prog_load(const struct sock_addr_test *test,
@@ -800,12 +748,12 @@  static int sendmsg4_rw_asm_prog_load(const struct sock_addr_test *test)
 
 static int recvmsg4_rw_c_prog_load(const struct sock_addr_test *test)
 {
-	return load_path(test, RECVMSG4_PROG_PATH);
+	return ld_path(test, RECVMSG4_PROG_PATH);
 }
 
 static int sendmsg4_rw_c_prog_load(const struct sock_addr_test *test)
 {
-	return load_path(test, SENDMSG4_PROG_PATH);
+	return ld_path(test, SENDMSG4_PROG_PATH);
 }
 
 static int sendmsg6_rw_dst_asm_prog_load(const struct sock_addr_test *test,
@@ -868,7 +816,7 @@  static int sendmsg6_rw_asm_prog_load(const struct sock_addr_test *test)
 
 static int recvmsg6_rw_c_prog_load(const struct sock_addr_test *test)
 {
-	return load_path(test, RECVMSG6_PROG_PATH);
+	return ld_path(test, RECVMSG6_PROG_PATH);
 }
 
 static int sendmsg6_rw_v4mapped_prog_load(const struct sock_addr_test *test)
@@ -883,7 +831,7 @@  static int sendmsg6_rw_wildcard_prog_load(const struct sock_addr_test *test)
 
 static int sendmsg6_rw_c_prog_load(const struct sock_addr_test *test)
 {
-	return load_path(test, SENDMSG6_PROG_PATH);
+	return ld_path(test, SENDMSG6_PROG_PATH);
 }
 
 static int cmp_addr(const struct sockaddr_storage *addr1,