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 <vsementsov@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)
{
BlockDriverState *bs = child->bs;
@@ -1906,7 +1906,7 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, uint64_t bytes,
}
static inline void coroutine_fn
-bdrv_co_write_req_finish(BdrvChild *child, int64_t offset, uint64_t bytes,
+bdrv_co_write_req_finish(BdrvChild *child, int64_t offset, int64_t bytes,
BdrvTrackedRequest *req, int ret)
{
int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
@@ -1948,14 +1948,14 @@ bdrv_co_write_req_finish(BdrvChild *child, int64_t offset, uint64_t bytes,
* after possibly fragmenting it.
*/
static int coroutine_fn bdrv_aligned_pwritev(BdrvChild *child,
- BdrvTrackedRequest *req, int64_t offset, unsigned int bytes,
+ BdrvTrackedRequest *req, int64_t offset, int64_t bytes,
int64_t align, QEMUIOVector *qiov, size_t qiov_offset, int flags)
{
BlockDriverState *bs = child->bs;
BlockDriver *drv = bs->drv;
int ret;
- uint64_t bytes_remaining = bytes;
+ int64_t bytes_remaining = bytes;
int max_transfer;
if (!drv) {
@@ -1967,6 +1967,8 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild *child,
}
assert(is_power_of_2(align));
+ assert(offset >= 0);
+ assert(bytes >= 0);
assert((offset & (align - 1)) == 0);
assert((bytes & (align - 1)) == 0);
assert(!qiov || qiov_offset + bytes <= qiov->size);
@@ -2067,7 +2069,7 @@ static int coroutine_fn bdrv_co_do_zero_pwritev(BdrvChild *child,
assert(!bytes || (offset & (align - 1)) == 0);
if (bytes >= align) {
/* Write the aligned part in the middle. */
- uint64_t aligned_bytes = bytes & ~(align - 1);
+ int64_t aligned_bytes = bytes & ~(align - 1);
ret = bdrv_aligned_pwritev(child, req, offset, aligned_bytes, align,
NULL, 0, flags);
if (ret < 0) {
--
2.21.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. 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 <vsementsov@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...
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
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 <vsementsov@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.
--
Best regards,
Vladimir
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 <vsementsov@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
--
Best regards,
Vladimir
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 <vsementsov@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
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 <vsementsov@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);
this variable used only for assertion, it's simple to refactor to avoid the problem.
> 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...
>
>
--
Best regards,
Vladimir
© 2016 - 2026 Red Hat, Inc.