Re: [PATCH v2 04/17] block/io: use int64_t bytes in driver wrappers

[Eric Blake <eblake@redhat.com>]

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. Convert driver wrappers parameters which are
> already 64bit to signed type.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/io.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 1267918def..4796476835 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1086,7 +1086,7 @@ static void bdrv_co_io_em_complete(void *opaque, int ret)
>   }
>   
>   static int coroutine_fn bdrv_driver_preadv(BlockDriverState *bs,
> -                                           uint64_t offset, uint64_t bytes,
> +                                           int64_t offset, int64_t bytes,

Remains 64 bits, the question is whether any caller could pass in 
something larger than 63 bits.  Callers are:

bdrv_co_do_copy_on_readv() - passes 'int64_t pnum', but sets pnum in a 
fragmenting loop, MAX_BOUNCE_BUFFER when copy-on-read is needed, or 
max_transfer bounded by BDRV_REQUEST_MAX_BYTES otherwise
bdrv_aligned_preadv() - passes 'unsigned int bytes' further limited by 
fragmenting loop on max_transfer <= INT_MAX

Input is bounded to < 2G, internal use of 'bytes' is:
drv->bdrv_co_preadv_part(uint64_t) - safe
qemu_iovec_init_slice(size_t) - potential truncation on 32-bit platform, 
if you don't fix qemu_iovec
drv->bdrv_co_preadv(uint64_t) - safe
drv->bdrv_aio_preadv(uint64_t) - safe
drv->bdrv_co_readv(int) after assertion of <2G - safe

>                                              QEMUIOVector *qiov,
>                                              size_t qiov_offset, int flags)
>   {
> @@ -1155,7 +1155,7 @@ out:
>   }
>   
>   static int coroutine_fn bdrv_driver_pwritev(BlockDriverState *bs,
> -                                            uint64_t offset, uint64_t bytes,
> +                                            int64_t offset, int64_t bytes,

Remains 64 bits, the question is whether any caller could pass in 
something larger than 63.  Callers are:

bdrv_co_do_copy_on_readv() - passes 'int64_t pnum', but set in a 
fragmenting loop bounded by MAX_BOUNCE_BUFFER
bdrv_co_do_pwrite_zeroes() - passes 'int num'
bdrv_aligned_pwritev() - passes 'unsigned int bytes' further limited by 
fragmenting loop on max_transfer <= INT_MAX

Input is bounded to < 2G, internal use of 'bytes' is:
drv->bdrv_co_pwritev_part(uint64_t) - safe
qemu_iovec_init_slice(size_t) - potential truncation on 32-bit platform, 
if you don't fix qemu_iovec
drv->bdrv_co_pwritev(uint64_t) - safe
drv->bdrv_aio_pwritev(uint64_t) - safe
drv->bdrv_co_writev(int) after assertion of <2G - safe

>                                               QEMUIOVector *qiov,
>                                               size_t qiov_offset, int flags)
>   {
> @@ -1235,8 +1235,8 @@ emulate_flags:
>   }
>   
>   static int coroutine_fn
> -bdrv_driver_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
> -                               uint64_t bytes, QEMUIOVector *qiov,
> +bdrv_driver_pwritev_compressed(BlockDriverState *bs, int64_t offset,
> +                               int64_t bytes, QEMUIOVector *qiov,
>                                  size_t qiov_offset)

Remains 64 bits, the question is whether any caller could pass in 
something larger than 63.  Callers are:

bdrv_aligned_pwritev() - passes 'unsigned int bytes'

Input is bounded to < 4G, internal use of 'bytes' is:
drv->bdrv_co_pwritev_compressed_part(uint64_t) - safe
drv->bdrv_co_pwritev_compressed(uint64_t) - safe
qemu_iovec_init_slice(size_t) - potential truncation on 32-bit platform, 
if you don't fix qemu_iovec

Because our input is bounded, all of the changes made here have no 
semantic impact, and I argue this patch is safe.  However, I also 
pointed out the spots where we have latent bugs (on 32-bit machines 
only) if the callers are relaxed to pass larger than 2G or 4G.  As long 
as you are auditing things, it would be nice to either fix that in this 
patch, or document in the commit message how a future patch will address 
that latent problem (whether by enforcing max_transfer to be capped at 
2G on 32-bit platforms, or else fixing qemu_iovec to use int64_t instead 
of size_t bounds).

Reviewed-by: Eric Blake <eblake@redhat.com>

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