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 <vsement...@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 <ebl...@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
--
sheepdog mailing list
sheepdog@lists.wpkg.org
https://lists.wpkg.org/mailman/listinfo/sheepdog