On Tue, Aug 04, 2020 at 03:55:16PM -0600, Jens Axboe wrote: > On 8/4/20 11:15 AM, Jens Axboe wrote: > > On 8/4/20 11:02 AM, xiao lin wrote: > >> 在 2020年8月4日星期二，Jens Axboe <axboe@xxxxxxxxx <mailto:axboe@xxxxxxxxx>> 写道： > >> > >> On 8/4/20 7:18 AM, Pavel Begunkov wrote: > >> > On 04/08/2020 15:56, Liu Yong wrote: > >> >> In io_send_recvmsg(), there is no check for the req->file. > >> >> User can change the opcode from IORING_OP_NOP to IORING_OP_SENDMSG > >> >> through competition after the io_req_set_file(). > >> > > >> > After sqe->opcode is read and copied in io_init_req(), it only uses > >> > in-kernel req->opcode. Also, io_init_req() should check for req->file > >> > NULL, so shouldn't happen after. > >> > > >> > Do you have a reproducer? What kernel version did you use? > >> > >> Was looking at this too, and I'm guessing this is some 5.4 based kernel. > >> Unfortunately the oops doesn't include that information. > > > >> Sorry, I forgot to mention that the kernel version I am using is 5.4.55. > > > > I think there are two options here: > > > > 1) Backport the series that ensured we only read those important bits once > > 2) Make s->sqe a full sqe, and memcpy it in > > Something like this should close the ->opcode re-read for 5.4-stable. > > diff --git a/fs/io_uring.c b/fs/io_uring.c > index e0200406765c..8bb5e19b7c3c 100644 > --- a/fs/io_uring.c > +++ b/fs/io_uring.c > @@ -279,6 +279,7 @@ struct sqe_submit { > bool has_user; > bool needs_lock; > bool needs_fixed_file; > + u8 opcode; > }; > > /* > @@ -505,7 +506,7 @@ static inline void io_queue_async_work(struct io_ring_ctx *ctx, > int rw = 0; > > if (req->submit.sqe) { > - switch (req->submit.sqe->opcode) { > + switch (req->submit.opcode) { > case IORING_OP_WRITEV: > case IORING_OP_WRITE_FIXED: > rw = !(req->rw.ki_flags & IOCB_DIRECT); > @@ -1254,23 +1255,15 @@ static int io_import_fixed(struct io_ring_ctx *ctx, int rw, > } > > static ssize_t io_import_iovec(struct io_ring_ctx *ctx, int rw, > - const struct sqe_submit *s, struct iovec **iovec, > + struct io_kiocb *req, struct iovec **iovec, > struct iov_iter *iter) > { > - const struct io_uring_sqe *sqe = s->sqe; > + const struct io_uring_sqe *sqe = req->submit.sqe; > void __user *buf = u64_to_user_ptr(READ_ONCE(sqe->addr)); > size_t sqe_len = READ_ONCE(sqe->len); > u8 opcode; > > - /* > - * We're reading ->opcode for the second time, but the first read > - * doesn't care whether it's _FIXED or not, so it doesn't matter > - * whether ->opcode changes concurrently. The first read does care > - * about whether it is a READ or a WRITE, so we don't trust this read > - * for that purpose and instead let the caller pass in the read/write > - * flag. > - */ > - opcode = READ_ONCE(sqe->opcode); > + opcode = req->submit.opcode; > if (opcode == IORING_OP_READ_FIXED || > opcode == IORING_OP_WRITE_FIXED) { > ssize_t ret = io_import_fixed(ctx, rw, sqe, iter); > @@ -1278,7 +1271,7 @@ static ssize_t io_import_iovec(struct io_ring_ctx *ctx, int rw, > return ret; > } > > - if (!s->has_user) > + if (!req->submit.has_user) > return -EFAULT; > > #ifdef CONFIG_COMPAT > @@ -1425,7 +1418,7 @@ static int io_read(struct io_kiocb *req, const struct sqe_submit *s, > if (unlikely(!(file->f_mode & FMODE_READ))) > return -EBADF; > > - ret = io_import_iovec(req->ctx, READ, s, &iovec, &iter); > + ret = io_import_iovec(req->ctx, READ, req, &iovec, &iter); > if (ret < 0) > return ret; > > @@ -1490,7 +1483,7 @@ static int io_write(struct io_kiocb *req, const struct sqe_submit *s, > if (unlikely(!(file->f_mode & FMODE_WRITE))) > return -EBADF; > > - ret = io_import_iovec(req->ctx, WRITE, s, &iovec, &iter); > + ret = io_import_iovec(req->ctx, WRITE, req, &iovec, &iter); > if (ret < 0) > return ret; > > @@ -2109,15 +2102,14 @@ static int io_req_defer(struct io_ring_ctx *ctx, struct io_kiocb *req, > static int __io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req, > const struct sqe_submit *s, bool force_nonblock) > { > - int ret, opcode; > + int ret; > > req->user_data = READ_ONCE(s->sqe->user_data); > > if (unlikely(s->index >= ctx->sq_entries)) > return -EINVAL; > > - opcode = READ_ONCE(s->sqe->opcode); > - switch (opcode) { > + switch (req->submit.opcode) { > case IORING_OP_NOP: > ret = io_nop(req, req->user_data); > break; > @@ -2181,10 +2173,10 @@ static int __io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req, > return 0; > } > > -static struct async_list *io_async_list_from_sqe(struct io_ring_ctx *ctx, > - const struct io_uring_sqe *sqe) > +static struct async_list *io_async_list_from_req(struct io_ring_ctx *ctx, > + struct io_kiocb *req) > { > - switch (sqe->opcode) { > + switch (req->submit.opcode) { > case IORING_OP_READV: > case IORING_OP_READ_FIXED: > return &ctx->pending_async[READ]; > @@ -2196,12 +2188,10 @@ static struct async_list *io_async_list_from_sqe(struct io_ring_ctx *ctx, > } > } > > -static inline bool io_sqe_needs_user(const struct io_uring_sqe *sqe) > +static inline bool io_req_needs_user(struct io_kiocb *req) > { > - u8 opcode = READ_ONCE(sqe->opcode); > - > - return !(opcode == IORING_OP_READ_FIXED || > - opcode == IORING_OP_WRITE_FIXED); > + return !(req->submit.opcode == IORING_OP_READ_FIXED || > + req->submit.opcode == IORING_OP_WRITE_FIXED); > } > > static void io_sq_wq_submit_work(struct work_struct *work) > @@ -2217,7 +2207,7 @@ static void io_sq_wq_submit_work(struct work_struct *work) > int ret; > > old_cred = override_creds(ctx->creds); > - async_list = io_async_list_from_sqe(ctx, req->submit.sqe); > + async_list = io_async_list_from_req(ctx, req); > > allow_kernel_signal(SIGINT); > restart: > @@ -2239,7 +2229,7 @@ static void io_sq_wq_submit_work(struct work_struct *work) > } > > ret = 0; > - if (io_sqe_needs_user(sqe) && !cur_mm) { > + if (io_req_needs_user(req) && !cur_mm) { > if (!mmget_not_zero(ctx->sqo_mm)) { > ret = -EFAULT; > } else { > @@ -2387,11 +2377,9 @@ static bool io_add_to_prev_work(struct async_list *list, struct io_kiocb *req) > return ret; > } > > -static bool io_op_needs_file(const struct io_uring_sqe *sqe) > +static bool io_op_needs_file(struct io_kiocb *req) > { > - int op = READ_ONCE(sqe->opcode); > - > - switch (op) { > + switch (req->submit.opcode) { > case IORING_OP_NOP: > case IORING_OP_POLL_REMOVE: > case IORING_OP_TIMEOUT: > @@ -2419,7 +2407,7 @@ static int io_req_set_file(struct io_ring_ctx *ctx, const struct sqe_submit *s, > */ > req->sequence = s->sequence; > > - if (!io_op_needs_file(s->sqe)) > + if (!io_op_needs_file(req)) > return 0; > > if (flags & IOSQE_FIXED_FILE) { > @@ -2460,7 +2448,7 @@ static int __io_queue_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req, > > s->sqe = sqe_copy; > memcpy(&req->submit, s, sizeof(*s)); > - list = io_async_list_from_sqe(ctx, s->sqe); > + list = io_async_list_from_req(ctx, req); > if (!io_add_to_prev_work(list, req)) { > if (list) > atomic_inc(&list->cnt); > @@ -2582,7 +2570,7 @@ static void io_submit_sqe(struct io_ring_ctx *ctx, struct sqe_submit *s, > req->user_data = s->sqe->user_data; > > #if defined(CONFIG_NET) > - switch (READ_ONCE(s->sqe->opcode)) { > + switch (req->submit.opcode) { > case IORING_OP_SENDMSG: > case IORING_OP_RECVMSG: > spin_lock(¤t->fs->lock); > @@ -2697,6 +2685,7 @@ static bool io_get_sqring(struct io_ring_ctx *ctx, struct sqe_submit *s) > if (head < ctx->sq_entries) { > s->index = head; > s->sqe = &ctx->sq_sqes[head]; > + s->opcode = READ_ONCE(s->sqe->opcode); > s->sequence = ctx->cached_sq_head; > ctx->cached_sq_head++; > return true; > > -- > Jens Axboe > I think this patch solves similar problems from the root cause. So, Should I submit this commit, or you?
