linux-kernel - Re: [PATCH] io_uring: pass struct io_tw

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] [day] [month] [year] [list]

Message-ID: <bdff3648-72cc-4dd1-95cf-f4a9bedbdb12@gmail.com>
Date: Thu, 13 Feb 2025 16:54:05 +0000
From: Pavel Begunkov <asml.silence@...il.com>
To: Caleb Sander Mateos <csander@...estorage.com>,
 Jens Axboe <axboe@...nel.dk>
Cc: io-uring@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] io_uring: pass struct io_tw_state by value

On 2/11/25 21:45, Caleb Sander Mateos wrote:
> 8e5b3b89ecaf ("io_uring: remove struct io_tw_state::locked") removed the
> only field of io_tw_state but kept it as a task work callback argument
> to "forc[e] users not to invoke them carelessly out of a wrong context".

The other unnamed reason was to limit the mess from io_uring wide
changes while it might still be used later on. It's always a pain
for backporting, so if we're doing that we might as well make it
easily convertible for future plans. Let's do

typedef struct io_tw_state iou_state_t;

Might be useful for leaking it to cmds as well.

> Passing the struct io_tw_state * argument adds a few instructions to all
> callers that can't inline the functions and see the argument is unused.
> 
> So pass struct io_tw_state by value instead. Since it's a 0-sized value,
> it can be passed without any instructions needed to initialize it.
> 
> Also add a comment to struct io_tw_state to explain its purpose.
> 
> Signed-off-by: Caleb Sander Mateos <csander@...estorage.com>
> ---
>   include/linux/io_uring_types.h |  3 ++-
>   io_uring/futex.c               |  6 +++---
>   io_uring/io_uring.c            | 30 +++++++++++++++---------------
>   io_uring/io_uring.h            |  8 ++++----
>   io_uring/msg_ring.c            |  2 +-
>   io_uring/notif.c               |  2 +-
>   io_uring/poll.c                |  4 ++--
>   io_uring/poll.h                |  2 +-
>   io_uring/rw.c                  |  2 +-
>   io_uring/rw.h                  |  2 +-
>   io_uring/timeout.c             |  6 +++---
>   io_uring/uring_cmd.c           |  2 +-
>   io_uring/waitid.c              |  6 +++---
>   13 files changed, 38 insertions(+), 37 deletions(-)
> 
> diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
> index e2fef264ff8b..4abd0299fdfb 100644
> --- a/include/linux/io_uring_types.h
> +++ b/include/linux/io_uring_types.h
> @@ -434,10 +434,11 @@ struct io_ring_ctx {
>   	struct io_mapped_region		ring_region;
>   	/* used for optimised request parameter and wait argument passing  */
>   	struct io_mapped_region		param_region;
>   };
>   
> +/* Token passed to task work callbacks to indicate ctx->uring_lock is held */

It indicates the current context not locking specifically, which is
why the chunk from waitid as below is not correct:

mutex_lock(uring_lock);
struct io_tw_state dummy;
io_uring_core_helper(&dummy);
mutex_unlock(uring_lock);


Probably something like "Token passed around to indicate the current
io_uring execution context". And it doesn't need to be more specific
as long as type system works for us. It might be a good idea to add
a note in bold that nobody outside of core io_uring is allowed
creating it, though it'll be ignored anyway.

>   struct io_tw_state {
>   };


-- 
Pavel Begunkov