diff mbox series

[v3,9/9] selftests/bpf/sockopt: Add io_uring support

Message ID 20230817145554.892543-10-leitao@debian.org
State New
Headers show
Series None | expand

Commit Message

Breno Leitao Aug. 17, 2023, 2:55 p.m. UTC 
Expand the sockopt test to use also check for io_uring {g,s}etsockopt
commands operations.

This patch starts by marking marks each test if they support io_uring
support or not. Right now, io_uring cmd getsockopt() has a limitation of
only accepting level == SOL_SOCKET, otherwise it returns -EOPNOTSUPP.

Later, each test runs using regular {g,s}etsockopt systemcalls, and, if
liburing is supported, execute the same test (again), but calling
liburing {g,s}setsockopt commands.

For that, leverage the mini liburing to do basic io_uring operations.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 tools/testing/selftests/bpf/Makefile          |   1 +
 .../selftests/bpf/prog_tests/sockopt.c        | 129 +++++++++++++++++-
 2 files changed, 124 insertions(+), 6 deletions(-)

Comments

Martin KaFai Lau Aug. 21, 2023, 8:59 p.m. UTC | #1 
On 8/17/23 7:55 AM, Breno Leitao wrote:
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 538df8fb8c42..4da04242b848 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -362,6 +362,7 @@ CLANG_CFLAGS = $(CLANG_SYS_INCLUDES) \
>   
>   $(OUTPUT)/test_l4lb_noinline.o: BPF_CFLAGS += -fno-inline
>   $(OUTPUT)/test_xdp_noinline.o: BPF_CFLAGS += -fno-inline
> +$(OUTPUT)/test_progs.o: CFLAGS += -I../../../include/

