On 27/05/2020 21:04, Jann Horn wrote:
> On Wed, May 27, 2020 at 12:14 PM Pavel Begunkov <asml.silence@xxxxxxxxx> wrote:
>> On 27/05/2020 01:04, Jann Horn wrote:
>>> On Tue, May 26, 2020 at 8:11 PM Pavel Begunkov <asml.silence@xxxxxxxxx> wrote:
>>>> It looks like taking ->uring_lock should work like kind of grace
>>>> period for struct files_struct and io_uring_flush(), and that would
>>>> solve the race with "fcheck(ctx->ring_fd) == ctx->ring_file".
>>>>
>>>> Can you take a look? If you like it, I'll send a proper patch
>>>> and a bunch of cleanups on top.
>>>>
>>>>
>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>> index a3dbd5f40391..012af200dc72 100644
>>>> --- a/fs/io_uring.c
>>>> +++ b/fs/io_uring.c
>>>> @@ -5557,12 +5557,11 @@ static int io_grab_files(struct io_kiocb *req)
>>>> * the fd has changed since we started down this path, and disallow
>>>> * this operation if it has.
>>>> */
>>>> - if (fcheck(ctx->ring_fd) == ctx->ring_file) {
>>>> - list_add(&req->inflight_entry, &ctx->inflight_list);
>>>> - req->flags |= REQ_F_INFLIGHT;
>>>> - req->work.files = current->files;
>>>> - ret = 0;
>>>> - }
>>>> + list_add(&req->inflight_entry, &ctx->inflight_list);
>>>> + req->flags |= REQ_F_INFLIGHT;
>>>> + req->work.files = current->files;
>>>> + ret = 0;
>>>> +
>>>> spin_unlock_irq(&ctx->inflight_lock);
>>>> rcu_read_unlock();
>>>>
>>>> @@ -7479,6 +7478,10 @@ static int io_uring_release(struct inode *inode, struct
>>>> file *file)
>>>> static void io_uring_cancel_files(struct io_ring_ctx *ctx,
>>>> struct files_struct *files)
>>>> {
>>>> + /* wait all submitters that can race for @files */
>>>> + mutex_lock(&ctx->uring_lock);
>>>> + mutex_unlock(&ctx->uring_lock);
>>>> +
>>>> while (!list_empty_careful(&ctx->inflight_list)) {
>>>> struct io_kiocb *cancel_req = NULL, *req;
>>>> DEFINE_WAIT(wait);
>>>
>>> First off: You're removing a check in io_grab_files() without changing
>>> the comment that describes the check; and the new comment you're
>>> adding in io_uring_cancel_files() is IMO too short to be useful.
>>
>> Obviously, it was stripped down to show the idea, nobody is talking about
>> commiting it as is. I hoped Jens remembers it well enough to understand.
>> Let me describe it in more details then:
>>
>>>
>>> I'm trying to figure out how your change is supposed to work, and I
>>> don't get it. If a submitter is just past fdget() (at which point no
>>> locks are held), the ->flush() caller can instantly take and drop the
>>> ->uring_lock, and then later the rest of the submission path will grab
>>> an unprotected pointer to the files_struct. Am I missing something?
>>
>> old = tsk->files;
>> task_lock(tsk);
>> tsk->files = files;
>> task_unlock(tsk);
>> put_files_struct(old); (i.e. ->flush(old))
>>
>> It's from reset_files_struct(), and I presume the whole idea of
>> io_uring->flush() is to protect against racing for similarly going away @old
>> files. I.e. ensuring of not having io_uring requests holding @old files.
>
> Kind of. We use the ->flush() handler to be notified when the
> files_struct goes away, so that instead of holding a reference to the
> files_struct (which would cause a reference loop), we can clean up our
> references when it goes away.
>
>> The only place, where current->files are accessed and copied by io_uring, is
>> io_grab_files(), which is called in the submission path. And the whole
>> submission path is done under @uring_mtx.
>
> No it isn't. We do fdget(fd) at the start of the io_uring_enter
> syscall, and at that point we obviously can't hold the uring_mtx yet.
__directly__ accessing ->files, or hand-coded. fdget() by itself shouldn't be a
problem.
>
>> For your case, the submitter will take @uring_mtx only after this lock/unlock
>> happened, so it won't see old files (happens-before by locking mutex).
>
> No, it will see the old files. The concurrent operation we're worried
>From what you wrote below, it will see the old files just because nobody would
try to replace them. Is that it?
> about is not that the files_struct goes away somehow (that can't
> happen); what we want to guard against is a concurrent close() or
> dup2() or so removing the uring fd from the files_struct, because if
> someone calls close() before we stash a pointer to current->files,
> that pointer isn't protected anymore.
Let me guess, you mean maintenance of ->files itself, such as expand_files()?
>
>
>> The thing I don't know is why current->files is originally accessed without
>> protection in io_grab_files(), but presumably rcu_read_lock() there is for that
>> reason.
>
> No, it's because current->files can never change under you; pretty
> much the only places where current->files can change are unshare() and
> execve().
I see, good to know
--
Pavel Begunkov
