IORING_SETUP_ATTACH_WQ (was Re: [PATCH 1/3] io_uring: fix invalid ctx->sq_thread_idle)

[Stefan Metzmacher <metze@samba.org>]

Am 11.03.21 um 12:18 schrieb Pavel Begunkov:
> On 10/03/2021 13:56, Stefan Metzmacher wrote:
>>
>> Hi Pavel,
>>
>> I wondered about the exact same change this morning, while researching
>> the IORING_SETUP_ATTACH_WQ behavior :-)
>>
>> It still seems to me that IORING_SETUP_ATTACH_WQ changed over time.
>> As you introduced that flag, can you summaries it's behavior (and changes)
>> over time (over the releases).
> 
> Not sure I remember the story in details, but from the beginning it was
> for io-wq sharing only, then it had expanded to SQPOLL as well. Now it's
> only about SQPOLL sharing, because of the recent io-wq changes that made
> it per-task and shared by default.
> 
> In all cases it should be checking the passed in file, that should retain
> the old behaviour of failing setup if the flag is set but wq_fd is not valid.

Thanks, that's what I also found so far, see below for more findings.

>>
>> I'm wondering if ctx->sq_creds is really the only thing we need to take care of.
> 
> io-wq is not affected by IORING_SETUP_ATTACH_WQ. It's per-task and mimics
> all the resources of the creator (on the moment of io-wq creation). Off
> ATTACH_WQ topic, but that's almost matches what it has been before, and
> with dropped unshare bit, should be totally same.
> 
> Regarding SQPOLL, it was always using resources of the first task, so
> those are just reaped of from it, and not only some particular like
> mm/files but all of them, like fork does, so should be safer.
> 
> Creds are just a special case because of that personality stuff, at least
> if we add back iowq unshare handling.
> 
>>
>> Do we know about existing users of IORING_SETUP_ATTACH_WQ and their use case?
> 
> Have no clue.
> 
>> As mm, files and other things may differ now between sqe producer and the sq_thread.
> 
> It was always using mm/files of the ctx creator's task, aka ctx->sqo_task,
> but right, for the sharing case those may be different b/w ctx, so looks
> like a regression to me

Good. I'll try to explore a possible way out below.

Ok, I'm continuing the thread here (just pasting the mail I already started to write :-)

I did some more research regarding IORING_SETUP_ATTACH_WQ in 5.12.

The current logic in io_sq_offload_create() is this:

+       /* Retain compatibility with failing for an invalid attach attempt */
+       if ((ctx->flags & (IORING_SETUP_ATTACH_WQ | IORING_SETUP_SQPOLL)) ==
+                               IORING_SETUP_ATTACH_WQ) {
+               struct fd f;
+
+               f = fdget(p->wq_fd);
+               if (!f.file)
+                       return -ENXIO;
+               if (f.file->f_op != &io_uring_fops) {
+                       fdput(f);
+                       return -EINVAL;
+               }
+               fdput(f);
+       }

That means that IORING_SETUP_ATTACH_WQ (without IORING_SETUP_SQPOLL) is completely
ignored (except that we still simulate the -ENXIO and -EINVAL  cases), correct?
(You already agreed on that above :-)

The reason for this is that io_wq is no longer maintained per io_ring_ctx,
but instead it is now global per io_uring_task.
Which means each userspace thread (or the sq_thread) has its own io_uring_task and
thus its own io_wq.

Regarding the IORING_SETUP_SQPOLL|IORING_SETUP_ATTACH_WQ case we still allow attaching
to the sq_thread of a different io_ring_ctx. The sq_thread runs in the context of
the io_uring_setup() syscall that created it. We used to switch current->mm, current->files
and other things before calling __io_sq_thread() before, but we no longer do that.
And this seems to be security problem to me, as it's now possible for the attached
io_ring_ctx to start sqe's copying the whole address space of the donator into
a registered fixed file of the attached process.

As we already ignore IORING_SETUP_ATTACH_WQ without IORING_SETUP_SQPOLL, what about
ignoring it as well if the attaching task uses different ->mm, ->files, ...
So IORING_SETUP_ATTACH_WQ would only have any effect if the task calling io_uring_setup()
runs in the same context (except of the creds) as the existing sq_thread, which means it would work
if multiple userspace threads of the same userspace process want to share the sq_thread and its
io_wq. Everything else would be stupid (similar to the unshare() cases).
But as this has worked before, we just silently ignore IORING_SETUP_ATTACH_WQ is
we find a context mismatch and let io_uring_setup() silently create a new sq_thread.

What do you think?

metze