[PATCH] io_uring: kiocb_done() should *not* trust ->ki_pos if ->{read,write}_iter() failed

[PATCH] io_uring: kiocb_done() should *not* trust ->ki_pos if ->{read,write}_iter() failed

[Al Viro]

[in viro/vfs.git#fixes at the moment]
->ki_pos value is unreliable in such cases.  For an obvious example,
consider O_DSYNC write - we feed the data to page cache and start IO,
then we make sure it's completed.  Update of ->ki_pos is dealt with
by the first part; failure in the second ends up with negative value
returned _and_ ->ki_pos left advanced as if sync had been successful.
In the same situation write(2) does not advance the file position
at all.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 io_uring/rw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/io_uring/rw.c b/io_uring/rw.c
index c8c822fa7980..08d94fb972f0 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -339,7 +339,7 @@ static int kiocb_done(struct io_kiocb *req, ssize_t ret,
 	struct io_rw *rw = io_kiocb_to_cmd(req, struct io_rw);
 	unsigned final_ret = io_fixup_rw_res(req, ret);
 
-	if (req->flags & REQ_F_CUR_POS)
+	if (ret >= 0 && req->flags & REQ_F_CUR_POS)
 		req->file->f_pos = rw->kiocb.ki_pos;
 	if (ret >= 0 && (rw->kiocb.ki_complete == io_complete_rw)) {
 		if (!__io_complete_rw_common(req, ret)) {
-- 
2.39.2

Re: [PATCH] io_uring: kiocb_done() should *not* trust ->ki_pos if ->{read,write}_iter() failed

[Christian Brauner]

On Thu, Oct 26, 2023 at 03:18:40AM +0100, Al Viro wrote:
> [in viro/vfs.git#fixes at the moment]
> ->ki_pos value is unreliable in such cases.  For an obvious example,
> consider O_DSYNC write - we feed the data to page cache and start IO,
> then we make sure it's completed.  Update of ->ki_pos is dealt with
> by the first part; failure in the second ends up with negative value
> returned _and_ ->ki_pos left advanced as if sync had been successful.
> In the same situation write(2) does not advance the file position
> at all.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---

Looks good to me,
Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH] io_uring: kiocb_done() should *not* trust ->ki_pos if ->{read,write}_iter() failed

[Jens Axboe]

On 10/25/23 8:18 PM, Al Viro wrote:
> [in viro/vfs.git#fixes at the moment]
> ->ki_pos value is unreliable in such cases.  For an obvious example,
> consider O_DSYNC write - we feed the data to page cache and start IO,
> then we make sure it's completed.  Update of ->ki_pos is dealt with
> by the first part; failure in the second ends up with negative value
> returned _and_ ->ki_pos left advanced as if sync had been successful.
> In the same situation write(2) does not advance the file position
> at all.

Looks good, thanks Al:

Reviewed-by: Jens Axboe <axboe@kernel.dk>

or let me know if you want me to pick it up.

-- 
Jens Axboe