netdev - Re: [PATCH v3 9/9] selftests/bpf/sockopt: Add io

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

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

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

Message-ID: <ZOi3aeq43hVb1Ner@gmail.com>
Date: Fri, 25 Aug 2023 07:15:05 -0700
From: Breno Leitao <leitao@...ian.org>
To: Martin KaFai Lau <martin.lau@...ux.dev>
Cc: Alexei Starovoitov <ast@...nel.org>,
	Daniel Borkmann <daniel@...earbox.net>, bpf@...r.kernel.org,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	io-uring@...r.kernel.org, kuba@...nel.org, pabeni@...hat.com,
	krisman@...e.de, Wang Yufen <wangyufen@...wei.com>,
	Daniel Müller <deso@...teo.net>,
	"open list:KERNEL SELFTEST FRAMEWORK" <linux-kselftest@...r.kernel.org>,
	sdf@...gle.com, axboe@...nel.dk, asml.silence@...il.com,
	willemdebruijn.kernel@...il.com,
	Andrii Nakryiko <andrii@...nel.org>,
	Mykola Lysenko <mykolal@...com>, Song Liu <song@...nel.org>,
	Yonghong Song <yhs@...com>,
	John Fastabend <john.fastabend@...il.com>,
	KP Singh <kpsingh@...nel.org>, Hao Luo <haoluo@...gle.com>,
	Jiri Olsa <jolsa@...nel.org>, Shuah Khan <shuah@...nel.org>
Subject: Re: [PATCH v3 9/9] selftests/bpf/sockopt: Add io_uring support

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 ?
> 
>