[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 03/17] block/io: use int64_t bytes parameter in bdrv_check
From: |
Eric Blake |
Subject: |
Re: [PATCH v2 03/17] block/io: use int64_t bytes parameter in bdrv_check_byte_request() |
Date: |
Wed, 29 Apr 2020 14:27:37 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 |
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
[PATCH v2 05/17] block/io: support int64_t bytes in bdrv_co_do_pwrite_zeroes(), Vladimir Sementsov-Ogievskiy, 2020/04/27
[PATCH v2 03/17] block/io: use int64_t bytes parameter in bdrv_check_byte_request(), Vladimir Sementsov-Ogievskiy, 2020/04/27
- Re: [PATCH v2 03/17] block/io: use int64_t bytes parameter in bdrv_check_byte_request(),
Eric Blake <=
[PATCH v2 06/17] block/io: support int64_t bytes in bdrv_aligned_pwritev(), Vladimir Sementsov-Ogievskiy, 2020/04/27
Re: [PATCH v2 06/17] block/io: support int64_t bytes in bdrv_aligned_pwritev(), Vladimir Sementsov-Ogievskiy, 2020/04/30
[PATCH v2 08/17] block/io: support int64_t bytes in bdrv_aligned_preadv(), Vladimir Sementsov-Ogievskiy, 2020/04/27
[PATCH v2 07/17] block/io: support int64_t bytes in bdrv_co_do_copy_on_readv(), Vladimir Sementsov-Ogievskiy, 2020/04/27
[PATCH v2 09/17] block/io: support int64_t bytes in bdrv_co_p{read, write}v_part(), Vladimir Sementsov-Ogievskiy, 2020/04/27