| Message ID | 20230911103407.1393149-1-leitao@debian.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | io_uring: Initial support for {s,g}etsockopt commands | expand |
Breno Leitao <leitao@debian.org> writes: > Create a new flag to track if the operation is running compat mode. > This basically check the context->compat and pass it to the issue_flags, > so, it could be queried later in the callbacks. > > Signed-off-by: Breno Leitao <leitao@debian.org> Reviewed-by: Gabriel Krisman Bertazi <krisman@suse.de> > --- > include/linux/io_uring.h | 1 + > io_uring/uring_cmd.c | 2 ++ > 2 files changed, 3 insertions(+) > > diff --git a/include/linux/io_uring.h b/include/linux/io_uring.h > index 106cdc55ff3b..bc53b35966ed 100644 > --- a/include/linux/io_uring.h > +++ b/include/linux/io_uring.h > @@ -20,6 +20,7 @@ enum io_uring_cmd_flags { > IO_URING_F_SQE128 = (1 << 8), > IO_URING_F_CQE32 = (1 << 9), > IO_URING_F_IOPOLL = (1 << 10), > + IO_URING_F_COMPAT = (1 << 11), > }; > > struct io_uring_cmd { > diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c > index 537795fddc87..60f843a357e0 100644 > --- a/io_uring/uring_cmd.c > +++ b/io_uring/uring_cmd.c > @@ -128,6 +128,8 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags) > issue_flags |= IO_URING_F_SQE128; > if (ctx->flags & IORING_SETUP_CQE32) > issue_flags |= IO_URING_F_CQE32; > + if (ctx->compat) > + issue_flags |= IO_URING_F_COMPAT; > if (ctx->flags & IORING_SETUP_IOPOLL) { > if (!file->f_op->uring_cmd_iopoll) > return -EOPNOTSUPP;
Breno Leitao <leitao@debian.org> writes: > Protect io_uring_cmd_sock() to be called if CONFIG_NET is not set. If > network is not enabled, but io_uring is, then we want to return > -EOPNOTSUPP for any possible socket operation. > > This is helpful because io_uring_cmd_sock() can now call functions that > only exits if CONFIG_NET is enabled without having #ifdef CONFIG_NET > inside the function itself. > > Signed-off-by: Breno Leitao <leitao@debian.org> > --- > io_uring/uring_cmd.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c > index 60f843a357e0..a7d6a7d112b7 100644 > --- a/io_uring/uring_cmd.c > +++ b/io_uring/uring_cmd.c > @@ -167,6 +167,7 @@ int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw, > } > EXPORT_SYMBOL_GPL(io_uring_cmd_import_fixed); > > +#if defined(CONFIG_NET) > int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) > { > struct socket *sock = cmd->file->private_data; > @@ -193,3 +194,10 @@ int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) > } > } > EXPORT_SYMBOL_GPL(io_uring_cmd_sock); > +#else > +int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) > +{ > + return -EOPNOTSUPP; > +} > +#endif > + Is net/socket.c even built without CONFIG_NET? if not, you don't even need the alternative EOPNOTSUPP implementation.
On Mon, Sep 11, 2023 at 11:53:58AM -0400, Gabriel Krisman Bertazi wrote: > Breno Leitao <leitao@debian.org> writes: > > > Protect io_uring_cmd_sock() to be called if CONFIG_NET is not set. If > > network is not enabled, but io_uring is, then we want to return > > -EOPNOTSUPP for any possible socket operation. > > > > This is helpful because io_uring_cmd_sock() can now call functions that > > only exits if CONFIG_NET is enabled without having #ifdef CONFIG_NET > > inside the function itself. > > > > Signed-off-by: Breno Leitao <leitao@debian.org> > > --- > > io_uring/uring_cmd.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c > > index 60f843a357e0..a7d6a7d112b7 100644 > > --- a/io_uring/uring_cmd.c > > +++ b/io_uring/uring_cmd.c > > @@ -167,6 +167,7 @@ int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw, > > } > > EXPORT_SYMBOL_GPL(io_uring_cmd_import_fixed); > > > > +#if defined(CONFIG_NET) > > int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) > > { > > struct socket *sock = cmd->file->private_data; > > @@ -193,3 +194,10 @@ int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) > > } > > } > > EXPORT_SYMBOL_GPL(io_uring_cmd_sock); > > +#else > > +int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) > > +{ > > + return -EOPNOTSUPP; > > +} > > +#endif > > + > > Is net/socket.c even built without CONFIG_NET? if not, you don't even need > the alternative EOPNOTSUPP implementation. It seems so. net/socket.o is part of obj-y: https://github.com/torvalds/linux/blob/master/net/Makefile#L9
Breno Leitao <leitao@debian.org> writes: > On Mon, Sep 11, 2023 at 11:53:58AM -0400, Gabriel Krisman Bertazi wrote: >> Breno Leitao <leitao@debian.org> writes: >> >> > Protect io_uring_cmd_sock() to be called if CONFIG_NET is not set. If >> > network is not enabled, but io_uring is, then we want to return >> > -EOPNOTSUPP for any possible socket operation. >> > >> > This is helpful because io_uring_cmd_sock() can now call functions that >> > only exits if CONFIG_NET is enabled without having #ifdef CONFIG_NET >> > inside the function itself. >> > >> > Signed-off-by: Breno Leitao <leitao@debian.org> >> > --- >> > io_uring/uring_cmd.c | 8 ++++++++ >> > 1 file changed, 8 insertions(+) >> > >> > diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c >> > index 60f843a357e0..a7d6a7d112b7 100644 >> > --- a/io_uring/uring_cmd.c >> > +++ b/io_uring/uring_cmd.c >> > @@ -167,6 +167,7 @@ int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw, >> > } >> > EXPORT_SYMBOL_GPL(io_uring_cmd_import_fixed); >> > >> > +#if defined(CONFIG_NET) >> > int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) >> > { >> > struct socket *sock = cmd->file->private_data; >> > @@ -193,3 +194,10 @@ int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) >> > } >> > } >> > EXPORT_SYMBOL_GPL(io_uring_cmd_sock); >> > +#else >> > +int io_uring_cmd_sock(struct io_uring_cmd *cmd, unsigned int issue_flags) >> > +{ >> > + return -EOPNOTSUPP; >> > +} >> > +#endif >> > + >> >> Is net/socket.c even built without CONFIG_NET? if not, you don't even need >> the alternative EOPNOTSUPP implementation. > > It seems so. net/socket.o is part of obj-y: > > https://github.com/torvalds/linux/blob/master/net/Makefile#L9 Yes. But also: [0:cartola linux]$ grep 'net/' Kbuild obj-$(CONFIG_NET) += net/ I doubled checked and it should build fine without it. Technically, you also want to also guard the declaration in the header file, IMO, even if it compiles fine. Also, there is an extra blank line warning when applying the patch but, surprisingly, checkpatch.pl seems to miss it.
This patchset adds support for getsockopt (SOCKET_URING_OP_GETSOCKOPT) and setsockopt (SOCKET_URING_OP_SETSOCKOPT) in io_uring commands. SOCKET_URING_OP_SETSOCKOPT and SOCKET_URING_OP_GETSOCKOPT implement generic case, covering all levels and optnames (a change from the previous version, where getsockopt was limited to level=SOL_SOCKET). In order to keep the implementation (and tests) simple, some refactors were done prior to the changes, as follows: Patch 1-2: Remove the core {s,g}etsockopt() core function from __sys_{g,s}etsockopt, so, the code could be reused by other callers, such as io_uring. Patch 3: Pass compat mode to the file/socket callbacks Patch 4: Move io_uring helpers from io_uring_zerocopy_tx to a generic io_uring headers. This simplify the test case (last patch) Patch 5: Protect io_uring_cmd_sock() to not be called if CONFIG_NET is disabled. Important to say that userspace pointers need to be alive until the operation is completed, as in the systemcall. These changes were tested with a new test[1] in liburing, LTP sockopt* tests, as also with bpf/progs/sockopt test case, which is now adapted to run using both system calls and io_uring commands. [1] Link: https://github.com/leitao/liburing/blob/getsock/test/socket-getsetsock-cmd.c RFC -> V1: * Copy user memory at io_uring subsystem, and call proto_ops callbacks using kernel memory * Implement all the cases for SOCKET_URING_OP_SETSOCKOPT V1 -> V2 * Implemented the BPF part * Using user pointers from optval to avoid kmalloc in io_uring part. V2 -> V3: * Break down __sys_setsockopt and reuse the core code, avoiding duplicated code. This removed the requirement to expose sock_use_custom_sol_socket(). * Added io_uring test to selftests/bpf/sockopt. * Fixed compat argument, by passing it to the issue_flags. V3 -> V4: * Rebase on top of commit 1ded5e5a5931b ("net: annotate data-races around sock->ops") * Also broke down __sys_setsockopt() to reuse the core function from io_uring. * Create a new patch to return -EOPNOTSUPP if CONFIG_NET is disabled * Added two SOL_SOCKET tests in bpf/prog_tests/sockopt. V4 -> V5: * Do not use sockptr anymore, by changing the optlen getsock argument to be a user pointer (instead of a kernel pointer). This change also drop the limitation on getsockopt from previous versions, and now all levels are supported. * Simplified the BPF sockopt test, since there is no more limitation on the io_uring commands. * No more changes in the BPF subsystem. * Moved the optlen field in the SQE struct. It is now a pointer instead of u32. Breno Leitao (8): net/socket: Break down __sys_setsockopt net/socket: Break down __sys_getsockopt io_uring/cmd: Pass compat mode in issue_flags selftests/net: Extract uring helpers to be reusable io_uring/cmd: return -EOPNOTSUPP if net is disabled io_uring/cmd: Introduce SOCKET_URING_OP_GETSOCKOPT io_uring/cmd: Introduce SOCKET_URING_OP_SETSOCKOPT selftests/bpf/sockopt: Add io_uring support include/linux/io_uring.h | 1 + include/net/sock.h | 5 + include/uapi/linux/io_uring.h | 10 + io_uring/uring_cmd.c | 41 +++ net/socket.c | 89 ++++-- tools/include/io_uring/mini_liburing.h | 292 ++++++++++++++++++ .../selftests/bpf/prog_tests/sockopt.c | 95 +++++- tools/testing/selftests/net/Makefile | 1 + .../selftests/net/io_uring_zerocopy_tx.c | 268 +--------------- 9 files changed, 497 insertions(+), 305 deletions(-) create mode 100644 tools/include/io_uring/mini_liburing.h