* sparse fixes for io_uring @ 2022-05-18 8:39 Christoph Hellwig 2022-05-18 8:40 ` [PATCH 1/6] io_uring: use a rwf_t for io_rw.flags Christoph Hellwig ` (5 more replies) 0 siblings, 6 replies; 8+ messages in thread From: Christoph Hellwig @ 2022-05-18 8:39 UTC (permalink / raw) To: axboe; +Cc: asml.silence, io-uring Try to make sparse a little more happy. The poll code is still a major trainwreck, but that will need more intensive care. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/6] io_uring: use a rwf_t for io_rw.flags 2022-05-18 8:39 sparse fixes for io_uring Christoph Hellwig @ 2022-05-18 8:40 ` Christoph Hellwig 2022-05-18 12:27 ` Jens Axboe 2022-05-18 8:40 ` [PATCH 2/6] io_uring: don't use ERR_PTR for user pointers Christoph Hellwig ` (4 subsequent siblings) 5 siblings, 1 reply; 8+ messages in thread From: Christoph Hellwig @ 2022-05-18 8:40 UTC (permalink / raw) To: axboe; +Cc: asml.silence, io-uring Remove the bogus __force casts and just use the proper type instead. Signed-off-by: Christoph Hellwig <[email protected]> --- fs/io_uring.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index 0b05d5aa5891a..99862cbc1041c 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -644,7 +644,7 @@ struct io_rw { struct kiocb kiocb; u64 addr; u32 len; - u32 flags; + rwf_t flags; }; struct io_connect { @@ -3636,7 +3636,7 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe) req->imu = NULL; req->rw.addr = READ_ONCE(sqe->addr); req->rw.len = READ_ONCE(sqe->len); - req->rw.flags = (__force u32)READ_ONCE(sqe->rw_flags); + req->rw.flags = READ_ONCE(sqe->rw_flags); /* used for fixed read/write too - just read unconditionally */ req->buf_index = READ_ONCE(sqe->buf_index); return 0; @@ -4274,7 +4274,7 @@ static int io_rw_init_file(struct io_kiocb *req, fmode_t mode) req->flags |= io_file_get_flags(file) << REQ_F_SUPPORT_NOWAIT_BIT; kiocb->ki_flags = iocb_flags(file); - ret = kiocb_set_rw_flags(kiocb, (__force rwf_t)req->rw.flags); + ret = kiocb_set_rw_flags(kiocb, req->rw.flags); if (unlikely(ret)) return ret; -- 2.30.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/6] io_uring: use a rwf_t for io_rw.flags 2022-05-18 8:40 ` [PATCH 1/6] io_uring: use a rwf_t for io_rw.flags Christoph Hellwig @ 2022-05-18 12:27 ` Jens Axboe 0 siblings, 0 replies; 8+ messages in thread From: Jens Axboe @ 2022-05-18 12:27 UTC (permalink / raw) To: Christoph Hellwig; +Cc: io-uring, asml.silence On Wed, 18 May 2022 10:40:00 +0200, Christoph Hellwig wrote: > Remove the bogus __force casts and just use the proper type instead. > > Applied, thanks! [1/6] io_uring: use a rwf_t for io_rw.flags commit: 20cbd21d899b72765e38481a926c7b2008c64350 [2/6] io_uring: don't use ERR_PTR for user pointers commit: 984824db844a9bd6e9e15ee469241982526a6ccd [3/6] io_uring: drop a spurious inline on a forward declaration commit: ee67ba3b20f7dcd001b7743eb8e46880cb27fdc6 [4/6] io_uring: make apoll_events a __poll_t commit: 58f5c8d39e0ea07fdaaea6a85c49000da83dc0cc [5/6] io_uring: consistently use the EPOLL* defines commit: a294bef57c55a45aef51d31e71d6892e8eba1483 [6/6] io_uring: use rcu_dereference in io_close commit: 0bf1dbee9baf3e78bff297245178f8c9a8ef8670 Best regards, -- Jens Axboe ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/6] io_uring: don't use ERR_PTR for user pointers 2022-05-18 8:39 sparse fixes for io_uring Christoph Hellwig 2022-05-18 8:40 ` [PATCH 1/6] io_uring: use a rwf_t for io_rw.flags Christoph Hellwig @ 2022-05-18 8:40 ` Christoph Hellwig 2022-05-18 8:40 ` [PATCH 3/6] io_uring: drop a spurious inline on a forward declaration Christoph Hellwig ` (3 subsequent siblings) 5 siblings, 0 replies; 8+ messages in thread From: Christoph Hellwig @ 2022-05-18 8:40 UTC (permalink / raw) To: axboe; +Cc: asml.silence, io-uring ERR_PTR abuses the high bits of a pointer to transport error information. This is only safe for kernel pointers and not user pointers. Fix io_buffer_select and its helpers to just return NULL for failure and get rid of this abuse. Signed-off-by: Christoph Hellwig <[email protected]> --- fs/io_uring.c | 83 +++++++++++++++++++++++---------------------------- 1 file changed, 37 insertions(+), 46 deletions(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index 99862cbc1041c..abb7108258f96 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -3797,11 +3797,8 @@ static int io_buffer_add_list(struct io_ring_ctx *ctx, } static void __user *io_provided_buffer_select(struct io_kiocb *req, size_t *len, - struct io_buffer_list *bl, - unsigned int issue_flags) + struct io_buffer_list *bl) { - void __user *ret = ERR_PTR(-ENOBUFS); - if (!list_empty(&bl->buf_list)) { struct io_buffer *kbuf; @@ -3812,11 +3809,9 @@ static void __user *io_provided_buffer_select(struct io_kiocb *req, size_t *len, req->flags |= REQ_F_BUFFER_SELECTED; req->kbuf = kbuf; req->buf_index = kbuf->bid; - ret = u64_to_user_ptr(kbuf->addr); + return u64_to_user_ptr(kbuf->addr); } - - io_ring_submit_unlock(req->ctx, issue_flags); - return ret; + return NULL; } static void __user *io_ring_buffer_select(struct io_kiocb *req, size_t *len, @@ -3829,7 +3824,7 @@ static void __user *io_ring_buffer_select(struct io_kiocb *req, size_t *len, if (unlikely(smp_load_acquire(&br->tail) == head)) { io_ring_submit_unlock(req->ctx, issue_flags); - return ERR_PTR(-ENOBUFS); + return NULL; } head &= bl->mask; @@ -3847,22 +3842,19 @@ static void __user *io_ring_buffer_select(struct io_kiocb *req, size_t *len, req->buf_list = bl; req->buf_index = buf->bid; - if (!(issue_flags & IO_URING_F_UNLOCKED)) - return u64_to_user_ptr(buf->addr); - - /* - * If we came in unlocked, we have no choice but to - * consume the buffer here. This does mean it'll be - * pinned until the IO completes. But coming in - * unlocked means we're in io-wq context, hence there - * should be no further retry. For the locked case, the - * caller must ensure to call the commit when the - * transfer completes (or if we get -EAGAIN and must - * poll or retry). - */ - req->buf_list = NULL; - bl->head++; - io_ring_submit_unlock(req->ctx, issue_flags); + if (issue_flags & IO_URING_F_UNLOCKED) { + /* + * If we came in unlocked, we have no choice but to consume the + * buffer here. This does mean it'll be pinned until the IO + * completes. But coming in unlocked means we're in io-wq + * context, hence there should be no further retry. For the + * locked case, the caller must ensure to call the commit when + * the transfer completes (or if we get -EAGAIN and must poll + * or retry). + */ + req->buf_list = NULL; + bl->head++; + } return u64_to_user_ptr(buf->addr); } @@ -3871,20 +3863,19 @@ static void __user *io_buffer_select(struct io_kiocb *req, size_t *len, { struct io_ring_ctx *ctx = req->ctx; struct io_buffer_list *bl; + void __user *ret = NULL; io_ring_submit_lock(req->ctx, issue_flags); bl = io_buffer_get_list(ctx, req->buf_index); - if (unlikely(!bl)) { - io_ring_submit_unlock(req->ctx, issue_flags); - return ERR_PTR(-ENOBUFS); + if (likely(bl)) { + if (bl->buf_nr_pages) + ret = io_ring_buffer_select(req, len, bl, issue_flags); + else + ret = io_provided_buffer_select(req, len, bl); } - - /* selection helpers drop the submit lock again, if needed */ - if (bl->buf_nr_pages) - return io_ring_buffer_select(req, len, bl, issue_flags); - - return io_provided_buffer_select(req, len, bl, issue_flags); + io_ring_submit_unlock(req->ctx, issue_flags); + return ret; } #ifdef CONFIG_COMPAT @@ -3906,8 +3897,8 @@ static ssize_t io_compat_import(struct io_kiocb *req, struct iovec *iov, len = clen; buf = io_buffer_select(req, &len, issue_flags); - if (IS_ERR(buf)) - return PTR_ERR(buf); + if (!buf) + return -ENOBUFS; req->rw.addr = (unsigned long) buf; iov[0].iov_base = buf; req->rw.len = iov[0].iov_len = (compat_size_t) len; @@ -3929,8 +3920,8 @@ static ssize_t __io_iov_buffer_select(struct io_kiocb *req, struct iovec *iov, if (len < 0) return -EINVAL; buf = io_buffer_select(req, &len, issue_flags); - if (IS_ERR(buf)) - return PTR_ERR(buf); + if (!buf) + return -ENOBUFS; req->rw.addr = (unsigned long) buf; iov[0].iov_base = buf; req->rw.len = iov[0].iov_len = len; @@ -3987,8 +3978,8 @@ static struct iovec *__io_import_iovec(int rw, struct io_kiocb *req, if (opcode == IORING_OP_READ || opcode == IORING_OP_WRITE) { if (io_do_buffer_select(req)) { buf = io_buffer_select(req, &sqe_len, issue_flags); - if (IS_ERR(buf)) - return ERR_CAST(buf); + if (!buf) + return ERR_PTR(-ENOBUFS); req->rw.addr = (unsigned long) buf; req->rw.len = sqe_len; } @@ -5259,8 +5250,8 @@ static int io_nop(struct io_kiocb *req, unsigned int issue_flags) size_t len = 1; buf = io_buffer_select(req, &len, issue_flags); - if (IS_ERR(buf)) - return PTR_ERR(buf); + if (!buf) + return -ENOBUFS; } if (!(req->ctx->flags & IORING_SETUP_CQE32)) @@ -6394,8 +6385,8 @@ static int io_recvmsg(struct io_kiocb *req, unsigned int issue_flags) void __user *buf; buf = io_buffer_select(req, &sr->len, issue_flags); - if (IS_ERR(buf)) - return PTR_ERR(buf); + if (!buf) + return -ENOBUFS; kmsg->fast_iov[0].iov_base = buf; kmsg->fast_iov[0].iov_len = sr->len; iov_iter_init(&kmsg->msg.msg_iter, READ, kmsg->fast_iov, 1, @@ -6464,8 +6455,8 @@ static int io_recv(struct io_kiocb *req, unsigned int issue_flags) void __user *buf; buf = io_buffer_select(req, &sr->len, issue_flags); - if (IS_ERR(buf)) - return PTR_ERR(buf); + if (!buf) + return -ENOBUFS; sr->buf = buf; } -- 2.30.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/6] io_uring: drop a spurious inline on a forward declaration 2022-05-18 8:39 sparse fixes for io_uring Christoph Hellwig 2022-05-18 8:40 ` [PATCH 1/6] io_uring: use a rwf_t for io_rw.flags Christoph Hellwig 2022-05-18 8:40 ` [PATCH 2/6] io_uring: don't use ERR_PTR for user pointers Christoph Hellwig @ 2022-05-18 8:40 ` Christoph Hellwig 2022-05-18 8:40 ` [PATCH 4/6] io_uring: make apoll_events a __poll_t Christoph Hellwig ` (2 subsequent siblings) 5 siblings, 0 replies; 8+ messages in thread From: Christoph Hellwig @ 2022-05-18 8:40 UTC (permalink / raw) To: axboe; +Cc: asml.silence, io-uring io_file_get_normal isn't marked inline, so don't claim it as such in the forward declaration. Signed-off-by: Christoph Hellwig <[email protected]> --- fs/io_uring.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index abb7108258f96..fc435f95ef340 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -1354,7 +1354,7 @@ static int __io_register_rsrc_update(struct io_ring_ctx *ctx, unsigned type, static void io_clean_op(struct io_kiocb *req); static inline struct file *io_file_get_fixed(struct io_kiocb *req, int fd, unsigned issue_flags); -static inline struct file *io_file_get_normal(struct io_kiocb *req, int fd); +static struct file *io_file_get_normal(struct io_kiocb *req, int fd); static void io_drop_inflight_file(struct io_kiocb *req); static bool io_assign_file(struct io_kiocb *req, unsigned int issue_flags); static void io_queue_sqe(struct io_kiocb *req); -- 2.30.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 4/6] io_uring: make apoll_events a __poll_t 2022-05-18 8:39 sparse fixes for io_uring Christoph Hellwig ` (2 preceding siblings ...) 2022-05-18 8:40 ` [PATCH 3/6] io_uring: drop a spurious inline on a forward declaration Christoph Hellwig @ 2022-05-18 8:40 ` Christoph Hellwig 2022-05-18 8:40 ` [PATCH 5/6] io_uring: consistently use the EPOLL* defines Christoph Hellwig 2022-05-18 8:40 ` [PATCH 6/6] io_uring: use rcu_dereference in io_close Christoph Hellwig 5 siblings, 0 replies; 8+ messages in thread From: Christoph Hellwig @ 2022-05-18 8:40 UTC (permalink / raw) To: axboe; +Cc: asml.silence, io-uring apoll_events is fed to vfs_poll and the poll tables, so it should be a __poll_t. Signed-off-by: Christoph Hellwig <[email protected]> --- fs/io_uring.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index fc435f95ef340..1b46c3e9df33a 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -1031,7 +1031,7 @@ struct io_kiocb { /* used by request caches, completion batching and iopoll */ struct io_wq_work_node comp_list; /* cache ->apoll->events */ - int apoll_events; + __poll_t apoll_events; }; atomic_t refs; atomic_t poll_refs; @@ -6977,7 +6977,7 @@ static void io_apoll_task_func(struct io_kiocb *req, bool *locked) io_req_complete_failed(req, ret); } -static void __io_poll_execute(struct io_kiocb *req, int mask, int events) +static void __io_poll_execute(struct io_kiocb *req, int mask, __poll_t events) { req->cqe.res = mask; /* @@ -6996,7 +6996,8 @@ static void __io_poll_execute(struct io_kiocb *req, int mask, int events) io_req_task_work_add(req, false); } -static inline void io_poll_execute(struct io_kiocb *req, int res, int events) +static inline void io_poll_execute(struct io_kiocb *req, int res, + __poll_t events) { if (io_poll_get_ownership(req)) __io_poll_execute(req, res, events); -- 2.30.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 5/6] io_uring: consistently use the EPOLL* defines 2022-05-18 8:39 sparse fixes for io_uring Christoph Hellwig ` (3 preceding siblings ...) 2022-05-18 8:40 ` [PATCH 4/6] io_uring: make apoll_events a __poll_t Christoph Hellwig @ 2022-05-18 8:40 ` Christoph Hellwig 2022-05-18 8:40 ` [PATCH 6/6] io_uring: use rcu_dereference in io_close Christoph Hellwig 5 siblings, 0 replies; 8+ messages in thread From: Christoph Hellwig @ 2022-05-18 8:40 UTC (permalink / raw) To: axboe; +Cc: asml.silence, io-uring POLL* are unannotated values for the userspace ABI, while everything in-kernel should use EPOLL* and the __poll_t type. Signed-off-by: Christoph Hellwig <[email protected]> --- fs/io_uring.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index 1b46c3e9df33a..c9596d551bd67 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -7012,7 +7012,7 @@ static void io_poll_cancel_req(struct io_kiocb *req) #define wqe_to_req(wait) ((void *)((unsigned long) (wait)->private & ~1)) #define wqe_is_double(wait) ((unsigned long) (wait)->private & 1) -#define IO_ASYNC_POLL_COMMON (EPOLLONESHOT | POLLPRI) +#define IO_ASYNC_POLL_COMMON (EPOLLONESHOT | EPOLLPRI) static int io_poll_wake(struct wait_queue_entry *wait, unsigned mode, int sync, void *key) @@ -7217,14 +7217,14 @@ static int io_arm_poll_handler(struct io_kiocb *req, unsigned issue_flags) mask |= EPOLLONESHOT; if (def->pollin) { - mask |= POLLIN | POLLRDNORM; + mask |= EPOLLIN | EPOLLRDNORM; /* If reading from MSG_ERRQUEUE using recvmsg, ignore POLLIN */ if ((req->opcode == IORING_OP_RECVMSG) && (req->sr_msg.msg_flags & MSG_ERRQUEUE)) - mask &= ~POLLIN; + mask &= ~EPOLLIN; } else { - mask |= POLLOUT | POLLWRNORM; + mask |= EPOLLOUT | EPOLLWRNORM; } if (def->poll_exclusive) mask |= EPOLLEXCLUSIVE; -- 2.30.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 6/6] io_uring: use rcu_dereference in io_close 2022-05-18 8:39 sparse fixes for io_uring Christoph Hellwig ` (4 preceding siblings ...) 2022-05-18 8:40 ` [PATCH 5/6] io_uring: consistently use the EPOLL* defines Christoph Hellwig @ 2022-05-18 8:40 ` Christoph Hellwig 5 siblings, 0 replies; 8+ messages in thread From: Christoph Hellwig @ 2022-05-18 8:40 UTC (permalink / raw) To: axboe; +Cc: asml.silence, io-uring Accessing the file table needs a rcu_dereference_protected(). Signed-off-by: Christoph Hellwig <[email protected]> --- fs/io_uring.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index c9596d551bd67..2b848a8dfc46c 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -5984,7 +5984,8 @@ static int io_close(struct io_kiocb *req, unsigned int issue_flags) spin_unlock(&files->file_lock); goto err; } - file = fdt->fd[close->fd]; + file = rcu_dereference_protected(fdt->fd[close->fd], + lockdep_is_held(&files->file_lock)); if (!file || file->f_op == &io_uring_fops) { spin_unlock(&files->file_lock); file = NULL; -- 2.30.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-05-18 12:27 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-05-18 8:39 sparse fixes for io_uring Christoph Hellwig 2022-05-18 8:40 ` [PATCH 1/6] io_uring: use a rwf_t for io_rw.flags Christoph Hellwig 2022-05-18 12:27 ` Jens Axboe 2022-05-18 8:40 ` [PATCH 2/6] io_uring: don't use ERR_PTR for user pointers Christoph Hellwig 2022-05-18 8:40 ` [PATCH 3/6] io_uring: drop a spurious inline on a forward declaration Christoph Hellwig 2022-05-18 8:40 ` [PATCH 4/6] io_uring: make apoll_events a __poll_t Christoph Hellwig 2022-05-18 8:40 ` [PATCH 5/6] io_uring: consistently use the EPOLL* defines Christoph Hellwig 2022-05-18 8:40 ` [PATCH 6/6] io_uring: use rcu_dereference in io_close Christoph Hellwig
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox