sparse fixes for io_uring

sparse fixes for io_uring

[Christoph Hellwig]

Try to make sparse a little more happy.  The poll code is still a major
trainwreck, but that will need more intensive care.

[PATCH 1/6] io_uring: use a rwf_t for io_rw.flags

[Christoph Hellwig]

Remove the bogus __force casts and just use the proper type instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 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

[PATCH 2/6] io_uring: don't use ERR_PTR for user pointers

[Christoph Hellwig]

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 <hch@lst.de>
---
 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

[PATCH 3/6] io_uring: drop a spurious inline on a forward declaration

[Christoph Hellwig]

io_file_get_normal isn't marked inline, so don't claim it as such in the
forward declaration.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 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

[PATCH 4/6] io_uring: make apoll_events a __poll_t

[Christoph Hellwig]

apoll_events is fed to vfs_poll and the poll tables, so it should be
a __poll_t.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 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

[PATCH 5/6] io_uring: consistently use the EPOLL* defines

[Christoph Hellwig]

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 <hch@lst.de>
---
 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

[PATCH 6/6] io_uring: use rcu_dereference in io_close

[Christoph Hellwig]

Accessing the file table needs a rcu_dereference_protected().

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 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

Re: [PATCH 1/6] io_uring: use a rwf_t for io_rw.flags

[Jens Axboe]

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