This is the tools/include? Is it really needed? iirc, some of the prog_tests/*.c 
has already been using files from tools/include.

>   
>   $(OUTPUT)/flow_dissector_load.o: flow_dissector_load.h
>   $(OUTPUT)/cgroup_getset_retval_hooks.o: cgroup_getset_retval_hooks.h
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt.c b/tools/testing/selftests/bpf/prog_tests/sockopt.c
> index 9e6a5e3ed4de..4693ad8bfe8f 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockopt.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockopt.c
> @@ -1,5 +1,6 @@
>   // SPDX-License-Identifier: GPL-2.0
>   #include <test_progs.h>
> +#include <io_uring/mini_liburing.h>
>   #include "cgroup_helpers.h"
>   
>   static char bpf_log_buf[4096];
> @@ -38,6 +39,7 @@ static struct sockopt_test {
>   	socklen_t			get_optlen_ret;
>   
>   	enum sockopt_test_error		error;
> +	bool				io_uring_support;
>   } tests[] = {
>   
>   	/* ==================== getsockopt ====================  */
> @@ -53,6 +55,7 @@ static struct sockopt_test {
>   		.attach_type = BPF_CGROUP_GETSOCKOPT,
>   		.expected_attach_type = 0,
>   		.error = DENY_LOAD,
> +		.io_uring_support = true,

DENY_LOAD probably won't be an intersting test. The set/getsockopt won't be called.

The existing test does not seem to have SOL_SOCKET for getsockopt also.

> -static int run_test(int cgroup_fd, struct sockopt_test *test)
> +/* Core function that handles io_uring ring initialization,
> + * sending SQE with sockopt command and waiting for the CQE.
> + */
> +static int uring_sockopt(int op, int fd, int level, int optname,
> +			 const void *optval, socklen_t optlen)
> +{
> +	struct io_uring_cqe *cqe;
> +	struct io_uring_sqe *sqe;
> +	struct io_uring ring;
> +	int err;
> +
> +	err = io_uring_queue_init(1, &ring, 0);
> +	if (err) {
> +		log_err("Failed to initialize io_uring ring");
> +		return err;
> +	}
> +
> +	sqe = io_uring_get_sqe(&ring);
> +	if (!sqe) {
> +		log_err("Failed to get an SQE");
> +		return -1;

No need to io_uring_queue_exit() on the error path?


> +	}
> +
> +	io_uring_prep_cmd(sqe, op, fd, level, optname, optval, optlen);
> +
> +	err = io_uring_submit(&ring);
> +	if (err != 1) {
> +		log_err("Failed to submit SQE");

Use ASSERT_* instead.

Regarding how to land this set,
it will be useful to have the selftest running in the bpf CI. While there is 
iouring changes, some of the changes is in bpf and/or netdev also. eg. Patch 3 
already has a conflict with the net-next and bpf-next tree because of a recent 
commit in socket.c on Aug 9.

May be Alexi and Daniel can advise how was similar patch managed before ?
Breno Leitao Aug. 25, 2023, 2:15 p.m. UTC | #2 
On Mon, Aug 21, 2023 at 01:59:12PM -0700, Martin KaFai Lau wrote:
> On 8/17/23 7:55 AM, Breno Leitao wrote:
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index 538df8fb8c42..4da04242b848 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -362,6 +362,7 @@ CLANG_CFLAGS = $(CLANG_SYS_INCLUDES) \
> >   $(OUTPUT)/test_l4lb_noinline.o: BPF_CFLAGS += -fno-inline
> >   $(OUTPUT)/test_xdp_noinline.o: BPF_CFLAGS += -fno-inline
> > +$(OUTPUT)/test_progs.o: CFLAGS += -I../../../include/
> 
> This is the tools/include? Is it really needed? iirc, some of the
> prog_tests/*.c has already been using files from tools/include.

You are right, we don't need it.

> >   $(OUTPUT)/flow_dissector_load.o: flow_dissector_load.h
> >   $(OUTPUT)/cgroup_getset_retval_hooks.o: cgroup_getset_retval_hooks.h
> > diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt.c b/tools/testing/selftests/bpf/prog_tests/sockopt.c
> > index 9e6a5e3ed4de..4693ad8bfe8f 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/sockopt.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/sockopt.c
> > @@ -1,5 +1,6 @@
> >   // SPDX-License-Identifier: GPL-2.0
> >   #include <test_progs.h>
> > +#include <io_uring/mini_liburing.h>
> >   #include "cgroup_helpers.h"
> >   static char bpf_log_buf[4096];
> > @@ -38,6 +39,7 @@ static struct sockopt_test {
> >   	socklen_t			get_optlen_ret;
> >   	enum sockopt_test_error		error;
> > +	bool				io_uring_support;
> >   } tests[] = {
> >   	/* ==================== getsockopt ====================  */
> > @@ -53,6 +55,7 @@ static struct sockopt_test {
> >   		.attach_type = BPF_CGROUP_GETSOCKOPT,
> >   		.expected_attach_type = 0,
> >   		.error = DENY_LOAD,
> > +		.io_uring_support = true,
> 
> DENY_LOAD probably won't be an intersting test. The set/getsockopt won't be called.

Yea, I will remove all the DENY_LOAD and DENY_ATTACH tests.

> The existing test does not seem to have SOL_SOCKET for getsockopt also.

I am planning to move two tests to use SOL_SOCKET so we can also
exercise the io_uring tests. This is what I have in mind right now:

 * getsockopt: read ctx->optlen
 * getsockopt: support smaller ctx->optlen

> > -static int run_test(int cgroup_fd, struct sockopt_test *test)
> > +/* Core function that handles io_uring ring initialization,
> > + * sending SQE with sockopt command and waiting for the CQE.
> > + */
> > +static int uring_sockopt(int op, int fd, int level, int optname,
> > +			 const void *optval, socklen_t optlen)
> > +{
> > +	struct io_uring_cqe *cqe;
> > +	struct io_uring_sqe *sqe;
> > +	struct io_uring ring;
> > +	int err;
> > +
> > +	err = io_uring_queue_init(1, &ring, 0);
> > +	if (err) {
> > +		log_err("Failed to initialize io_uring ring");
> > +		return err;
> > +	}
> > +
> > +	sqe = io_uring_get_sqe(&ring);
> > +	if (!sqe) {
> > +		log_err("Failed to get an SQE");
> > +		return -1;
> 
> No need to io_uring_queue_exit() on the error path?

Good idea. updating it.

> > +	}
> > +
> > +	io_uring_prep_cmd(sqe, op, fd, level, optname, optval, optlen);
> > +
> > +	err = io_uring_submit(&ring);
> > +	if (err != 1) {
> > +		log_err("Failed to submit SQE");
> 
> Use ASSERT_* instead.
> 
> Regarding how to land this set,
> it will be useful to have the selftest running in the bpf CI. While there is
> iouring changes, some of the changes is in bpf and/or netdev also. eg. Patch
> 3 already has a conflict with the net-next and bpf-next tree because of a
> recent commit in socket.c on Aug 9.
> 
> May be Alexi and Daniel can advise how was similar patch managed before ?
> 
>
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 538df8fb8c42..4da04242b848 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -362,6 +362,7 @@  CLANG_CFLAGS = $(CLANG_SYS_INCLUDES) \
 
 $(OUTPUT)/test_l4lb_noinline.o: BPF_CFLAGS += -fno-inline
 $(OUTPUT)/test_xdp_noinline.o: BPF_CFLAGS += -fno-inline
+$(OUTPUT)/test_progs.o: CFLAGS += -I../../../include/
 
 $(OUTPUT)/flow_dissector_load.o: flow_dissector_load.h
 $(OUTPUT)/cgroup_getset_retval_hooks.o: cgroup_getset_retval_hooks.h
diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt.c b/tools/testing/selftests/bpf/prog_tests/sockopt.c
index 9e6a5e3ed4de..4693ad8bfe8f 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockopt.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockopt.c
@@ -1,5 +1,6 @@ 
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
+#include <io_uring/mini_liburing.h>
 #include "cgroup_helpers.h"
 
 static char bpf_log_buf[4096];
@@ -38,6 +39,7 @@  static struct sockopt_test {
 	socklen_t			get_optlen_ret;
 
 	enum sockopt_test_error		error;
+	bool				io_uring_support;
 } tests[] = {
 
 	/* ==================== getsockopt ====================  */
@@ -53,6 +55,7 @@  static struct sockopt_test {
 		.attach_type = BPF_CGROUP_GETSOCKOPT,
 		.expected_attach_type = 0,
 		.error = DENY_LOAD,
+		.io_uring_support = true,
 	},
 	{
 		.descr = "getsockopt: wrong expected_attach_type",
@@ -65,6 +68,7 @@  static struct sockopt_test {
 		.attach_type = BPF_CGROUP_GETSOCKOPT,
 		.expected_attach_type = BPF_CGROUP_SETSOCKOPT,
 		.error = DENY_ATTACH,
+		.io_uring_support = true,
 	},
 	{
 		.descr = "getsockopt: bypass bpf hook",
@@ -121,6 +125,7 @@  static struct sockopt_test {
 		.attach_type = BPF_CGROUP_GETSOCKOPT,
 		.expected_attach_type = BPF_CGROUP_GETSOCKOPT,
 		.error = DENY_LOAD,
+		.io_uring_support = true,
 	},
 	{
 		.descr = "getsockopt: read ctx->level",
@@ -164,6 +169,7 @@  static struct sockopt_test {
 		.expected_attach_type = BPF_CGROUP_GETSOCKOPT,
 
 		.error = DENY_LOAD,
+		.io_uring_support = true,
 	},
 	{
 		.descr = "getsockopt: read ctx->optname",
@@ -225,6 +231,7 @@  static struct sockopt_test {
 		.expected_attach_type = BPF_CGROUP_GETSOCKOPT,
 
 		.error = DENY_LOAD,
+		.io_uring_support = true,
 	},
 	{
 		.descr = "getsockopt: read ctx->optlen",
@@ -252,6 +259,7 @@  static struct sockopt_test {
 		.expected_attach_type = BPF_CGROUP_GETSOCKOPT,
 
 		.get_optlen = 64,
+		.io_uring_support = true,
 	},
 	{
 		.descr = "getsockopt: deny bigger ctx->optlen",
@@ -276,6 +284,7 @@  static struct sockopt_test {
 		.get_optlen = 64,
 
 		.error = EFAULT_GETSOCKOPT,
+		.io_uring_support = true,
 	},
 	{
 		.descr = "getsockopt: ignore >PAGE_SIZE optlen",
@@ -318,6 +327,7 @@  static struct sockopt_test {
 		.get_optval = {}, /* the changes are ignored */
 		.get_optlen = PAGE_SIZE + 1,
 		.error = EOPNOTSUPP_GETSOCKOPT,
+		.io_uring_support = true,
 	},
 	{
 		.descr = "getsockopt: support smaller ctx->optlen",
@@ -339,6 +349,7 @@  static struct sockopt_test {
 
 		.get_optlen = 64,
 		.get_optlen_ret = 32,
+		.io_uring_support = true,
 	},
 	{
 		.descr = "getsockopt: deny writing to ctx->optval",
@@ -353,6 +364,7 @@  static struct sockopt_test {
 		.expected_attach_type = BPF_CGROUP_GETSOCKOPT,
 
 		.error = DENY_LOAD,
+		.io_uring_support = true,
 	},
 	{
 		.descr = "getsockopt: deny writing to ctx->optval_end",
@@ -367,6 +379,7 @@  static struct sockopt_test {
 		.expected_attach_type = BPF_CGROUP_GETSOCKOPT,
 
 		.error = DENY_LOAD,
+		.io_uring_support = true,
 	},
 	{
 		.descr = "getsockopt: rewrite value",
@@ -421,6 +434,7 @@  static struct sockopt_test {
 		.attach_type = BPF_CGROUP_SETSOCKOPT,
 		.expected_attach_type = 0,
 		.error = DENY_LOAD,
+		.io_uring_support = true,
 	},
 	{
 		.descr = "setsockopt: wrong expected_attach_type",
@@ -433,6 +447,7 @@  static struct sockopt_test {
 		.attach_type = BPF_CGROUP_SETSOCKOPT,
 		.expected_attach_type = BPF_CGROUP_GETSOCKOPT,
 		.error = DENY_ATTACH,
+		.io_uring_support = true,
 	},
 	{
 		.descr = "setsockopt: bypass bpf hook",
@@ -518,6 +533,7 @@  static struct sockopt_test {
 		.set_level = 123,
 
 		.set_optlen = 1,
+		.io_uring_support = true,
 	},
 	{
 		.descr = "setsockopt: allow changing ctx->level",
@@ -572,6 +588,7 @@  static struct sockopt_test {
 		.set_optname = 123,
 
 		.set_optlen = 1,
+		.io_uring_support = true,
 	},
 	{
 		.descr = "setsockopt: allow changing ctx->optname",
@@ -624,6 +641,7 @@  static struct sockopt_test {
 		.expected_attach_type = BPF_CGROUP_SETSOCKOPT,
 
 		.set_optlen = 64,
+		.io_uring_support = true,
 	},
 	{
 		.descr = "setsockopt: ctx->optlen == -1 is ok",
@@ -640,6 +658,7 @@  static struct sockopt_test {
 		.expected_attach_type = BPF_CGROUP_SETSOCKOPT,
 
 		.set_optlen = 64,
+		.io_uring_support = true,
 	},
 	{
 		.descr = "setsockopt: deny ctx->optlen < 0 (except -1)",
@@ -658,6 +677,7 @@  static struct sockopt_test {
 		.set_optlen = 4,
 
 		.error = EFAULT_SETSOCKOPT,
+		.io_uring_support = true,
 	},
 	{
 		.descr = "setsockopt: deny ctx->optlen > input optlen",
@@ -675,6 +695,7 @@  static struct sockopt_test {
 		.set_optlen = 64,
 
 		.error = EFAULT_SETSOCKOPT,
+		.io_uring_support = true,
 	},
 	{
 		.descr = "setsockopt: ignore >PAGE_SIZE optlen",
@@ -775,6 +796,7 @@  static struct sockopt_test {
 		.expected_attach_type = BPF_CGROUP_SETSOCKOPT,
 
 		.error = DENY_LOAD,
+		.io_uring_support = true,
 	},
 	{
 		.descr = "setsockopt: deny read ctx->retval",
@@ -791,6 +813,7 @@  static struct sockopt_test {
 		.expected_attach_type = BPF_CGROUP_SETSOCKOPT,
 
 		.error = DENY_LOAD,
+		.io_uring_support = true,
 	},
 	{
 		.descr = "setsockopt: deny writing to ctx->optval",
@@ -805,6 +828,7 @@  static struct sockopt_test {
 		.expected_attach_type = BPF_CGROUP_SETSOCKOPT,
 
 		.error = DENY_LOAD,
+		.io_uring_support = true,
 	},
 	{
 		.descr = "setsockopt: deny writing to ctx->optval_end",
@@ -819,6 +843,7 @@  static struct sockopt_test {
 		.expected_attach_type = BPF_CGROUP_SETSOCKOPT,
 
 		.error = DENY_LOAD,
+		.io_uring_support = true,
 	},
 	{
 		.descr = "setsockopt: allow IP_TOS <= 128",
@@ -940,7 +965,94 @@  static int load_prog(const struct bpf_insn *insns,
 	return fd;
 }
 
-static int run_test(int cgroup_fd, struct sockopt_test *test)
+/* Core function that handles io_uring ring initialization,
+ * sending SQE with sockopt command and waiting for the CQE.
+ */
+static int uring_sockopt(int op, int fd, int level, int optname,
+			 const void *optval, socklen_t optlen)
+{
+	struct io_uring_cqe *cqe;
+	struct io_uring_sqe *sqe;
+	struct io_uring ring;
+	int err;
+
+	err = io_uring_queue_init(1, &ring, 0);
+	if (err) {
+		log_err("Failed to initialize io_uring ring");
+		return err;
+	}
+
+	sqe = io_uring_get_sqe(&ring);
+	if (!sqe) {
+		log_err("Failed to get an SQE");
+		return -1;
+	}
+
+	io_uring_prep_cmd(sqe, op, fd, level, optname, optval, optlen);
+
+	err = io_uring_submit(&ring);
+	if (err != 1) {
+		log_err("Failed to submit SQE");
+		return err;
+	}
+
+	err = io_uring_wait_cqe(&ring, &cqe);
+	if (err) {
+		log_err("Failed to wait for CQE");
+		return err;
+	}
+
+	err = cqe->res;
+
+	io_uring_queue_exit(&ring);
+
+	return err;
+}
+
+static int uring_setsockopt(int fd, int level, int optname, const void *optval,
+			    socklen_t optlen)
+{
+	return uring_sockopt(SOCKET_URING_OP_SETSOCKOPT, fd, level, optname,
+			     optval, optlen);
+}
+
+static int uring_getsockopt(int fd, int level, int optname, void *optval,
+			    socklen_t *optlen)
+{
+	int ret = uring_sockopt(SOCKET_URING_OP_GETSOCKOPT, fd, level, optname,
+				optval, *optlen);
+	if (ret < 0)
+		return ret;
+
+	/* Populate optlen back to be compatible with systemcall interface,
+	 * and simplify the test.
+	 */
+	*optlen = ret;
+
+	return 0;
+}
+
+/* Execute the setsocktopt operation */
+static int call_setsockopt(bool use_io_uring, int fd, int level, int optname,
+			   const void *optval, socklen_t optlen)
+{
+	if (use_io_uring)
+		return uring_setsockopt(fd, level, optname, optval, optlen);
+
+	return setsockopt(fd, level, optname, optval, optlen);
+}
+
+/* Execute the getsocktopt operation */
+static int call_getsockopt(bool use_io_uring, int fd, int level, int optname,
+			   void *optval, socklen_t *optlen)
+{
+	if (use_io_uring)
+		return uring_getsockopt(fd, level, optname, optval, optlen);
+
+	return getsockopt(fd, level, optname, optval, optlen);
+}
+
+static int run_test(int cgroup_fd, struct sockopt_test *test, bool use_io_uring)
 {
 	int sock_fd, err, prog_fd;
 	void *optval = NULL;
@@ -980,8 +1092,9 @@  static int run_test(int cgroup_fd, struct sockopt_test *test)
 			test->set_optlen = num_pages * sysconf(_SC_PAGESIZE) + remainder;
 		}
 
-		err = setsockopt(sock_fd, test->set_level, test->set_optname,
-				 test->set_optval, test->set_optlen);
+		err = call_setsockopt(use_io_uring, sock_fd, test->set_level,
+				      test->set_optname, test->set_optval,
+				      test->set_optlen);
 		if (err) {
 			if (errno == EPERM && test->error == EPERM_SETSOCKOPT)
 				goto close_sock_fd;
@@ -1008,8 +1121,8 @@  static int run_test(int cgroup_fd, struct sockopt_test *test)
 		socklen_t expected_get_optlen = test->get_optlen_ret ?:
 			test->get_optlen;
 
-		err = getsockopt(sock_fd, test->get_level, test->get_optname,
-				 optval, &optlen);
+		err = call_getsockopt(use_io_uring, sock_fd, test->get_level,
+				      test->get_optname, optval, &optlen);
 		if (err) {
 			if (errno == EOPNOTSUPP && test->error == EOPNOTSUPP_GETSOCKOPT)
 				goto free_optval;
@@ -1063,7 +1176,11 @@  void test_sockopt(void)
 		if (!test__start_subtest(tests[i].descr))
 			continue;
 
-		ASSERT_OK(run_test(cgroup_fd, &tests[i]), tests[i].descr);
+		ASSERT_OK(run_test(cgroup_fd, &tests[i], false),
+			  tests[i].descr);
+		if (tests[i].io_uring_support)
+			ASSERT_OK(run_test(cgroup_fd, &tests[i], true),
+				  tests[i].descr);
 	}
 
 	close(cgroup_fd);