Am 11.03.21 um 12:46 schrieb Stefan Metzmacher:
>
> 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.
Or we completely ignore IORING_SETUP_ATTACH_WQ (execpt the error cases).
Then we can implement a new IORING_SETUP_ATTACH_SQ with new semantics,
that the existing sq_thread will be used as it and both sides now what it means to them.
We also add a new IORING_REGISTER_RESTRICTIONS/IORING_RESTRICTION_ALLOW_SQ_ATTACHMENTS
which prepares the first io_ring_ctx to allow others to attach.
Would that make sense?
metze
