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

[Vladimir Sementsov-Ogievskiy]

29.04.2020 22:27, Eric Blake wrote:
> 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?

fragmentation loop in io.c should have larger granularity than in block-copy.c, 
isn't it? Fragmentation loop in block-copy will be async for performance 
reasons, based on aio-task-pool. And it should cover both copy_range and simple 
copy and write-zeroes cases.. So, I think it's simpler to keep separate 
fragmentation loop in io.c. Still it's not really needed until block-copy is 
the only user of the interface.

> > -    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.


--
Best regards,
Vladimir