[GIT PULL] io_uring fix for 5.16-rc7

[GIT PULL] io_uring fix for 5.16-rc7

[Jens Axboe]

Hi Linus,

Single fix for not clearing kiocb->ki_pos back to 0 for a stream,
destined for stable as well.

Please pull!


The following changes since commit d800c65c2d4eccebb27ffb7808e842d5b533823c:

  io-wq: drop wqe lock before creating new worker (2021-12-13 09:04:01 -0700)

are available in the Git repository at:

  git://git.kernel.dk/linux-block.git tags/io_uring-5.16-2021-12-23

for you to fetch changes up to 7b9762a5e8837b92a027d58d396a9d27f6440c36:

  io_uring: zero iocb->ki_pos for stream file types (2021-12-22 20:34:32 -0700)

----------------------------------------------------------------
io_uring-5.16-2021-12-23

----------------------------------------------------------------
Jens Axboe (1):
      io_uring: zero iocb->ki_pos for stream file types

 fs/io_uring.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

-- 
Jens Axboe

Re: [GIT PULL] io_uring fix for 5.16-rc7

[Linus Torvalds]

On Thu, Dec 23, 2021 at 1:11 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> Single fix for not clearing kiocb->ki_pos back to 0 for a stream,
> destined for stable as well.

I don't think any of this is right.

You can't use f_pos without using fdget_pos() to actually get the lock to it.

Which you don't do in io_uring.

            Linus

Re: [GIT PULL] io_uring fix for 5.16-rc7

[Linus Torvalds]

On Thu, Dec 23, 2021 at 3:39 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I don't think any of this is right.
>
> You can't use f_pos without using fdget_pos() to actually get the lock to it.
>
> Which you don't do in io_uring.

I've pulled it because it's still an improvement on what it used to
be, but f_pos use in io_uring does seem very broken.

            Linus

Re: [GIT PULL] io_uring fix for 5.16-rc7

[Jens Axboe]

On 12/23/21 4:43 PM, Linus Torvalds wrote:
> On Thu, Dec 23, 2021 at 3:39 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> I don't think any of this is right.
>>
>> You can't use f_pos without using fdget_pos() to actually get the lock to it.
>>
>> Which you don't do in io_uring.
> 
> I've pulled it because it's still an improvement on what it used to
> be, but f_pos use in io_uring does seem very broken.

Totally untested, but something like this should make it saner. This
grabs the lock (if needed) and position at issue time, rather than at
prep time. We could potentially handle the unlock in the handler
themselves too, but store the state in a flag and make it part of the
slower path cleanup instead. That ensures we always end up performing
it, even if the request is errored.

Might be straight forward enough for 5.16 in fact, but considering it's
not a new regression, deferring for 5.17 will be saner imho.


diff --git a/fs/io_uring.c b/fs/io_uring.c
index fb2a0cb4aaf8..a80f9f8f2cd0 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -112,7 +112,7 @@
 
 #define IO_REQ_CLEAN_FLAGS (REQ_F_BUFFER_SELECTED | REQ_F_NEED_CLEANUP | \
 				REQ_F_POLLED | REQ_F_INFLIGHT | REQ_F_CREDS | \
-				REQ_F_ASYNC_DATA)
+				REQ_F_ASYNC_DATA | REQ_F_CUR_POS_LOCK)
 
 #define IO_TCTX_REFS_CACHE_NR	(1U << 10)
 
