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
