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

[Jens Axboe <axboe@kernel.dk>]

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