30.04.2020 8:30, Vladimir Sementsov-Ogievskiy wrote:
30.04.2020 8:25, Vladimir Sementsov-Ogievskiy wrote:
30.04.2020 1:04, 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. 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 <vsement...@virtuozzo.com>
---
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...
Thanks for reviewing!
I'm very sorry, I just need to say once again: the series should be rebased on
"[PATCH for-5.0? 0/9] block/io: safer inc/dec in_flight sections", as it is
already mostly reviewed by Stefan. Seems, that your analysis will be still valid after
it, although patches will change. I'll do it now to see, can I keep your r-b's.
I mean "[PATCH v2 0/9] block/io: safer inc/dec in_flight sections" of course
https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg04559.html
Cool! Exactly up to this patch (inclusive) it rebases without conflicts. And
the next patch (and may be further) are conflicting. I'll finish rebasing and
resend.
--
Best regards,
Vladimir