@@ -726,6 +726,7 @@ enum {
 	REQ_F_FAIL_BIT		= 8,
 	REQ_F_INFLIGHT_BIT,
 	REQ_F_CUR_POS_BIT,
+	REQ_F_CUR_POS_LOCK_BIT,
 	REQ_F_NOWAIT_BIT,
 	REQ_F_LINK_TIMEOUT_BIT,
 	REQ_F_NEED_CLEANUP_BIT,
@@ -765,6 +766,8 @@ enum {
 	REQ_F_INFLIGHT		= BIT(REQ_F_INFLIGHT_BIT),
 	/* read/write uses file position */
 	REQ_F_CUR_POS		= BIT(REQ_F_CUR_POS_BIT),
+	/* request is holding file position lock */
+	REQ_F_CUR_POS_LOCK	= BIT(REQ_F_CUR_POS_LOCK_BIT),
 	/* must not punt to workers */
 	REQ_F_NOWAIT		= BIT(REQ_F_NOWAIT_BIT),
 	/* has or had linked timeout */
@@ -2892,12 +2895,15 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 
 	kiocb->ki_pos = READ_ONCE(sqe->off);
 	if (kiocb->ki_pos == -1) {
-		if (!(file->f_mode & FMODE_STREAM)) {
+		/*
+		 * If we end up using the current file position, just set the
+		 * flag and the actual file position will be read in the op
+		 * handler themselves.
+		 */
+		if (!(file->f_mode & FMODE_STREAM))
 			req->flags |= REQ_F_CUR_POS;
-			kiocb->ki_pos = file->f_pos;
-		} else {
+		else
 			kiocb->ki_pos = 0;
-		}
 	}
 	kiocb->ki_flags = iocb_flags(file);
 	ret = kiocb_set_rw_flags(kiocb, READ_ONCE(sqe->rw_flags));
@@ -3515,6 +3521,25 @@ static bool need_read_all(struct io_kiocb *req)
 		S_ISBLK(file_inode(req->file)->i_mode);
 }
 
+static bool io_file_pos_lock(struct io_kiocb *req, bool force_nonblock)
+{
+	struct file *file = req->file;
+
+	if (!(req->flags & REQ_F_CUR_POS))
+		return false;
+	if (file->f_mode & FMODE_ATOMIC_POS && file_count(file) > 1) {
+		if (force_nonblock) {
+			if (!mutex_trylock(&file->f_pos_lock))
+				return true;
+		} else {
+			mutex_lock(&file->f_pos_lock);
+		}
+		req->flags |= REQ_F_CUR_POS_LOCK;
+	}
+	req->rw.kiocb.ki_pos = file->f_pos;
+	return false;
+}
+
 static int io_read(struct io_kiocb *req, unsigned int issue_flags)
 {
 	struct io_rw_state __s, *s = &__s;
@@ -3543,7 +3568,8 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags)
 
 	if (force_nonblock) {
 		/* If the file doesn't support async, just async punt */
-		if (unlikely(!io_file_supports_nowait(req))) {
+		if (unlikely(!io_file_supports_nowait(req) ||
+			     io_file_pos_lock(req, true))) {
 			ret = io_setup_async_rw(req, iovec, s, true);
 			return ret ?: -EAGAIN;
 		}
@@ -3551,6 +3577,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags)
 	} else {
 		/* Ensure we clear previously set non-block flag */
 		kiocb->ki_flags &= ~IOCB_NOWAIT;
+		io_file_pos_lock(req, false);
 	}
 
 	ret = rw_verify_area(READ, req->file, io_kiocb_ppos(kiocb), req->result);
@@ -3668,7 +3695,8 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags)
 
 	if (force_nonblock) {
 		/* If the file doesn't support async, just async punt */
-		if (unlikely(!io_file_supports_nowait(req)))
+		if (unlikely(!io_file_supports_nowait(req) ||
+			     io_file_pos_lock(req, true)))
 			goto copy_iov;
 
 		/* file path doesn't support NOWAIT for non-direct_IO */
@@ -3680,6 +3708,7 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags)
 	} else {
 		/* Ensure we clear previously set non-block flag */
 		kiocb->ki_flags &= ~IOCB_NOWAIT;
+		io_file_pos_lock(req, false);
 	}
 
 	ret = rw_verify_area(WRITE, req->file, io_kiocb_ppos(kiocb), req->result);
@@ -6584,6 +6613,8 @@ static void io_clean_op(struct io_kiocb *req)
 		kfree(req->kbuf);
 		req->kbuf = NULL;
 	}
+	if (req->flags & REQ_F_CUR_POS_LOCK)
+		__f_unlock_pos(req->file);
 
 	if (req->flags & REQ_F_NEED_CLEANUP) {
 		switch (req->opcode) {

-- 
Jens Axboe

Re: [GIT PULL] io_uring fix for 5.16-rc7

[Jens Axboe]

On 12/23/21 4:39 PM, Linus Torvalds wrote:
> On Thu, Dec 23, 2021 at 1:11 PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> Single fix for not clearing kiocb->ki_pos back to 0 for a stream,
>> destined for stable as well.
> 
> I don't think any of this is right.
> 
> You can't use f_pos without using fdget_pos() to actually get the lock
> to it.
> 
> Which you don't do in io_uring.

Well, current position doesn't really make much sense to begin with in
an async API, but there are various use cases that want to use it for
sync IO. We do have the file at that point, but it's most certainly not
sychronized across completions (or serialized with other fdget_pos()
users).

We could hold ->f_pos_lock over the duration of the IO however, that
would probably be saner... As completions are always done from the task
itself as well, that should work. I'll take a look at that for 5.17.

-- 
Jens Axboe

Re: [GIT PULL] io_uring fix for 5.16-rc7

[pr-tracker-bot]

The pull request you sent on Thu, 23 Dec 2021 14:11:26 -0700:

> git://git.kernel.dk/linux-block.git tags/io_uring-5.16-2021-12-23

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/a026fa5404316787c2104bec3f8ff506acf85b98

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html