[PATCH] io_uring/rw: always clear ->bytes_done on io_async_rw setup

[PATCH] io_uring/rw: always clear ->bytes_done on io_async_rw setup

[Jens Axboe]

A previous commit mistakenly moved the clearing of the in-progress byte
count into the section that's dependent on having a cached iovec or not,
but it should be cleared for any IO. If not, then extra bytes may be
added at IO completion time, causing potentially weird behavior like
over-reporting the amount of IO done.

Fixes: f628c7e5a7c0 ("io_uring/rw: Allocate async data through helper")
Reported-by: kernel test robot <oliver.sang@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202412271132.a09c3500-lkp@intel.com
Signed-off-by: Jens Axboe <axboe@kernel.dk>

---

diff --git a/io_uring/rw.c b/io_uring/rw.c
index 75f70935ccf4..ca1b19d3d142 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -228,8 +228,8 @@ static int io_rw_alloc_async(struct io_kiocb *req)
 		kasan_mempool_unpoison_object(rw->free_iovec,
 					      rw->free_iov_nr * sizeof(struct iovec));
 		req->flags |= REQ_F_NEED_CLEANUP;
-		rw->bytes_done = 0;
 	}
+	rw->bytes_done = 0;
 	return 0;
 }
 
-- 
Jens Axboe

Re: [PATCH] io_uring/rw: always clear ->bytes_done on io_async_rw setup

[Gabriel Krisman Bertazi]

Jens Axboe <axboe@kernel.dk> writes:

> A previous commit mistakenly moved the clearing of the in-progress byte
> count into the section that's dependent on having a cached iovec or not,
> but it should be cleared for any IO. If not, then extra bytes may be
> added at IO completion time, causing potentially weird behavior like
> over-reporting the amount of IO done.

Hi Jens,

Sorry for the delay.  I went completely offline during the christmas
week.

Did this solve the sysbot report?  I'm failing to understand how it can
happen.  This could only be hit if the allocation returned a cached
object that doesn't have a free_iov, since any newly kmalloc'ed object
will have this field cleaned inside the io_rw_async_data_init callback.
But I don't understand where we can cache the rw object without having a
valid free_iov - it didn't seem possible to me before or now.

the iov is freed only by io_rw_iovec_free, which is called from

(1) io_rw_recycle, in the case where we don't cache.  we drop also
drop the CLEANUP flag, so we will just call kfree inside io_clean_op later.
(2) io_readv_writev_cleanup: where we also don't cache, since we are inside
    the io_clean_op, we'll just hit the kfree(req->async_data), and
(3) io_rw_cache_free:  where we are emptying the cache to shut down.

> diff --git a/io_uring/rw.c b/io_uring/rw.c
> index 75f70935ccf4..ca1b19d3d142 100644
> --- a/io_uring/rw.c
> +++ b/io_uring/rw.c
> @@ -228,8 +228,8 @@ static int io_rw_alloc_async(struct io_kiocb *req)
>  		kasan_mempool_unpoison_object(rw->free_iovec,
>  					      rw->free_iov_nr * sizeof(struct iovec));
>  		req->flags |= REQ_F_NEED_CLEANUP;
> -		rw->bytes_done = 0;
>  	}
> +	rw->bytes_done = 0;
>  	return 0;
>  }

-- 
Gabriel Krisman Bertazi

Re: [PATCH] io_uring/rw: always clear ->bytes_done on io_async_rw setup

[Jens Axboe]

On 12/30/24 9:08 AM, Gabriel Krisman Bertazi wrote:
> Jens Axboe <axboe@kernel.dk> writes:
> 
>> A previous commit mistakenly moved the clearing of the in-progress byte
>> count into the section that's dependent on having a cached iovec or not,
>> but it should be cleared for any IO. If not, then extra bytes may be
>> added at IO completion time, causing potentially weird behavior like
>> over-reporting the amount of IO done.
> 
> Hi Jens,
> 
> Sorry for the delay.  I went completely offline during the christmas
> week.

No worries, sounds like a good plan!

> Did this solve the sysbot report?  I'm failing to understand how it can
> happen.  This could only be hit if the allocation returned a cached
> object that doesn't have a free_iov, since any newly kmalloc'ed object
> will have this field cleaned inside the io_rw_async_data_init callback.
> But I don't understand where we can cache the rw object without having a
> valid free_iov - it didn't seem possible to me before or now.

Not sure I follow - you may never have a valid free_iov, it completely
depends on whether or not the existing rw user needed to allocate an iov
or not. Hence it's indeed possible that there's a free_iov and the user
doesn't need or use it, or the opposite of there not being one and the
user then allocating one that persists.

