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

[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_aligned_pwritev() now (and convert the
> dependencies: bdrv_co_write_req_prepare() and
> bdrv_co_write_req_finish() to signed type bytes)
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>   block/io.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index c8c30e3699..fe19e09034 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1854,7 +1854,7 @@ fail:
>   }
>   
>   static inline int coroutine_fn
> -bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, uint64_t bytes,
> +bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, int64_t bytes,
>                             BdrvTrackedRequest *req, int flags)
>   {

No change in size.  First, check usage within function:
    int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
Changes computation from uint64_t to int64_t.  This causes a borderline 
bug on images between INT64_MAX-511 and INT64_MAX (nbdkit can produce 
such images over NBD, although they are atypical on disk), where 
DIV_ROUND_UP() would give the right answer as uint64_t but a negative 
answer with int64_t.  As those images are not sector-aligned, maybe we 
don't need to care?
all other uses appear to be within asserts related to offset+bytes being 
positive, so that's what we should check for.

Callers:
bdrv_aligned_pwritev() - changed in this patch to 'int64_t', analyzed 
below [1]
bdrv_co_pdiscard() - already passes 'int64_t', also checks for 
offset+bytes overflow - safe
bdrv_co_copy_range_internal() - 'uint64_t', but already analyzed for 
3/17 how it was capped < 2M - safe
bdrv_co_truncate() - already passes 'int64_t', passes new_bytes computed 
by subtracting from a positive 'int64_t offset' - safe


[1] except I hit the end of my work day, so my analysis will have to 
continue tomorrow...


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