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

[Gabriel Krisman Bertazi <krisman@suse.de>]

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