Re: [PATCHSET 0/4] Add support for shared io-wq backends

[Pavel Begunkov <asml.silence@gmail.com>]

On 1/28/2020 1:01 PM, Stefan Metzmacher wrote:
>>>>>>> OK, so how about this:
>>>>>>>
>>>>>>> - We use the 'fd' as the lookup key. This makes it easy since we can
>>>>>>>   just check if it's a io_uring instance or not, we don't need to do any
>>>>>>>   tracking on the side. It also means that the application asking for
>>>>>>>   sharing must already have some relationship to the process that
>>>>>>>   created the ring.
>>>>>>>
>>>>>>> - mm/creds must be transferred through the work item. Any SQE done on
>>>>>>>   behalf of io_uring_enter() directly already has that, if punted we
>>>>>>>   must pass the creds and mm. This means we break the static setup of
>>>>>>>   io_wq->mm/creds. It also means that we probably have to add that to
>>>>>>>   io_wq_work, which kind of sucks, but...
>>>>>> It'd fix Stefan's worry too.
>>>>>>
>>>>>>> I think with that we have a decent setup, that's also safe. I've dropped
>>>>>>> the sharing patches for now, from the 5.6 tree.
>>>>>>
>>>>>> So one concern might be SQPOLL, it'll have to use the ctx creds and mm
>>>>>> as usual. I guess that is ok.
>>
>> https://git.kernel.dk/cgit/linux-block/log/?h=for-5.6/io_uring-vfs-wq
>>
>> Top patch there is the mm/creds passing. I kind of like it even if it
>> means we're growing io_wq_worker (and subsequently io_kiocb) by 16
>> bytes, as it means we can be more flexible. This solves it for this use
>> case, but also the case that Stefan was worried about.
> 
> Ok, that means that ctx->creds is only used in the IORING_SETUP_SQPOLL
> case and there it's used for all requests as get_current_cred() is the
> same as ctx->creds from within io_sq_thread(), correct?

Right

> And in all other cases get_current_cred() is used at io_uring_enter() time.

Exactly

> That's good in order to make the behavior consistent again and prevents
> potential random security problems.

BTW, there also can be problems with registered resources. E.g. for
buffers io_uring can get_user_pages() of one process, and then use the
pages from another process by passing a buffer index. This is not as
bad, however.

> 
> BTW: you need to revert/drop 44d282796f81eb1debc1d7cb53245b4cb3214cb5
> in that branch. Or just rebase on v5.5 final?
> 
> Thanks!
> metze
> 

-- 
Pavel Begunkov