[PATCH] io_uring: revert consumed iov_iter bytes on error

[PATCH] io_uring: revert consumed iov_iter bytes on error

[Jens Axboe]

Some consumers of the iov_iter will return an error, but still have
bytes consumed in the iterator. This is an issue for -EAGAIN, since we
rely on a sane iov_iter state across retries.

Fix this by ensuring that we revert consumed bytes, if any, if the file
operations have consumed any bytes from iterator. This is similar to what
generic_file_read_iter() does, and is always safe as we have the previous
bytes count handy already.

Fixes: ff6165b2d7f6 ("io_uring: retain iov_iter state over io_read/io_write calls")
Reported-by: Dmitry Shulyak <yashulyak@gmail.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>

---

diff --git a/fs/io_uring.c b/fs/io_uring.c
index c9d526ff55e0..e030b33fa53e 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -3153,6 +3153,8 @@ static int io_read(struct io_kiocb *req, bool force_nonblock,
 	} else if (ret == -EAGAIN) {
 		if (!force_nonblock)
 			goto done;
+		/* some cases will consume bytes even on error returns */
+		iov_iter_revert(iter, iov_count - iov_iter_count(iter));
 		ret = io_setup_async_rw(req, iovec, inline_vecs, iter, false);
 		if (ret)
 			goto out_free;
@@ -3294,6 +3296,8 @@ static int io_write(struct io_kiocb *req, bool force_nonblock,
 	if (!force_nonblock || ret2 != -EAGAIN) {
 		kiocb_done(kiocb, ret2, cs);
 	} else {
+		/* some cases will consume bytes even on error returns */
+		iov_iter_revert(iter, iov_count - iov_iter_count(iter));
 copy_iov:
 		ret = io_setup_async_rw(req, iovec, inline_vecs, iter, false);
 		if (!ret)

-- 
Jens Axboe

Re: [PATCH] io_uring: revert consumed iov_iter bytes on error

[Stefano Garzarella]

On Mon, Aug 24, 2020 at 11:48:44AM -0600, Jens Axboe wrote:
> Some consumers of the iov_iter will return an error, but still have
> bytes consumed in the iterator. This is an issue for -EAGAIN, since we
> rely on a sane iov_iter state across retries.
> 
> Fix this by ensuring that we revert consumed bytes, if any, if the file
> operations have consumed any bytes from iterator. This is similar to what
> generic_file_read_iter() does, and is always safe as we have the previous
> bytes count handy already.
> 
> Fixes: ff6165b2d7f6 ("io_uring: retain iov_iter state over io_read/io_write calls")
> Reported-by: Dmitry Shulyak <yashulyak@gmail.com>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> 
> ---
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index c9d526ff55e0..e030b33fa53e 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -3153,6 +3153,8 @@ static int io_read(struct io_kiocb *req, bool force_nonblock,
>  	} else if (ret == -EAGAIN) {
>  		if (!force_nonblock)
>  			goto done;
> +		/* some cases will consume bytes even on error returns */
> +		iov_iter_revert(iter, iov_count - iov_iter_count(iter));
>  		ret = io_setup_async_rw(req, iovec, inline_vecs, iter, false);
>  		if (ret)
>  			goto out_free;
> @@ -3294,6 +3296,8 @@ static int io_write(struct io_kiocb *req, bool force_nonblock,
>  	if (!force_nonblock || ret2 != -EAGAIN) {
>  		kiocb_done(kiocb, ret2, cs);
>  	} else {
> +		/* some cases will consume bytes even on error returns */
> +		iov_iter_revert(iter, iov_count - iov_iter_count(iter));
>  copy_iov:
>  		ret = io_setup_async_rw(req, iovec, inline_vecs, iter, false);
>  		if (!ret)
> 

What about moving iov_iter_revert() in io_setup_async_rw(), passing
iov_initial_count as parameter?

Maybe it's out of purpose since we use it even when we're not trying again.


Anyway the patch LGTM:

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Thanks,
Stefano

Re: [PATCH] io_uring: revert consumed iov_iter bytes on error

[Jens Axboe]

On 8/25/20 6:33 AM, Stefano Garzarella wrote:
> On Mon, Aug 24, 2020 at 11:48:44AM -0600, Jens Axboe wrote:
>> Some consumers of the iov_iter will return an error, but still have
>> bytes consumed in the iterator. This is an issue for -EAGAIN, since we
>> rely on a sane iov_iter state across retries.
>>
>> Fix this by ensuring that we revert consumed bytes, if any, if the file
>> operations have consumed any bytes from iterator. This is similar to what
>> generic_file_read_iter() does, and is always safe as we have the previous
>> bytes count handy already.
>>
>> Fixes: ff6165b2d7f6 ("io_uring: retain iov_iter state over io_read/io_write calls")
>> Reported-by: Dmitry Shulyak <yashulyak@gmail.com>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>
>> ---
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index c9d526ff55e0..e030b33fa53e 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -3153,6 +3153,8 @@ static int io_read(struct io_kiocb *req, bool force_nonblock,
>>  	} else if (ret == -EAGAIN) {
>>  		if (!force_nonblock)
>>  			goto done;
>> +		/* some cases will consume bytes even on error returns */
>> +		iov_iter_revert(iter, iov_count - iov_iter_count(iter));
>>  		ret = io_setup_async_rw(req, iovec, inline_vecs, iter, false);
>>  		if (ret)
>>  			goto out_free;
>> @@ -3294,6 +3296,8 @@ static int io_write(struct io_kiocb *req, bool force_nonblock,
>>  	if (!force_nonblock || ret2 != -EAGAIN) {
>>  		kiocb_done(kiocb, ret2, cs);
>>  	} else {
>> +		/* some cases will consume bytes even on error returns */
>> +		iov_iter_revert(iter, iov_count - iov_iter_count(iter));
>>  copy_iov:
>>  		ret = io_setup_async_rw(req, iovec, inline_vecs, iter, false);
>>  		if (!ret)
>>
> 
> What about moving iov_iter_revert() in io_setup_async_rw(), passing
> iov_initial_count as parameter?
> 
> Maybe it's out of purpose since we use it even when we're not trying
> again.

The read side looks a little nicer, since we keep it close to where the
-EAGAIN happened. And as you mention, we don't need it for all the async
setup cases, only the ones where we tried to do IO first.
io_setup_async_rw is already pretty busy with arguments, so I think
that'd just make it harder to follow.

> Anyway the patch LGTM:
> 
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Thanks, added.

-- 
Jens Axboe