Re: [PATCH v2 03/17] block/io: use int64_t bytes parameter in bdrv_check_byte_request()

[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. Convert bdrv_check_byte_request() 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 7cbb80bd24..1267918def 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -875,9 +875,9 @@ static bool coroutine_fn 
> bdrv_wait_serialising_requests(BdrvTrackedRequest *self
>   }
>   
>   static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
> -                                   size_t size)
> +                                   int64_t bytes)
>   {

This changes an unsigned to signed value on 64-bit machines, and 
additionally widens the parameter on 32-bit machines.  Existing callers:

bdrv_co_preadv_part() with 'unsigned int' - no impact
bdrv_co_pwritev_part() with 'unsigned int' - no impact
bdrv_co_copy_range_internal() with 'uint64_t' - potentially fixes a 
latent bug on 32-bit machines. Requires a larger audit to see how 
bdrv_co_copy_range() and friends are used:

block/block-backend.c:blk_co_copy_range() - passes 'int', thus < 2G
block/block-copy.c:block_copy_do_copy() - passes 'int64_t', but only 
after assert(nbytes < INT_MAX); also it has a BLOCK_COPY_MAX_COPY_RANGE 
set to 16M that factors into its calculations on how much to do per 
iteration

So it looks like at present we are safe, but the commit message should 
definitely call out the fix for an unreachable latent bug.

I will also point out that on Linux, copy_file_range() is capped by a 
size_t len parameter, so even if we DO have a 32-bit caller passing > 
2G, it will encounter further truncation bugs if the block layer does 
not fragment the request internally.  I don't see any fragmentation 
logic in the public bdrv_co_copy_range() or in 
bdrv_co_copy_range_internal() - I suspect that needs to be added 
(probably as a separate patch); maybe by moving the fragmentation loop 
currently in block-copy.c to instead live in io.c?

> -    if (size > BDRV_REQUEST_MAX_BYTES) {
> +    if (bytes > BDRV_REQUEST_MAX_BYTES) {
>           return -EIO;
>       }
>   
> @@ -885,7 +885,7 @@ static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
>           return -ENOMEDIUM;
>       }
>   
> -    if (offset < 0) {
> +    if (offset < 0 || bytes < 0) {
>           return -EIO;
>       }

Reviewed-by: Eric Blake <address@hidden>

I don't know if we have any iotest coverage of copy_range with a 5G 
image to prove that it doesn't misbehave on a 32-bit machine.  That 
seems like it will eventually be useful, but doesn't necessarily have to 
be at the same time as this patch.

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