Re: [PATCH v2 05/17] block/io: support int64_t bytes in bdrv_co_do_pwrite_zeroes()

[Eric Blake]

On 4/27/20 3:23 AM, Vladimir Sementsov-Ogievskiy wrote:
> We are generally moving to int64_t for both offset and bytes parameters
> on all io paths. Prepare bdrv_co_do_pwrite_zeroes() now.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>   block/io.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index 4796476835..c8c30e3699 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -42,7 +42,7 @@
>   
>   static void bdrv_parent_cb_resize(BlockDriverState *bs);
>   static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
> -    int64_t offset, int bytes, BdrvRequestFlags flags);
> +    int64_t offset, int64_t bytes, BdrvRequestFlags flags);
>   
>   static void bdrv_parent_drained_begin(BlockDriverState *bs, BdrvChild *ignore,
>                                         bool ignore_bds_parents)
> @@ -1743,7 +1743,7 @@ int coroutine_fn bdrv_co_preadv_part(BdrvChild *child,
>   }
>   
>   static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
> -    int64_t offset, int bytes, BdrvRequestFlags flags)
> +    int64_t offset, int64_t bytes, BdrvRequestFlags flags)

Widens from 32- to 64-bit.  Callers (I'm looking at pre-series code, the 
further I get into your series, the more likely that intermediate 
changes may alter the analysis...):

bdrv_co_do_copy_on_readv() - passes 'int64_t pnum' bounded by 
fragmenting loop limited to MAX_BOUNCE_BUFFER
bdrv_aligned_pwritev() - passes 'unsigned int bytes' - latent bug fix 
for sizes between 2G and 4G, if any

to see if that bug could be tickled, look at callers of 
bdrv_aligned_pwritev:

bdrv_co_do_zero_pwritev() - splits 'unsigned int bytes' into 
head|body|tail; head and tail are safe but body could be > 2G
bdrv_co_pwritev_part() - gates with bdrv_check_byte_request()

continuing the audit, callers of bdrv_co_do_zero_pwritev:

bdrv_co_pwritev_part() - gates with bdrv_check_byte_request()

okay, all callers pass < 2G per our current code in 
bdrv_check_byte_request(), so there is no actual bug.  Still, the latent 
fix would be nice to mention in the commit message.

>   {
>       BlockDriver *drv = bs->drv;
>       QEMUIOVector qiov;
> @@ -1773,7 +1773,7 @@ static int coroutine_fn 
> bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
>       assert(max_write_zeroes >= bs->bl.request_alignment);
>   
>       while (bytes > 0 && !ret) {
> -        int num = bytes;
> +        int64_t num = bytes;

Use of 'bytes' within the function:

compute 'int tail' via % 'int alignment' - safe
fragmentation loop 'int num' - still fragments with a cap on max_transfer

use of 'num' within the loop
compute 'int head' via % 'int alignment' - safe
clamp size by 'int max_write_zeroes' - safe
drv->bdrv_co_pwrite_zeroes(int) - safe because of clamping
clamp size by 'int max_transfer' - safe
qemu_iovec_init_buf(size_t) - safe because of clamping
bdrv_driver_pwritev(uint64_t) [well, int64_t after 4/17] - safe

So even with the wider type, we aren't exceeding the contract of 
anything we pass it on to.  Later patches may improve 
drv->bdrv_co_pwrite_zeroes and qemu_iovec_init_buf to be 64-bit clean, 
at which point we would want to revisit this function to use 64-bit 
clamping rather than 32-bit clamping, but it does not have to happen here.

Reviewed-by: Eric Blake <address@hidden>

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org