In any case, it's of course orthogonal to the issue here, which is that
->bytes_done must _always_ be initialized, it has no dependency on a
free_iovec or not. Whenever someone gets an 'rw', it should be pristine
in that sense.

-- 
Jens Axboe

Re: [PATCH] io_uring/rw: always clear ->bytes_done on io_async_rw setup

[Gabriel Krisman Bertazi]

Jens Axboe <axboe@kernel.dk> writes:

> On 12/30/24 9:08 AM, Gabriel Krisman Bertazi wrote:
>> Jens Axboe <axboe@kernel.dk> writes:
>> 
>>> A previous commit mistakenly moved the clearing of the in-progress byte
>>> count into the section that's dependent on having a cached iovec or not,
>>> but it should be cleared for any IO. If not, then extra bytes may be
>>> added at IO completion time, causing potentially weird behavior like
>>> over-reporting the amount of IO done.
>> 
>> Hi Jens,
>> 
>> Sorry for the delay.  I went completely offline during the christmas
>> week.
>
> No worries, sounds like a good plan!
>
>> Did this solve the sysbot report?  I'm failing to understand how it can
>> happen.  This could only be hit if the allocation returned a cached
>> object that doesn't have a free_iov, since any newly kmalloc'ed object
>> will have this field cleaned inside the io_rw_async_data_init callback.
>> But I don't understand where we can cache the rw object without having a
>> valid free_iov - it didn't seem possible to me before or now.
>
> Not sure I follow - you may never have a valid free_iov, it completely
> depends on whether or not the existing rw user needed to allocate an iov
> or not.

> Hence it's indeed possible that there's a free_iov and the user
> doesn't need or use it, or the opposite of there not being one and the
> user then allocating one that persists.
>
> In any case, it's of course orthogonal to the issue here, which is that
> ->bytes_done must _always_ be initialized, it has no dependency on a
> free_iovec or not. Whenever someone gets an 'rw', it should be pristine
> in that sense.

I see. In addition, I was actually confusing rw->free_iov_nr with
rw->bytes_done when writing my previous message.  The first needs to
have a valid value if ->free_iov is valid. Thanks for the explanation
and making me review this code.

The fix looks good to me now, obviously.

Thanks,
-- 
Gabriel Krisman Bertazi

Re: [PATCH] io_uring/rw: always clear ->bytes_done on io_async_rw setup

[Jens Axboe]

On 12/30/24 4:02 PM, Gabriel Krisman Bertazi wrote:
> Jens Axboe <axboe@kernel.dk> writes:
> 
>> On 12/30/24 9:08 AM, Gabriel Krisman Bertazi wrote:
>>> Jens Axboe <axboe@kernel.dk> writes:
>>>
>>>> A previous commit mistakenly moved the clearing of the in-progress byte
>>>> count into the section that's dependent on having a cached iovec or not,
>>>> but it should be cleared for any IO. If not, then extra bytes may be
>>>> added at IO completion time, causing potentially weird behavior like
>>>> over-reporting the amount of IO done.
>>>
>>> Hi Jens,
>>>
>>> Sorry for the delay.  I went completely offline during the christmas
>>> week.
>>
>> No worries, sounds like a good plan!
>>
>>> Did this solve the sysbot report?  I'm failing to understand how it can
>>> happen.  This could only be hit if the allocation returned a cached
>>> object that doesn't have a free_iov, since any newly kmalloc'ed object
>>> will have this field cleaned inside the io_rw_async_data_init callback.
>>> But I don't understand where we can cache the rw object without having a
>>> valid free_iov - it didn't seem possible to me before or now.
>>
>> Not sure I follow - you may never have a valid free_iov, it completely
>> depends on whether or not the existing rw user needed to allocate an iov
>> or not.
> 
>> Hence it's indeed possible that there's a free_iov and the user
>> doesn't need or use it, or the opposite of there not being one and the
>> user then allocating one that persists.
>>
>> In any case, it's of course orthogonal to the issue here, which is that
>> ->bytes_done must _always_ be initialized, it has no dependency on a
>> free_iovec or not. Whenever someone gets an 'rw', it should be pristine
>> in that sense.
> 
> I see. In addition, I was actually confusing rw->free_iov_nr with
> rw->bytes_done when writing my previous message.  The first needs to
> have a valid value if ->free_iov is valid. Thanks for the explanation
> and making me review this code.

Ah right, yes free_iov_nr would obviously need to be valid if free_iov
is set.

> The fix looks good to me now, obviously.

Thanks for checking!

-- 
Jens Axboe