[sheepdog] [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 <vsementsov at virtuozzo.com>
>> ---
>>   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 <eblake at redhat.com>
> 
> 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