Re: [PATCH v2 01/17] block/throttle-groups: throttle_group_co_io_limits_intercept(): 64bit bytes

[Vladimir Sementsov-Ogievskiy]

29.04.2020 1:09, Eric Blake wrote:
> On 4/27/20 3:23 AM, Vladimir Sementsov-Ogievskiy wrote:
> > The function is called from 64bit io handlers, and bytes is just passed
> > to throttle_account() which is 64bit too (unsigned though). So, let's
> > convert intermediate argument to 64bit too.
>
> My audit for this patch:
>
> Caller has 32-bit, this patch now causes widening which is safe:
> block/block-backend.c: blk_do_preadv() passes 'unsigned int'
> block/block-backend.c: blk_do_pwritev_part() passes 'unsigned int'
> block/throttle.c: throttle_co_pwrite_zeroes() passes 'int'
> block/throttle.c: throttle_co_pdiscard() passes 'int'
>
> Caller has 64-bit, this patch fixes potential bug where pre-patch could narrow, 
> except it's easy enough to trace that callers are still capped at 2G actions:
> block/throttle.c: throttle_co_preadv() passes 'uint64_t'
> block/throttle.c: throttle_co_pwritev() passes 'uint64_t'
>
> Implementation in question: block/throttle-groups.c 
> throttle_group_co_io_limits_intercept() takes 'unsigned int bytes' and uses it:
> argument to util/throttle.c throttle_account(uint64_t)
>
> All safe: it patches a latent bug, and does not introduce any 64-bit gotchas 
> once throttle_co_p{read,write}v are relaxed, and assuming throttle_account() is 
> not buggy.

Should I add this all to commit message?

> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > ---
> >   include/block/throttle-groups.h | 2 +-
> >   block/throttle-groups.c         | 2 +-
> >   2 files changed, 2 insertions(+), 2 deletions(-)
>
> Reviewed-by: Eric Blake <address@hidden>

Thanks for careful review!

--
Best regards,
Vladimir