From 766f5ffcfc9e511c6b26abb8a80e935359a5185f Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Thu, 10 Sep 2020 16:06:21 -0600 Subject: [PATCH 2/2] io_uring: implement ->flush() sequence to handle ->files validity The current scheme stashes away ->ring_fd and ->ring_file, and uses that to check against whether or not ->files could have changed. This works, but doesn't work so well for SQPOLL. If the application does close the ring_fd, then we require that applications enter the kernel to refresh our state. Add an atomic sequence for the ->flush() count on the ring fd, and if we get a mismatch between checking this sequence before and after grabbing the ->files, then we fail the request. This should offer the same protection that we currently have, with the added benefit of being able to update the ->files automatically. Signed-off-by: Jens Axboe --- fs/io_uring.c | 131 +++++++++++++++++++++++++++++--------------------- 1 file changed, 77 insertions(+), 54 deletions(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index 5d0247875237..bf994f195aaf 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -308,8 +308,11 @@ struct io_ring_ctx { */ struct fixed_file_data *file_data; unsigned nr_user_files; - int ring_fd; - struct file *ring_file; + + /* incremented when ->flush() is called */ + atomic_t files_seq; + /* assigned when ->files are grabbed */ + int cur_files_seq; /* if used, fixed mapped user buffers */ unsigned nr_user_bufs; @@ -394,6 +397,7 @@ struct io_close { struct file *file; struct file *put_file; int fd; + int files_seq; }; struct io_timeout_data { @@ -409,6 +413,7 @@ struct io_accept { int __user *addr_len; int flags; unsigned long nofile; + int files_seq; }; struct io_sync { @@ -461,6 +466,7 @@ struct io_sr_msg { struct io_open { struct file *file; int dfd; + int files_seq; struct filename *filename; struct open_how how; unsigned long nofile; @@ -471,6 +477,7 @@ struct io_files_update { u64 arg; u32 nr_args; u32 offset; + int files_seq; }; struct io_fadvise { @@ -492,6 +499,7 @@ struct io_epoll { int epfd; int op; int fd; + int files_seq; struct epoll_event event; }; @@ -518,6 +526,7 @@ struct io_statx { int dfd; unsigned int mask; unsigned int flags; + int files_seq; const char __user *filename; struct statx __user *buffer; }; @@ -3860,6 +3869,28 @@ static int io_provide_buffers(struct io_kiocb *req, bool force_nonblock, return 0; } +/* + * Check that our ->files sequence matches. If files isn't assigned yet, + * just store the current sequence. If they are assigned, check against + * the sequence from when they got assigned. If we get a mismatch, we fail + * the request. This is only applicable to requests that sets ->file_table + * in io_op_defs[], indicating that they need access to the file_struct + * when executed async. + */ +static int io_check_files_seq(struct io_kiocb *req, int *seq) +{ + struct io_ring_ctx *ctx = req->ctx; + + if (!req->work.files) { + *seq = atomic_read(&ctx->files_seq); + return 0; + } else if (*seq == ctx->cur_files_seq) { + return 0; + } + + return -EBADF; +} + static int io_epoll_ctl_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) { @@ -3881,6 +3912,7 @@ static int io_epoll_ctl_prep(struct io_kiocb *req, return -EFAULT; } + req->epoll.files_seq = req->ctx->cur_files_seq; return 0; #else return -EOPNOTSUPP; @@ -3894,10 +3926,15 @@ static int io_epoll_ctl(struct io_kiocb *req, bool force_nonblock, struct io_epoll *ie = &req->epoll; int ret; + ret = io_check_files_seq(req, &ie->files_seq); + if (ret) + goto done; + ret = do_epoll_ctl(ie->epfd, ie->op, ie->fd, &ie->event, force_nonblock); if (force_nonblock && ret == -EAGAIN) return -EAGAIN; +done: if (ret < 0) req_set_fail_links(req); __io_req_complete(req, ret, 0, cs); @@ -3993,6 +4030,7 @@ static int io_statx_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) req->statx.filename = u64_to_user_ptr(READ_ONCE(sqe->addr)); req->statx.buffer = u64_to_user_ptr(READ_ONCE(sqe->addr2)); req->statx.flags = READ_ONCE(sqe->statx_flags); + req->statx.files_seq = req->ctx->cur_files_seq; return 0; } @@ -4002,6 +4040,10 @@ static int io_statx(struct io_kiocb *req, bool force_nonblock) struct io_statx *ctx = &req->statx; int ret; + ret = io_check_files_seq(req, &ctx->files_seq); + if (ret) + goto done; + if (force_nonblock) { /* only need file table for an actual valid fd */ if (ctx->dfd == -1 || ctx->dfd == AT_FDCWD) @@ -4012,6 +4054,7 @@ static int io_statx(struct io_kiocb *req, bool force_nonblock) ret = do_statx(ctx->dfd, ctx->filename, ctx->flags, ctx->mask, ctx->buffer); +done: if (ret < 0) req_set_fail_links(req); io_req_complete(req, ret); @@ -4037,11 +4080,11 @@ static int io_close_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) return -EBADF; req->close.fd = READ_ONCE(sqe->fd); - if ((req->file && req->file->f_op == &io_uring_fops) || - req->close.fd == req->ctx->ring_fd) + if (req->file && req->file->f_op == &io_uring_fops) return -EBADF; req->close.put_file = NULL; + req->close.files_seq = req->ctx->cur_files_seq; return 0; } @@ -4051,6 +4094,10 @@ static int io_close(struct io_kiocb *req, bool force_nonblock, struct io_close *close = &req->close; int ret; + ret = io_check_files_seq(req, &close->files_seq); + if (ret) + goto done; + /* might be already done during nonblock submission */ if (!close->put_file) { ret = __close_fd_get_file(close->fd, &close->put_file); @@ -4069,10 +4116,11 @@ static int io_close(struct io_kiocb *req, bool force_nonblock, /* No ->flush() or already async, safely close from here */ ret = filp_close(close->put_file, req->work.files); - if (ret < 0) - req_set_fail_links(req); fput(close->put_file); close->put_file = NULL; +done: + if (ret < 0) + req_set_fail_links(req); __io_req_complete(req, ret, 0, cs); return 0; } @@ -4526,6 +4574,7 @@ static int io_accept_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) accept->addr_len = u64_to_user_ptr(READ_ONCE(sqe->addr2)); accept->flags = READ_ONCE(sqe->accept_flags); accept->nofile = rlimit(RLIMIT_NOFILE); + accept->files_seq = req->ctx->cur_files_seq; return 0; } @@ -4536,6 +4585,10 @@ static int io_accept(struct io_kiocb *req, bool force_nonblock, unsigned int file_flags = force_nonblock ? O_NONBLOCK : 0; int ret; + ret = io_check_files_seq(req, &accept->files_seq); + if (ret) + goto done; + if (req->file->f_flags & O_NONBLOCK) req->flags |= REQ_F_NOWAIT; @@ -4544,6 +4597,7 @@ static int io_accept(struct io_kiocb *req, bool force_nonblock, accept->nofile); if (ret == -EAGAIN && force_nonblock) return -EAGAIN; +done: if (ret < 0) { if (ret == -ERESTARTSYS) ret = -EINTR; @@ -5513,6 +5567,7 @@ static int io_files_update_prep(struct io_kiocb *req, if (!req->files_update.nr_args) return -EINVAL; req->files_update.arg = READ_ONCE(sqe->addr); + req->files_update.files_seq = req->ctx->cur_files_seq; return 0; } @@ -5523,6 +5578,10 @@ static int io_files_update(struct io_kiocb *req, bool force_nonblock, struct io_uring_files_update up; int ret; + ret = io_check_files_seq(req, &req->files_update.files_seq); + if (ret) + goto done; + if (force_nonblock) return -EAGAIN; @@ -5532,7 +5591,7 @@ static int io_files_update(struct io_kiocb *req, bool force_nonblock, mutex_lock(&ctx->uring_lock); ret = __io_sqe_files_update(ctx, &up, req->files_update.nr_args); mutex_unlock(&ctx->uring_lock); - +done: if (ret < 0) req_set_fail_links(req); __io_req_complete(req, ret, 0, cs); @@ -6118,34 +6177,21 @@ static int io_req_set_file(struct io_submit_state *state, struct io_kiocb *req, static int io_grab_files(struct io_kiocb *req) { - int ret = -EBADF; struct io_ring_ctx *ctx = req->ctx; io_req_init_async(req); if (req->work.files || (req->flags & REQ_F_NO_FILE_TABLE)) return 0; - if (!ctx->ring_file) - return -EBADF; - rcu_read_lock(); spin_lock_irq(&ctx->inflight_lock); - /* - * We use the f_ops->flush() handler to ensure that we can flush - * out work accessing these files if the fd is closed. Check if - * 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; + ctx->cur_files_seq = atomic_read(&ctx->files_seq); + req->work.files = current->files; spin_unlock_irq(&ctx->inflight_lock); - rcu_read_unlock(); - return ret; + return 0; } static inline int io_prep_work_files(struct io_kiocb *req) @@ -6705,14 +6751,7 @@ static enum sq_ret __io_sq_thread(struct io_ring_ctx *ctx, mutex_unlock(&ctx->uring_lock); } - /* - * If ->ring_file is NULL, we're waiting on new fd/file assigment. - * Don't submit anything new until that happens. - */ - if (ctx->ring_file) - to_submit = io_sqring_entries(ctx); - else - to_submit = 0; + to_submit = io_sqring_entries(ctx); /* * If submit got -EBUSY, flag us as needing the application @@ -6756,7 +6795,7 @@ static enum sq_ret __io_sq_thread(struct io_ring_ctx *ctx, } to_submit = io_sqring_entries(ctx); - if (!to_submit || ret == -EBUSY || !ctx->ring_file) + if (!to_submit || ret == -EBUSY) return SQT_IDLE; finish_wait(&sqd->wait, &ctx->sqo_wait_entry); @@ -8547,6 +8586,9 @@ static int io_uring_flush(struct file *file, void *data) { struct io_ring_ctx *ctx = file->private_data; + /* assume current files sequence is no longer valid */ + atomic_inc(&ctx->files_seq); + io_uring_cancel_files(ctx, data); /* @@ -8558,13 +8600,8 @@ static int io_uring_flush(struct file *file, void *data) } else if (ctx->flags & IORING_SETUP_SQPOLL) { struct io_sq_data *sqd = ctx->sq_data; - /* Ring is being closed, mark us as neding new assignment */ + /* quiesce sqpoll thread */ io_sq_thread_park(sqd); - mutex_lock(&ctx->uring_lock); - ctx->ring_fd = -1; - ctx->ring_file = NULL; - mutex_unlock(&ctx->uring_lock); - io_ring_set_wakeup_flag(ctx); io_sq_thread_unpark(sqd); } @@ -8701,18 +8738,6 @@ SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit, if (ctx->flags & IORING_SETUP_SQPOLL) { if (!list_empty_careful(&ctx->cq_overflow_list)) io_cqring_overflow_flush(ctx, false); - if (fd != ctx->ring_fd) { - struct io_sq_data *sqd = ctx->sq_data; - - io_sq_thread_park(sqd); - - mutex_lock(&ctx->uring_lock); - ctx->ring_fd = fd; - ctx->ring_file = f.file; - mutex_unlock(&ctx->uring_lock); - - io_sq_thread_unpark(sqd); - } if (flags & IORING_ENTER_SQ_WAKEUP) wake_up(&ctx->sq_data->wait); if (flags & IORING_ENTER_SQ_WAIT) @@ -8720,8 +8745,6 @@ SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit, submitted = to_submit; } else if (to_submit) { mutex_lock(&ctx->uring_lock); - ctx->ring_fd = fd; - ctx->ring_file = f.file; submitted = io_submit_sqes(ctx, to_submit); mutex_unlock(&ctx->uring_lock); -- 2.28.0