* [PATCH v5 0/9] parallels: Refactor the code of images checks and fix a bug
@ 2022-08-22 9:05 Alexander Ivanov
2022-08-22 9:05 ` [PATCH v5 1/9] parallels: Out of image offset in BAT leads to image inflation Alexander Ivanov
` (8 more replies)
0 siblings, 9 replies; 31+ messages in thread
From: Alexander Ivanov @ 2022-08-22 9:05 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, den, stefanha, vsementsov, kwolf, hreitz
Fix image inflation when offset in BAT is out of image.
Replace whole BAT syncing by flushing only dirty blocks.
Move all the checks outside the main check function in
separate functions
Use WITH_QEMU_LOCK_GUARD for simplier code.
v5:
2: Change the way of data_end fixing.
6,7: Move data_end check to parallels_check_leak().
v4:
1: Move s->data_end fix to parallels_co_check(). Split the check
in parallels_open() and the fix in parallels_co_check() to two patches.
2: A new patch - a part of the patch 1.
Add a fix for data_end to parallels_co_check().
3: Move offset convertation to parallels_set_bat_entry().
4: Fix 'ret' rewriting by bdrv_co_flush() results.
7: Keep 'i' as uint32_t.
v3:
1-8: Fix commit message.
v2:
2: A new patch - a part of the splitted patch 2.
3: Patch order was changed so the replacement is done in parallels_co_check.
Now we use a helper to set BAT entry and mark the block dirty.
4: Revert the condition with s->header_unclean.
5: Move unrelated helper parallels_set_bat_entry creation to a separate patch.
7: Move fragmentation counting code to this function too.
8: Fix an incorrect usage of WITH_QEMU_LOCK_GUARD.
Alexander Ivanov (9):
parallels: Out of image offset in BAT leads to image inflation
parallels: Fix data_end field value in parallels_co_check()
parallels: create parallels_set_bat_entry_helper() to assign BAT value
parallels: Use generic infrastructure for BAT writing in
parallels_co_check()
parallels: Move check of unclean image to a separate function
parallels: Move check of cluster outside image to a separate function
parallels: Move check of leaks to a separate function
parallels: Move statistic collection to a separate function
parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD
block/parallels.c | 194 +++++++++++++++++++++++++++++++++-------------
1 file changed, 138 insertions(+), 56 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v5 1/9] parallels: Out of image offset in BAT leads to image inflation
2022-08-22 9:05 [PATCH v5 0/9] parallels: Refactor the code of images checks and fix a bug Alexander Ivanov
@ 2022-08-22 9:05 ` Alexander Ivanov
2022-08-23 6:58 ` Vladimir Sementsov-Ogievskiy
2022-08-23 9:51 ` Denis V. Lunev
2022-08-22 9:05 ` [PATCH v5 2/9] parallels: Fix data_end field value in parallels_co_check() Alexander Ivanov
` (7 subsequent siblings)
8 siblings, 2 replies; 31+ messages in thread
From: Alexander Ivanov @ 2022-08-22 9:05 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, den, stefanha, vsementsov, kwolf, hreitz
data_end field in BDRVParallelsState is set to the biggest offset present
in BAT. If this offset is outside of the image, any further write will create
the cluster at this offset and/or the image will be truncated to this
offset on close. This is definitely not correct.
Raise an error in parallels_open() if data_end points outside the image and
it is not a check (let the check to repaire the image).
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
block/parallels.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index a229c06f25..c245ca35cd 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
BDRVParallelsState *s = bs->opaque;
ParallelsHeader ph;
int ret, size, i;
+ int64_t file_size;
QemuOpts *opts = NULL;
Error *local_err = NULL;
char *buf;
@@ -811,6 +812,19 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
}
}
+ file_size = bdrv_getlength(bs->file->bs);
+ if (file_size < 0) {
+ ret = file_size;
+ goto fail;
+ }
+
+ file_size >>= BDRV_SECTOR_BITS;
+ if (s->data_end > file_size && !(flags & BDRV_O_CHECK)) {
+ error_setg(errp, "parallels: Offset in BAT is out of image");
+ ret = -EINVAL;
+ goto fail;
+ }
+
if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
/* Image was not closed correctly. The check is mandatory */
s->header_unclean = true;
--
2.34.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v5 2/9] parallels: Fix data_end field value in parallels_co_check()
2022-08-22 9:05 [PATCH v5 0/9] parallels: Refactor the code of images checks and fix a bug Alexander Ivanov
2022-08-22 9:05 ` [PATCH v5 1/9] parallels: Out of image offset in BAT leads to image inflation Alexander Ivanov
@ 2022-08-22 9:05 ` Alexander Ivanov
2022-08-23 7:38 ` Vladimir Sementsov-Ogievskiy
2022-08-23 9:23 ` Denis V. Lunev
2022-08-22 9:05 ` [PATCH v5 3/9] parallels: create parallels_set_bat_entry_helper() to assign BAT value Alexander Ivanov
` (6 subsequent siblings)
8 siblings, 2 replies; 31+ messages in thread
From: Alexander Ivanov @ 2022-08-22 9:05 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, den, stefanha, vsementsov, kwolf, hreitz
Make data_end pointing to the end of the last cluster if a leak was fixed.
Otherwise set the file size to data_end.
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
block/parallels.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/block/parallels.c b/block/parallels.c
index c245ca35cd..2be56871bc 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -513,7 +513,15 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
res->leaks_fixed += count;
}
}
-
+ /*
+ * If res->image_end_offset less than the file size,
+ * a leak was fixed and we should set the new offset to s->data_end.
+ * Otherwise set the file size to s->data_end.
+ */
+ if (res->image_end_offset < size && fix & BDRV_FIX_LEAKS) {
+ size = res->image_end_offset;
+ }
+ s->data_end = size >> BDRV_SECTOR_BITS;
out:
qemu_co_mutex_unlock(&s->lock);
return ret;
--
2.34.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v5 3/9] parallels: create parallels_set_bat_entry_helper() to assign BAT value
2022-08-22 9:05 [PATCH v5 0/9] parallels: Refactor the code of images checks and fix a bug Alexander Ivanov
2022-08-22 9:05 ` [PATCH v5 1/9] parallels: Out of image offset in BAT leads to image inflation Alexander Ivanov
2022-08-22 9:05 ` [PATCH v5 2/9] parallels: Fix data_end field value in parallels_co_check() Alexander Ivanov
@ 2022-08-22 9:05 ` Alexander Ivanov
2022-08-22 9:05 ` [PATCH v5 4/9] parallels: Use generic infrastructure for BAT writing in parallels_co_check() Alexander Ivanov
` (5 subsequent siblings)
8 siblings, 0 replies; 31+ messages in thread
From: Alexander Ivanov @ 2022-08-22 9:05 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, den, stefanha, vsementsov, kwolf, hreitz
This helper will be reused in next patches during parallels_co_check
rework to simplify its code.
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
block/parallels.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 2be56871bc..8733ef8a70 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -165,6 +165,13 @@ static int64_t block_status(BDRVParallelsState *s, int64_t sector_num,
return start_off;
}
+static void parallels_set_bat_entry(BDRVParallelsState *s,
+ uint32_t index, uint32_t offset)
+{
+ s->bat_bitmap[index] = cpu_to_le32(offset);
+ bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / s->bat_dirty_block, 1);
+}
+
static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, int *pnum)
{
@@ -250,10 +257,8 @@ static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num,
}
for (i = 0; i < to_allocate; i++) {
- s->bat_bitmap[idx + i] = cpu_to_le32(s->data_end / s->off_multiplier);
+ parallels_set_bat_entry(s, idx + i, s->data_end / s->off_multiplier);
s->data_end += s->tracks;
- bitmap_set(s->bat_dirty_bmap,
- bat_entry_off(idx + i) / s->bat_dirty_block, 1);
}
return bat2sect(s, idx) + sector_num % s->tracks;
--
2.34.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v5 4/9] parallels: Use generic infrastructure for BAT writing in parallels_co_check()
2022-08-22 9:05 [PATCH v5 0/9] parallels: Refactor the code of images checks and fix a bug Alexander Ivanov
` (2 preceding siblings ...)
2022-08-22 9:05 ` [PATCH v5 3/9] parallels: create parallels_set_bat_entry_helper() to assign BAT value Alexander Ivanov
@ 2022-08-22 9:05 ` Alexander Ivanov
2022-08-23 9:36 ` Denis V. Lunev
2022-08-22 9:05 ` [PATCH v5 5/9] parallels: Move check of unclean image to a separate function Alexander Ivanov
` (4 subsequent siblings)
8 siblings, 1 reply; 31+ messages in thread
From: Alexander Ivanov @ 2022-08-22 9:05 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, den, stefanha, vsementsov, kwolf, hreitz
BAT is written in the context of conventional operations over
the image inside bdrv_co_flush() when it calls
parallels_co_flush_to_os() callback. Thus we should not
modify BAT array directly, but call parallels_set_bat_entry()
helper and bdrv_co_flush() further on. After that there is no
need to manually write BAT and track its modification.
This makes code more generic and allows to split
parallels_set_bat_entry() for independent pieces.
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
block/parallels.c | 23 ++++++++++-------------
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 8733ef8a70..357c345517 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -425,9 +425,8 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
{
BDRVParallelsState *s = bs->opaque;
int64_t size, prev_off, high_off;
- int ret;
+ int ret = 0;
uint32_t i;
- bool flush_bat = false;
size = bdrv_getlength(bs->file->bs);
if (size < 0) {
@@ -466,9 +465,8 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
res->corruptions++;
if (fix & BDRV_FIX_ERRORS) {
prev_off = 0;
- s->bat_bitmap[i] = 0;
+ parallels_set_bat_entry(s, i, 0);
res->corruptions_fixed++;
- flush_bat = true;
continue;
}
}
@@ -484,15 +482,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
prev_off = off;
}
- ret = 0;
- if (flush_bat) {
- ret = bdrv_co_pwrite_sync(bs->file, 0, s->header_size, s->header, 0);
- if (ret < 0) {
- res->check_errors++;
- goto out;
- }
- }
-
res->image_end_offset = high_off + s->cluster_size;
if (size > res->image_end_offset) {
int64_t count;
@@ -529,6 +518,14 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
s->data_end = size >> BDRV_SECTOR_BITS;
out:
qemu_co_mutex_unlock(&s->lock);
+
+ if (ret == 0) {
+ ret = bdrv_co_flush(bs);
+ if (ret < 0) {
+ res->check_errors++;
+ }
+ }
+
return ret;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v5 5/9] parallels: Move check of unclean image to a separate function
2022-08-22 9:05 [PATCH v5 0/9] parallels: Refactor the code of images checks and fix a bug Alexander Ivanov
` (3 preceding siblings ...)
2022-08-22 9:05 ` [PATCH v5 4/9] parallels: Use generic infrastructure for BAT writing in parallels_co_check() Alexander Ivanov
@ 2022-08-22 9:05 ` Alexander Ivanov
2022-08-22 9:05 ` [PATCH v5 6/9] parallels: Move check of cluster outside " Alexander Ivanov
` (3 subsequent siblings)
8 siblings, 0 replies; 31+ messages in thread
From: Alexander Ivanov @ 2022-08-22 9:05 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, den, stefanha, vsementsov, kwolf, hreitz
We will add more and more checks so we need a better code structure
in parallels_co_check. Let each check performs in a separate loop
in a separate helper.
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
block/parallels.c | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 357c345517..a35035e231 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -418,6 +418,25 @@ static coroutine_fn int parallels_co_readv(BlockDriverState *bs,
return ret;
}
+static void parallels_check_unclean(BlockDriverState *bs,
+ BdrvCheckResult *res,
+ BdrvCheckMode fix)
+{
+ BDRVParallelsState *s = bs->opaque;
+
+ if (!s->header_unclean) {
+ return;
+ }
+
+ fprintf(stderr, "%s image was not closed correctly\n",
+ fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
+ res->corruptions++;
+ if (fix & BDRV_FIX_ERRORS) {
+ /* parallels_close will do the job right */
+ res->corruptions_fixed++;
+ s->header_unclean = false;
+ }
+}
static int coroutine_fn parallels_co_check(BlockDriverState *bs,
BdrvCheckResult *res,
@@ -435,16 +454,8 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
}
qemu_co_mutex_lock(&s->lock);
- if (s->header_unclean) {
- fprintf(stderr, "%s image was not closed correctly\n",
- fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
- res->corruptions++;
- if (fix & BDRV_FIX_ERRORS) {
- /* parallels_close will do the job right */
- res->corruptions_fixed++;
- s->header_unclean = false;
- }
- }
+
+ parallels_check_unclean(bs, res, fix);
res->bfi.total_clusters = s->bat_size;
res->bfi.compressed_clusters = 0; /* compression is not supported */
--
2.34.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v5 6/9] parallels: Move check of cluster outside image to a separate function
2022-08-22 9:05 [PATCH v5 0/9] parallels: Refactor the code of images checks and fix a bug Alexander Ivanov
` (4 preceding siblings ...)
2022-08-22 9:05 ` [PATCH v5 5/9] parallels: Move check of unclean image to a separate function Alexander Ivanov
@ 2022-08-22 9:05 ` Alexander Ivanov
2022-08-22 9:05 ` [PATCH v5 7/9] parallels: Move check of leaks " Alexander Ivanov
` (2 subsequent siblings)
8 siblings, 0 replies; 31+ messages in thread
From: Alexander Ivanov @ 2022-08-22 9:05 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, den, stefanha, vsementsov, kwolf, hreitz
We will add more and more checks so we need a better code structure
in parallels_co_check. Let each check performs in a separate loop
in a separate helper.
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
block/parallels.c | 50 ++++++++++++++++++++++++++++++++++-------------
1 file changed, 36 insertions(+), 14 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index a35035e231..bf074f247c 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -438,13 +438,43 @@ static void parallels_check_unclean(BlockDriverState *bs,
}
}
+static int parallels_check_outside_image(BlockDriverState *bs,
+ BdrvCheckResult *res,
+ BdrvCheckMode fix)
+{
+ BDRVParallelsState *s = bs->opaque;
+ uint32_t i;
+ int64_t off, size;
+
+ size = bdrv_getlength(bs->file->bs);
+ if (size < 0) {
+ res->check_errors++;
+ return size;
+ }
+
+ for (i = 0; i < s->bat_size; i++) {
+ off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+ if (off > size) {
+ fprintf(stderr, "%s cluster %u is outside image\n",
+ fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
+ res->corruptions++;
+ if (fix & BDRV_FIX_ERRORS) {
+ parallels_set_bat_entry(s, i, 0);
+ res->corruptions_fixed++;
+ }
+ }
+ }
+
+ return 0;
+}
+
static int coroutine_fn parallels_co_check(BlockDriverState *bs,
BdrvCheckResult *res,
BdrvCheckMode fix)
{
BDRVParallelsState *s = bs->opaque;
int64_t size, prev_off, high_off;
- int ret = 0;
+ int ret;
uint32_t i;
size = bdrv_getlength(bs->file->bs);
@@ -457,6 +487,11 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
parallels_check_unclean(bs, res, fix);
+ ret = parallels_check_outside_image(bs, res, fix);
+ if (ret < 0) {
+ goto out;
+ }
+
res->bfi.total_clusters = s->bat_size;
res->bfi.compressed_clusters = 0; /* compression is not supported */
@@ -469,19 +504,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
continue;
}
- /* cluster outside the image */
- if (off > size) {
- fprintf(stderr, "%s cluster %u is outside image\n",
- fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
- res->corruptions++;
- if (fix & BDRV_FIX_ERRORS) {
- prev_off = 0;
- parallels_set_bat_entry(s, i, 0);
- res->corruptions_fixed++;
- continue;
- }
- }
-
res->bfi.allocated_clusters++;
if (off > high_off) {
high_off = off;
--
2.34.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v5 7/9] parallels: Move check of leaks to a separate function
2022-08-22 9:05 [PATCH v5 0/9] parallels: Refactor the code of images checks and fix a bug Alexander Ivanov
` (5 preceding siblings ...)
2022-08-22 9:05 ` [PATCH v5 6/9] parallels: Move check of cluster outside " Alexander Ivanov
@ 2022-08-22 9:05 ` Alexander Ivanov
2022-08-23 9:40 ` Denis V. Lunev
2022-08-22 9:05 ` [PATCH v5 8/9] parallels: Move statistic collection " Alexander Ivanov
2022-08-22 9:05 ` [PATCH v5 9/9] parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD Alexander Ivanov
8 siblings, 1 reply; 31+ messages in thread
From: Alexander Ivanov @ 2022-08-22 9:05 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, den, stefanha, vsementsov, kwolf, hreitz
We will add more and more checks so we need a better code structure
in parallels_co_check. Let each check performs in a separate loop
in a separate helper.
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
block/parallels.c | 87 +++++++++++++++++++++++++++++------------------
1 file changed, 54 insertions(+), 33 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index bf074f247c..12e8d382f3 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -468,14 +468,13 @@ static int parallels_check_outside_image(BlockDriverState *bs,
return 0;
}
-static int coroutine_fn parallels_co_check(BlockDriverState *bs,
- BdrvCheckResult *res,
- BdrvCheckMode fix)
+static int parallels_check_leak(BlockDriverState *bs,
+ BdrvCheckResult *res,
+ BdrvCheckMode fix)
{
BDRVParallelsState *s = bs->opaque;
- int64_t size, prev_off, high_off;
- int ret;
- uint32_t i;
+ int64_t size, off, high_off, count;
+ int i, ret;
size = bdrv_getlength(bs->file->bs);
if (size < 0) {
@@ -483,41 +482,16 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
return size;
}
- qemu_co_mutex_lock(&s->lock);
-
- parallels_check_unclean(bs, res, fix);
-
- ret = parallels_check_outside_image(bs, res, fix);
- if (ret < 0) {
- goto out;
- }
-
- res->bfi.total_clusters = s->bat_size;
- res->bfi.compressed_clusters = 0; /* compression is not supported */
-
high_off = 0;
- prev_off = 0;
for (i = 0; i < s->bat_size; i++) {
- int64_t off = bat2sect(s, i) << BDRV_SECTOR_BITS;
- if (off == 0) {
- prev_off = 0;
- continue;
- }
-
- res->bfi.allocated_clusters++;
+ off = bat2sect(s, i) << BDRV_SECTOR_BITS;
if (off > high_off) {
high_off = off;
}
-
- if (prev_off != 0 && (prev_off + s->cluster_size) != off) {
- res->bfi.fragmented_clusters++;
- }
- prev_off = off;
}
res->image_end_offset = high_off + s->cluster_size;
if (size > res->image_end_offset) {
- int64_t count;
count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
@@ -535,11 +509,12 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
if (ret < 0) {
error_report_err(local_err);
res->check_errors++;
- goto out;
+ return ret;
}
res->leaks_fixed += count;
}
}
+
/*
* If res->image_end_offset less than the file size,
* a leak was fixed and we should set the new offset to s->data_end.
@@ -549,6 +524,52 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
size = res->image_end_offset;
}
s->data_end = size >> BDRV_SECTOR_BITS;
+
+ return 0;
+}
+
+static int coroutine_fn parallels_co_check(BlockDriverState *bs,
+ BdrvCheckResult *res,
+ BdrvCheckMode fix)
+{
+ BDRVParallelsState *s = bs->opaque;
+ int64_t prev_off;
+ int ret;
+ uint32_t i;
+
+ qemu_co_mutex_lock(&s->lock);
+
+ parallels_check_unclean(bs, res, fix);
+
+ ret = parallels_check_outside_image(bs, res, fix);
+ if (ret < 0) {
+ goto out;
+ }
+
+ ret = parallels_check_leak(bs, res, fix);
+ if (ret < 0) {
+ goto out;
+ }
+
+ res->bfi.total_clusters = s->bat_size;
+ res->bfi.compressed_clusters = 0; /* compression is not supported */
+
+ prev_off = 0;
+ for (i = 0; i < s->bat_size; i++) {
+ int64_t off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+ if (off == 0) {
+ prev_off = 0;
+ continue;
+ }
+
+ res->bfi.allocated_clusters++;
+
+ if (prev_off != 0 && (prev_off + s->cluster_size) != off) {
+ res->bfi.fragmented_clusters++;
+ }
+ prev_off = off;
+ }
+
out:
qemu_co_mutex_unlock(&s->lock);
--
2.34.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v5 8/9] parallels: Move statistic collection to a separate function
2022-08-22 9:05 [PATCH v5 0/9] parallels: Refactor the code of images checks and fix a bug Alexander Ivanov
` (6 preceding siblings ...)
2022-08-22 9:05 ` [PATCH v5 7/9] parallels: Move check of leaks " Alexander Ivanov
@ 2022-08-22 9:05 ` Alexander Ivanov
2022-08-22 9:05 ` [PATCH v5 9/9] parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD Alexander Ivanov
8 siblings, 0 replies; 31+ messages in thread
From: Alexander Ivanov @ 2022-08-22 9:05 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, den, stefanha, vsementsov, kwolf, hreitz
We will add more and more checks so we need a better code structure
in parallels_co_check. Let each check performs in a separate loop
in a separate helper.
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
block/parallels.c | 53 +++++++++++++++++++++++++++--------------------
1 file changed, 31 insertions(+), 22 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 12e8d382f3..0fe6dd3237 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -528,47 +528,56 @@ static int parallels_check_leak(BlockDriverState *bs,
return 0;
}
-static int coroutine_fn parallels_co_check(BlockDriverState *bs,
- BdrvCheckResult *res,
- BdrvCheckMode fix)
+static void parallels_collect_statistics(BlockDriverState *bs,
+ BdrvCheckResult *res,
+ BdrvCheckMode fix)
{
BDRVParallelsState *s = bs->opaque;
- int64_t prev_off;
- int ret;
+ int64_t off, prev_off;
uint32_t i;
- qemu_co_mutex_lock(&s->lock);
-
- parallels_check_unclean(bs, res, fix);
-
- ret = parallels_check_outside_image(bs, res, fix);
- if (ret < 0) {
- goto out;
- }
-
- ret = parallels_check_leak(bs, res, fix);
- if (ret < 0) {
- goto out;
- }
-
res->bfi.total_clusters = s->bat_size;
res->bfi.compressed_clusters = 0; /* compression is not supported */
prev_off = 0;
for (i = 0; i < s->bat_size; i++) {
- int64_t off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+ off = bat2sect(s, i) << BDRV_SECTOR_BITS;
if (off == 0) {
prev_off = 0;
continue;
}
- res->bfi.allocated_clusters++;
-
if (prev_off != 0 && (prev_off + s->cluster_size) != off) {
res->bfi.fragmented_clusters++;
}
+
prev_off = off;
+ res->bfi.allocated_clusters++;
}
+}
+
+static int coroutine_fn parallels_co_check(BlockDriverState *bs,
+ BdrvCheckResult *res,
+ BdrvCheckMode fix)
+{
+ BDRVParallelsState *s = bs->opaque;
+ int ret;
+
+ qemu_co_mutex_lock(&s->lock);
+
+ parallels_check_unclean(bs, res, fix);
+
+ ret = parallels_check_outside_image(bs, res, fix);
+ if (ret < 0) {
+ goto out;
+ }
+
+ ret = parallels_check_leak(bs, res, fix);
+ if (ret < 0) {
+ goto out;
+ }
+
+ parallels_collect_statistics(bs, res, fix);
out:
qemu_co_mutex_unlock(&s->lock);
--
2.34.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v5 9/9] parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD
2022-08-22 9:05 [PATCH v5 0/9] parallels: Refactor the code of images checks and fix a bug Alexander Ivanov
` (7 preceding siblings ...)
2022-08-22 9:05 ` [PATCH v5 8/9] parallels: Move statistic collection " Alexander Ivanov
@ 2022-08-22 9:05 ` Alexander Ivanov
8 siblings, 0 replies; 31+ messages in thread
From: Alexander Ivanov @ 2022-08-22 9:05 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, den, stefanha, vsementsov, kwolf, hreitz
Replace the way we use mutex in parallels_co_check() for simplier
and less error prone code.
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
---
block/parallels.c | 33 ++++++++++++++-------------------
1 file changed, 14 insertions(+), 19 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 0fe6dd3237..47ef25b193 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -563,30 +563,25 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
BDRVParallelsState *s = bs->opaque;
int ret;
- qemu_co_mutex_lock(&s->lock);
+ WITH_QEMU_LOCK_GUARD(&s->lock) {
+ parallels_check_unclean(bs, res, fix);
- parallels_check_unclean(bs, res, fix);
+ ret = parallels_check_outside_image(bs, res, fix);
+ if (ret < 0) {
+ return ret;
+ }
- ret = parallels_check_outside_image(bs, res, fix);
- if (ret < 0) {
- goto out;
- }
+ ret = parallels_check_leak(bs, res, fix);
+ if (ret < 0) {
+ return ret;
+ }
- ret = parallels_check_leak(bs, res, fix);
- if (ret < 0) {
- goto out;
+ parallels_collect_statistics(bs, res, fix);
}
- parallels_collect_statistics(bs, res, fix);
-
-out:
- qemu_co_mutex_unlock(&s->lock);
-
- if (ret == 0) {
- ret = bdrv_co_flush(bs);
- if (ret < 0) {
- res->check_errors++;
- }
+ ret = bdrv_co_flush(bs);
+ if (ret < 0) {
+ res->check_errors++;
}
return ret;
--
2.34.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] parallels: Out of image offset in BAT leads to image inflation
2022-08-22 9:05 ` [PATCH v5 1/9] parallels: Out of image offset in BAT leads to image inflation Alexander Ivanov
@ 2022-08-23 6:58 ` Vladimir Sementsov-Ogievskiy
2022-08-23 7:23 ` Alexander Ivanov
2022-08-23 9:34 ` Denis V. Lunev
2022-08-23 9:51 ` Denis V. Lunev
1 sibling, 2 replies; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-08-23 6:58 UTC (permalink / raw)
To: Alexander Ivanov, qemu-devel; +Cc: qemu-block, den, stefanha, kwolf, hreitz
On 8/22/22 12:05, Alexander Ivanov wrote:
> data_end field in BDRVParallelsState is set to the biggest offset present
> in BAT. If this offset is outside of the image, any further write will create
> the cluster at this offset and/or the image will be truncated to this
> offset on close. This is definitely not correct.
> Raise an error in parallels_open() if data_end points outside the image and
> it is not a check (let the check to repaire the image).
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
> block/parallels.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index a229c06f25..c245ca35cd 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
> BDRVParallelsState *s = bs->opaque;
> ParallelsHeader ph;
> int ret, size, i;
> + int64_t file_size;
> QemuOpts *opts = NULL;
> Error *local_err = NULL;
> char *buf;
> @@ -811,6 +812,19 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
> }
> }
>
> + file_size = bdrv_getlength(bs->file->bs);
> + if (file_size < 0) {
> + ret = file_size;
> + goto fail;
> + }
> +
> + file_size >>= BDRV_SECTOR_BITS;
> + if (s->data_end > file_size && !(flags & BDRV_O_CHECK)) {
> + error_setg(errp, "parallels: Offset in BAT is out of image");
> + ret = -EINVAL;
> + goto fail;
> + }
If image is unaligned to sector size, and image size is less than s->data_end, but the difference itself is less than sector, the error message would be misleading.
Should we consider "file_size = DIV_ROUND_UP(file_size, BDRV_SECTOR_SIZE)" instead of "file_size >>= BDRV_SECTOR_BITS"?
It's hardly possible to get such image on valid scenarios with Qemu (keeping in mind bdrv_truncate() call in parallels_close()). But it still may be possible to have such images produced by another software or by some failure path.
> +
> if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> /* Image was not closed correctly. The check is mandatory */
> s->header_unclean = true;
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] parallels: Out of image offset in BAT leads to image inflation
2022-08-23 6:58 ` Vladimir Sementsov-Ogievskiy
@ 2022-08-23 7:23 ` Alexander Ivanov
2022-08-23 7:59 ` Vladimir Sementsov-Ogievskiy
2022-08-23 9:20 ` Denis V. Lunev
2022-08-23 9:34 ` Denis V. Lunev
1 sibling, 2 replies; 31+ messages in thread
From: Alexander Ivanov @ 2022-08-23 7:23 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: qemu-block, den, stefanha, kwolf, hreitz
On 23.08.2022 08:58, Vladimir Sementsov-Ogievskiy wrote:
> On 8/22/22 12:05, Alexander Ivanov wrote:
>> data_end field in BDRVParallelsState is set to the biggest offset
>> present
>> in BAT. If this offset is outside of the image, any further write
>> will create
>> the cluster at this offset and/or the image will be truncated to this
>> offset on close. This is definitely not correct.
>> Raise an error in parallels_open() if data_end points outside the
>> image and
>> it is not a check (let the check to repaire the image).
>>
>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>> ---
>> block/parallels.c | 14 ++++++++++++++
>> 1 file changed, 14 insertions(+)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index a229c06f25..c245ca35cd 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs,
>> QDict *options, int flags,
>> BDRVParallelsState *s = bs->opaque;
>> ParallelsHeader ph;
>> int ret, size, i;
>> + int64_t file_size;
>> QemuOpts *opts = NULL;
>> Error *local_err = NULL;
>> char *buf;
>> @@ -811,6 +812,19 @@ static int parallels_open(BlockDriverState *bs,
>> QDict *options, int flags,
>> }
>> }
>> + file_size = bdrv_getlength(bs->file->bs);
>> + if (file_size < 0) {
>> + ret = file_size;
>> + goto fail;
>> + }
>> +
>> + file_size >>= BDRV_SECTOR_BITS;
>> + if (s->data_end > file_size && !(flags & BDRV_O_CHECK)) {
>> + error_setg(errp, "parallels: Offset in BAT is out of image");
>> + ret = -EINVAL;
>> + goto fail;
>> + }
>
> If image is unaligned to sector size, and image size is less than
> s->data_end, but the difference itself is less than sector, the error
> message would be misleading.
>
> Should we consider "file_size = DIV_ROUND_UP(file_size,
> BDRV_SECTOR_SIZE)" instead of "file_size >>= BDRV_SECTOR_BITS"?
>
> It's hardly possible to get such image on valid scenarios with Qemu
> (keeping in mind bdrv_truncate() call in parallels_close()). But it
> still may be possible to have such images produced by another software
> or by some failure path.
>
I think you are right, it would be better to align image size up to
sector size.
>
>> +
>> if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
>> /* Image was not closed correctly. The check is mandatory */
>> s->header_unclean = true;
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 2/9] parallels: Fix data_end field value in parallels_co_check()
2022-08-22 9:05 ` [PATCH v5 2/9] parallels: Fix data_end field value in parallels_co_check() Alexander Ivanov
@ 2022-08-23 7:38 ` Vladimir Sementsov-Ogievskiy
2022-08-23 9:00 ` Alexander Ivanov
2022-08-23 9:23 ` Denis V. Lunev
1 sibling, 1 reply; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-08-23 7:38 UTC (permalink / raw)
To: Alexander Ivanov, qemu-devel; +Cc: qemu-block, den, stefanha, kwolf, hreitz
On 8/22/22 12:05, Alexander Ivanov wrote:
> Make data_end pointing to the end of the last cluster if a leak was fixed.
> Otherwise set the file size to data_end.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
> block/parallels.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index c245ca35cd..2be56871bc 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -513,7 +513,15 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
> res->leaks_fixed += count;
> }
> }
> -
> + /*
> + * If res->image_end_offset less than the file size,
> + * a leak was fixed and we should set the new offset to s->data_end.
> + * Otherwise set the file size to s->data_end.
I'm not sure about English :) For me "set A to B" means A := B, but you use it visa versa..
> + */
> + if (res->image_end_offset < size && fix & BDRV_FIX_LEAKS) {
> + size = res->image_end_offset;
> + }
> + s->data_end = size >> BDRV_SECTOR_BITS;
Hmm, actually, when size < data_end, you can shrink data_end only if (fix & BDRV_FIX_ERRORS), otherwise BAT is still not fixed.
> out:
> qemu_co_mutex_unlock(&s->lock);
> return ret;
Actually I think we only need to modify s->data_end after successful BAT fixing above. Then, bdrv_truncate is formally unrelated to s->data_end and shouldn't touch it.
So, I think, more correct is something like
diff --git a/block/parallels.c b/block/parallels.c
index 2be56871bc..b268788974 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -479,20 +479,24 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
prev_off = off;
}
ret = 0;
if (flush_bat) {
ret = bdrv_co_pwrite_sync(bs->file, 0, s->header_size, s->header, 0);
if (ret < 0) {
res->check_errors++;
goto out;
}
+
+ /* We have dropped some wrong clusters, update data_end */
+ assert(s->data_end * BDRV_SECTOR_SIZE >= high_off + s->cluster_size);
+ s->data_end = (high_off + s->cluster_size) / BDRV_SECTOR_SIZE;
}
res->image_end_offset = high_off + s->cluster_size;
if (size > res->image_end_offset) {
--
Best regards,
Vladimir
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] parallels: Out of image offset in BAT leads to image inflation
2022-08-23 7:23 ` Alexander Ivanov
@ 2022-08-23 7:59 ` Vladimir Sementsov-Ogievskiy
2022-08-23 9:45 ` Vladimir Sementsov-Ogievskiy
2022-08-23 9:20 ` Denis V. Lunev
1 sibling, 1 reply; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-08-23 7:59 UTC (permalink / raw)
To: Alexander Ivanov, qemu-devel; +Cc: qemu-block, den, stefanha, kwolf, hreitz
On 8/23/22 10:23, Alexander Ivanov wrote:
>
> On 23.08.2022 08:58, Vladimir Sementsov-Ogievskiy wrote:
>> On 8/22/22 12:05, Alexander Ivanov wrote:
>>> data_end field in BDRVParallelsState is set to the biggest offset present
>>> in BAT. If this offset is outside of the image, any further write will create
>>> the cluster at this offset and/or the image will be truncated to this
>>> offset on close. This is definitely not correct.
Actually, it's correct, as don't break the spec at docs/interop/parallels.txt.
So, it's bad but still correct.. So, we probably can restrict it in Qemu if we want. Do we?
Now I doubt. We are going to refuse images with leaks even for read. That means, qemu-img convert will stop working for images with leaks, you'll have to fix the image first, which may fail.
>>> Raise an error in parallels_open() if data_end points outside the image and
>>> it is not a check (let the check to repaire the image).
>>>
>>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>>> ---
>>> block/parallels.c | 14 ++++++++++++++
>>> 1 file changed, 14 insertions(+)
>>>
>>> diff --git a/block/parallels.c b/block/parallels.c
>>> index a229c06f25..c245ca35cd 100644
>>> --- a/block/parallels.c
>>> +++ b/block/parallels.c
>>> @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>> BDRVParallelsState *s = bs->opaque;
>>> ParallelsHeader ph;
>>> int ret, size, i;
>>> + int64_t file_size;
>>> QemuOpts *opts = NULL;
>>> Error *local_err = NULL;
>>> char *buf;
>>> @@ -811,6 +812,19 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>> }
>>> }
>>> + file_size = bdrv_getlength(bs->file->bs);
>>> + if (file_size < 0) {
>>> + ret = file_size;
>>> + goto fail;
>>> + }
>>> +
>>> + file_size >>= BDRV_SECTOR_BITS;
>>> + if (s->data_end > file_size && !(flags & BDRV_O_CHECK)) {
>>> + error_setg(errp, "parallels: Offset in BAT is out of image");
>>> + ret = -EINVAL;
>>> + goto fail;
>>> + }
>>
>> If image is unaligned to sector size, and image size is less than s->data_end, but the difference itself is less than sector, the error message would be misleading.
>>
>> Should we consider "file_size = DIV_ROUND_UP(file_size, BDRV_SECTOR_SIZE)" instead of "file_size >>= BDRV_SECTOR_BITS"?
>>
>> It's hardly possible to get such image on valid scenarios with Qemu (keeping in mind bdrv_truncate() call in parallels_close()). But it still may be possible to have such images produced by another software or by some failure path.
>>
> I think you are right, it would be better to align image size up to sector size.
Hmm, or even to cluster_size? When last cluster is unallocated. But cluster boundary is not required to be aligned to cluster size..
Anyway, now I think it's wrong to restrict opening file with leaks of any kind. That breaks qemu-img convert, and that's not how other formats behave.
>>
>>> +
>>> if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
>>> /* Image was not closed correctly. The check is mandatory */
>>> s->header_unclean = true;
>>
>>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 2/9] parallels: Fix data_end field value in parallels_co_check()
2022-08-23 7:38 ` Vladimir Sementsov-Ogievskiy
@ 2022-08-23 9:00 ` Alexander Ivanov
0 siblings, 0 replies; 31+ messages in thread
From: Alexander Ivanov @ 2022-08-23 9:00 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: qemu-block, den, stefanha, kwolf, hreitz
On 23.08.2022 09:38, Vladimir Sementsov-Ogievskiy wrote:
> On 8/22/22 12:05, Alexander Ivanov wrote:
>> Make data_end pointing to the end of the last cluster if a leak was
>> fixed.
>> Otherwise set the file size to data_end.
>>
>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>> ---
>> block/parallels.c | 10 +++++++++-
>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index c245ca35cd..2be56871bc 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -513,7 +513,15 @@ static int coroutine_fn
>> parallels_co_check(BlockDriverState *bs,
>> res->leaks_fixed += count;
>> }
>> }
>> -
>> + /*
>> + * If res->image_end_offset less than the file size,
>> + * a leak was fixed and we should set the new offset to
>> s->data_end.
>> + * Otherwise set the file size to s->data_end.
>
> I'm not sure about English :) For me "set A to B" means A := B, but
> you use it visa versa..
I hesitated about this. I saw both variants and used the incorrect one
probably. =)
>
>> + */
>> + if (res->image_end_offset < size && fix & BDRV_FIX_LEAKS) {
>> + size = res->image_end_offset;
>> + }
>> + s->data_end = size >> BDRV_SECTOR_BITS;
>
> Hmm, actually, when size < data_end, you can shrink data_end only if
> (fix & BDRV_FIX_ERRORS), otherwise BAT is still not fixed.
We shrink an image if there is a leak and (fix & BDRV_FIX_LEAKS).
Otherwise we set data_end to the file size. In such a way we don't
change the file size in parallels_close() even if we have out-of-image
offsets in BAT.
>
>> out:
>> qemu_co_mutex_unlock(&s->lock);
>> return ret;
>
>
> Actually I think we only need to modify s->data_end after successful
> BAT fixing above. Then, bdrv_truncate is formally unrelated to
> s->data_end and shouldn't touch it.
>
> So, I think, more correct is something like
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 2be56871bc..b268788974 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -479,20 +479,24 @@ static int coroutine_fn
> parallels_co_check(BlockDriverState *bs,
> prev_off = off;
> }
>
> ret = 0;
> if (flush_bat) {
> ret = bdrv_co_pwrite_sync(bs->file, 0, s->header_size,
> s->header, 0);
> if (ret < 0) {
> res->check_errors++;
> goto out;
> }
> +
> + /* We have dropped some wrong clusters, update data_end */
> + assert(s->data_end * BDRV_SECTOR_SIZE >= high_off +
> s->cluster_size);
> + s->data_end = (high_off + s->cluster_size) / BDRV_SECTOR_SIZE;
> }
>
> res->image_end_offset = high_off + s->cluster_size;
> if (size > res->image_end_offset) {
>
If an image was opened in RW mode for checking without fixing (I don't
know if it's possible), data_end can be set outside the image and the
image will be expanded in parallels_close().
OK, I would propose to fix data_end in two places:
1) after out-of-image check set data_end to the highest offset with (off
+ cluster_size <= file_size) condition, independent on fix flags;
2) after leak check, only if (size > res->image_end_offset).
In such a way we can have only one inconsistence after any check:
out-of-image offsets isn't fixed and we have offsets after data_end. But
it is much better than if we set data_end to petabytes, for example.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] parallels: Out of image offset in BAT leads to image inflation
2022-08-23 7:23 ` Alexander Ivanov
2022-08-23 7:59 ` Vladimir Sementsov-Ogievskiy
@ 2022-08-23 9:20 ` Denis V. Lunev
2022-08-23 9:49 ` Alexander Ivanov
2022-08-23 9:58 ` Vladimir Sementsov-Ogievskiy
1 sibling, 2 replies; 31+ messages in thread
From: Denis V. Lunev @ 2022-08-23 9:20 UTC (permalink / raw)
To: Alexander Ivanov, Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: qemu-block, stefanha, kwolf, hreitz
On 23.08.2022 09:23, Alexander Ivanov wrote:
>
> On 23.08.2022 08:58, Vladimir Sementsov-Ogievskiy wrote:
>> On 8/22/22 12:05, Alexander Ivanov wrote:
>>> data_end field in BDRVParallelsState is set to the biggest offset
>>> present
>>> in BAT. If this offset is outside of the image, any further write
>>> will create
>>> the cluster at this offset and/or the image will be truncated to this
>>> offset on close. This is definitely not correct.
>>> Raise an error in parallels_open() if data_end points outside the
>>> image and
>>> it is not a check (let the check to repaire the image).
>>>
>>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>>> ---
>>> block/parallels.c | 14 ++++++++++++++
>>> 1 file changed, 14 insertions(+)
>>>
>>> diff --git a/block/parallels.c b/block/parallels.c
>>> index a229c06f25..c245ca35cd 100644
>>> --- a/block/parallels.c
>>> +++ b/block/parallels.c
>>> @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs,
>>> QDict *options, int flags,
>>> BDRVParallelsState *s = bs->opaque;
>>> ParallelsHeader ph;
>>> int ret, size, i;
>>> + int64_t file_size;
>>> QemuOpts *opts = NULL;
>>> Error *local_err = NULL;
>>> char *buf;
>>> @@ -811,6 +812,19 @@ static int parallels_open(BlockDriverState *bs,
>>> QDict *options, int flags,
>>> }
>>> }
>>> + file_size = bdrv_getlength(bs->file->bs);
>>> + if (file_size < 0) {
>>> + ret = file_size;
>>> + goto fail;
>>> + }
>>> +
>>> + file_size >>= BDRV_SECTOR_BITS;
>>> + if (s->data_end > file_size && !(flags & BDRV_O_CHECK)) {
>>> + error_setg(errp, "parallels: Offset in BAT is out of image");
>>> + ret = -EINVAL;
>>> + goto fail;
>>> + }
>>
>> If image is unaligned to sector size, and image size is less than
>> s->data_end, but the difference itself is less than sector, the error
>> message would be misleading.
>>
>> Should we consider "file_size = DIV_ROUND_UP(file_size,
>> BDRV_SECTOR_SIZE)" instead of "file_size >>= BDRV_SECTOR_BITS"?
>>
>> It's hardly possible to get such image on valid scenarios with Qemu
>> (keeping in mind bdrv_truncate() call in parallels_close()). But it
>> still may be possible to have such images produced by another
>> software or by some failure path.
>>
> I think you are right, it would be better to align image size up to
> sector size.
I would say that we need to align not on sector size but on cluster size.
That would worth additional check.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 2/9] parallels: Fix data_end field value in parallels_co_check()
2022-08-22 9:05 ` [PATCH v5 2/9] parallels: Fix data_end field value in parallels_co_check() Alexander Ivanov
2022-08-23 7:38 ` Vladimir Sementsov-Ogievskiy
@ 2022-08-23 9:23 ` Denis V. Lunev
1 sibling, 0 replies; 31+ messages in thread
From: Denis V. Lunev @ 2022-08-23 9:23 UTC (permalink / raw)
To: Alexander Ivanov, qemu-devel
Cc: qemu-block, stefanha, vsementsov, kwolf, hreitz
On 22.08.2022 11:05, Alexander Ivanov wrote:
> Make data_end pointing to the end of the last cluster if a leak was fixed.
> Otherwise set the file size to data_end.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
> block/parallels.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index c245ca35cd..2be56871bc 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -513,7 +513,15 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
> res->leaks_fixed += count;
> }
> }
> -
> + /*
> + * If res->image_end_offset less than the file size,
> + * a leak was fixed and we should set the new offset to s->data_end.
> + * Otherwise set the file size to s->data_end.
> + */
> + if (res->image_end_offset < size && fix & BDRV_FIX_LEAKS) {
technically this is correct, but please add braces around
(fix & BDRV_FIX_LEAKS) - the code would look better
> + size = res->image_end_offset;
> + }
> + s->data_end = size >> BDRV_SECTOR_BITS;
> out:
> qemu_co_mutex_unlock(&s->lock);
> return ret;
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] parallels: Out of image offset in BAT leads to image inflation
2022-08-23 6:58 ` Vladimir Sementsov-Ogievskiy
2022-08-23 7:23 ` Alexander Ivanov
@ 2022-08-23 9:34 ` Denis V. Lunev
2022-08-23 9:47 ` Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 31+ messages in thread
From: Denis V. Lunev @ 2022-08-23 9:34 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, Alexander Ivanov, qemu-devel
Cc: qemu-block, stefanha, kwolf, hreitz
On 23.08.2022 08:58, Vladimir Sementsov-Ogievskiy wrote:
> On 8/22/22 12:05, Alexander Ivanov wrote:
>> data_end field in BDRVParallelsState is set to the biggest offset
>> present
>> in BAT. If this offset is outside of the image, any further write
>> will create
>> the cluster at this offset and/or the image will be truncated to this
>> offset on close. This is definitely not correct.
>> Raise an error in parallels_open() if data_end points outside the
>> image and
>> it is not a check (let the check to repaire the image).
>>
>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>> ---
>> block/parallels.c | 14 ++++++++++++++
>> 1 file changed, 14 insertions(+)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index a229c06f25..c245ca35cd 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs,
>> QDict *options, int flags,
>> BDRVParallelsState *s = bs->opaque;
>> ParallelsHeader ph;
>> int ret, size, i;
>> + int64_t file_size;
>> QemuOpts *opts = NULL;
>> Error *local_err = NULL;
>> char *buf;
>> @@ -811,6 +812,19 @@ static int parallels_open(BlockDriverState *bs,
>> QDict *options, int flags,
>> }
>> }
>> + file_size = bdrv_getlength(bs->file->bs);
>> + if (file_size < 0) {
>> + ret = file_size;
>> + goto fail;
>> + }
>> +
>> + file_size >>= BDRV_SECTOR_BITS;
>> + if (s->data_end > file_size && !(flags & BDRV_O_CHECK)) {
>> + error_setg(errp, "parallels: Offset in BAT is out of image");
>> + ret = -EINVAL;
>> + goto fail;
>> + }
>
> If image is unaligned to sector size, and image size is less than
> s->data_end, but the difference itself is less than sector, the error
> message would be misleading.
>
> Should we consider "file_size = DIV_ROUND_UP(file_size,
> BDRV_SECTOR_SIZE)" instead of "file_size >>= BDRV_SECTOR_BITS"?
That is a different check. We need to be sure that file_size is authentic,
which worth additional check.
At my opinion, this worth additional patch later on. Let us agree here,
queue and proceed with further pending fixes.
We should use something like the following
data_start = le32_to_cpu(s->header->data_off);
if (data_start == 0) {
data_start = ROUND_UP(bat_entry_off(s->bat_size),
BDRV_SECTOR_SIZE);
}
if ((file_size - data_start) % cluster_size != 0) {
error();
}
>
> It's hardly possible to get such image on valid scenarios with Qemu
> (keeping in mind bdrv_truncate() call in parallels_close()). But it
> still may be possible to have such images produced by another software
> or by some failure path.
>
>
>> +
>> if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
>> /* Image was not closed correctly. The check is mandatory */
>> s->header_unclean = true;
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 4/9] parallels: Use generic infrastructure for BAT writing in parallels_co_check()
2022-08-22 9:05 ` [PATCH v5 4/9] parallels: Use generic infrastructure for BAT writing in parallels_co_check() Alexander Ivanov
@ 2022-08-23 9:36 ` Denis V. Lunev
0 siblings, 0 replies; 31+ messages in thread
From: Denis V. Lunev @ 2022-08-23 9:36 UTC (permalink / raw)
To: Alexander Ivanov, qemu-devel
Cc: qemu-block, stefanha, vsementsov, kwolf, hreitz
On 22.08.2022 11:05, Alexander Ivanov wrote:
> BAT is written in the context of conventional operations over
> the image inside bdrv_co_flush() when it calls
> parallels_co_flush_to_os() callback. Thus we should not
> modify BAT array directly, but call parallels_set_bat_entry()
> helper and bdrv_co_flush() further on. After that there is no
> need to manually write BAT and track its modification.
>
> This makes code more generic and allows to split
> parallels_set_bat_entry() for independent pieces.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
> block/parallels.c | 23 ++++++++++-------------
> 1 file changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 8733ef8a70..357c345517 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -425,9 +425,8 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
> {
> BDRVParallelsState *s = bs->opaque;
> int64_t size, prev_off, high_off;
> - int ret;
> + int ret = 0;
> uint32_t i;
> - bool flush_bat = false;
>
> size = bdrv_getlength(bs->file->bs);
> if (size < 0) {
> @@ -466,9 +465,8 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
> res->corruptions++;
> if (fix & BDRV_FIX_ERRORS) {
> prev_off = 0;
> - s->bat_bitmap[i] = 0;
> + parallels_set_bat_entry(s, i, 0);
> res->corruptions_fixed++;
> - flush_bat = true;
> continue;
> }
> }
> @@ -484,15 +482,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
> prev_off = off;
> }
>
> - ret = 0;
> - if (flush_bat) {
> - ret = bdrv_co_pwrite_sync(bs->file, 0, s->header_size, s->header, 0);
> - if (ret < 0) {
> - res->check_errors++;
> - goto out;
> - }
> - }
> -
> res->image_end_offset = high_off + s->cluster_size;
> if (size > res->image_end_offset) {
> int64_t count;
> @@ -529,6 +518,14 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
> s->data_end = size >> BDRV_SECTOR_BITS;
> out:
> qemu_co_mutex_unlock(&s->lock);
> +
> + if (ret == 0) {
> + ret = bdrv_co_flush(bs);
> + if (ret < 0) {
> + res->check_errors++;
> + }
> + }
> +
> return ret;
> }
>
Reviewed-by: Denis V. Lunev <den@openvz.org>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 7/9] parallels: Move check of leaks to a separate function
2022-08-22 9:05 ` [PATCH v5 7/9] parallels: Move check of leaks " Alexander Ivanov
@ 2022-08-23 9:40 ` Denis V. Lunev
2022-08-23 9:56 ` Alexander Ivanov
0 siblings, 1 reply; 31+ messages in thread
From: Denis V. Lunev @ 2022-08-23 9:40 UTC (permalink / raw)
To: Alexander Ivanov, qemu-devel
Cc: qemu-block, stefanha, vsementsov, kwolf, hreitz
On 22.08.2022 11:05, Alexander Ivanov wrote:
> We will add more and more checks so we need a better code structure
> in parallels_co_check. Let each check performs in a separate loop
> in a separate helper.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
> block/parallels.c | 87 +++++++++++++++++++++++++++++------------------
> 1 file changed, 54 insertions(+), 33 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index bf074f247c..12e8d382f3 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -468,14 +468,13 @@ static int parallels_check_outside_image(BlockDriverState *bs,
> return 0;
> }
>
> -static int coroutine_fn parallels_co_check(BlockDriverState *bs,
> - BdrvCheckResult *res,
> - BdrvCheckMode fix)
> +static int parallels_check_leak(BlockDriverState *bs,
> + BdrvCheckResult *res,
> + BdrvCheckMode fix)
> {
> BDRVParallelsState *s = bs->opaque;
> - int64_t size, prev_off, high_off;
> - int ret;
> - uint32_t i;
> + int64_t size, off, high_off, count;
> + int i, ret;
This is not we have agreed on. 'i' should be uint32_t
You have fixed parallels_co_check, but this needs
the fix too.
>
> size = bdrv_getlength(bs->file->bs);
> if (size < 0) {
> @@ -483,41 +482,16 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
> return size;
> }
>
> - qemu_co_mutex_lock(&s->lock);
> -
> - parallels_check_unclean(bs, res, fix);
> -
> - ret = parallels_check_outside_image(bs, res, fix);
> - if (ret < 0) {
> - goto out;
> - }
> -
> - res->bfi.total_clusters = s->bat_size;
> - res->bfi.compressed_clusters = 0; /* compression is not supported */
> -
> high_off = 0;
> - prev_off = 0;
> for (i = 0; i < s->bat_size; i++) {
> - int64_t off = bat2sect(s, i) << BDRV_SECTOR_BITS;
> - if (off == 0) {
> - prev_off = 0;
> - continue;
> - }
> -
> - res->bfi.allocated_clusters++;
> + off = bat2sect(s, i) << BDRV_SECTOR_BITS;
> if (off > high_off) {
> high_off = off;
> }
> -
> - if (prev_off != 0 && (prev_off + s->cluster_size) != off) {
> - res->bfi.fragmented_clusters++;
> - }
> - prev_off = off;
> }
>
> res->image_end_offset = high_off + s->cluster_size;
> if (size > res->image_end_offset) {
> - int64_t count;
> count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
> fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
> fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
> @@ -535,11 +509,12 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
> if (ret < 0) {
> error_report_err(local_err);
> res->check_errors++;
> - goto out;
> + return ret;
> }
> res->leaks_fixed += count;
> }
> }
> +
> /*
> * If res->image_end_offset less than the file size,
> * a leak was fixed and we should set the new offset to s->data_end.
> @@ -549,6 +524,52 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
> size = res->image_end_offset;
> }
> s->data_end = size >> BDRV_SECTOR_BITS;
> +
> + return 0;
> +}
> +
> +static int coroutine_fn parallels_co_check(BlockDriverState *bs,
> + BdrvCheckResult *res,
> + BdrvCheckMode fix)
> +{
> + BDRVParallelsState *s = bs->opaque;
> + int64_t prev_off;
> + int ret;
> + uint32_t i;
> +
> + qemu_co_mutex_lock(&s->lock);
> +
> + parallels_check_unclean(bs, res, fix);
> +
> + ret = parallels_check_outside_image(bs, res, fix);
> + if (ret < 0) {
> + goto out;
> + }
> +
> + ret = parallels_check_leak(bs, res, fix);
> + if (ret < 0) {
> + goto out;
> + }
> +
> + res->bfi.total_clusters = s->bat_size;
> + res->bfi.compressed_clusters = 0; /* compression is not supported */
> +
> + prev_off = 0;
> + for (i = 0; i < s->bat_size; i++) {
> + int64_t off = bat2sect(s, i) << BDRV_SECTOR_BITS;
> + if (off == 0) {
> + prev_off = 0;
> + continue;
> + }
> +
> + res->bfi.allocated_clusters++;
> +
> + if (prev_off != 0 && (prev_off + s->cluster_size) != off) {
> + res->bfi.fragmented_clusters++;
> + }
> + prev_off = off;
> + }
> +
> out:
> qemu_co_mutex_unlock(&s->lock);
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] parallels: Out of image offset in BAT leads to image inflation
2022-08-23 7:59 ` Vladimir Sementsov-Ogievskiy
@ 2022-08-23 9:45 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-08-23 9:45 UTC (permalink / raw)
To: Alexander Ivanov, qemu-devel; +Cc: qemu-block, den, stefanha, kwolf, hreitz
On 8/23/22 10:59, Vladimir Sementsov-Ogievskiy wrote:
> On 8/23/22 10:23, Alexander Ivanov wrote:
>>
>> On 23.08.2022 08:58, Vladimir Sementsov-Ogievskiy wrote:
>>> On 8/22/22 12:05, Alexander Ivanov wrote:
>>>> data_end field in BDRVParallelsState is set to the biggest offset present
>>>> in BAT. If this offset is outside of the image, any further write will create
>>>> the cluster at this offset and/or the image will be truncated to this
>>>> offset on close. This is definitely not correct.
>
> Actually, it's correct, as don't break the spec at docs/interop/parallels.txt.
>
> So, it's bad but still correct.. So, we probably can restrict it in Qemu if we want. Do we?
>
> Now I doubt. We are going to refuse images with leaks even for read. That means, qemu-img convert will stop working for images with leaks, you'll have to fix the image first, which may fail.
>
>>>> Raise an error in parallels_open() if data_end points outside the image and
>>>> it is not a check (let the check to repaire the image).
>>>>
>>>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>>>> ---
>>>> block/parallels.c | 14 ++++++++++++++
>>>> 1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/block/parallels.c b/block/parallels.c
>>>> index a229c06f25..c245ca35cd 100644
>>>> --- a/block/parallels.c
>>>> +++ b/block/parallels.c
>>>> @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>>> BDRVParallelsState *s = bs->opaque;
>>>> ParallelsHeader ph;
>>>> int ret, size, i;
>>>> + int64_t file_size;
>>>> QemuOpts *opts = NULL;
>>>> Error *local_err = NULL;
>>>> char *buf;
>>>> @@ -811,6 +812,19 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>>> }
>>>> }
>>>> + file_size = bdrv_getlength(bs->file->bs);
>>>> + if (file_size < 0) {
>>>> + ret = file_size;
>>>> + goto fail;
>>>> + }
>>>> +
>>>> + file_size >>= BDRV_SECTOR_BITS;
>>>> + if (s->data_end > file_size && !(flags & BDRV_O_CHECK)) {
>>>> + error_setg(errp, "parallels: Offset in BAT is out of image");
>>>> + ret = -EINVAL;
>>>> + goto fail;
>>>> + }
>>>
>>> If image is unaligned to sector size, and image size is less than s->data_end, but the difference itself is less than sector, the error message would be misleading.
>>>
>>> Should we consider "file_size = DIV_ROUND_UP(file_size, BDRV_SECTOR_SIZE)" instead of "file_size >>= BDRV_SECTOR_BITS"?
>>>
>>> It's hardly possible to get such image on valid scenarios with Qemu (keeping in mind bdrv_truncate() call in parallels_close()). But it still may be possible to have such images produced by another software or by some failure path.
>>>
>> I think you are right, it would be better to align image size up to sector size.
>
>
> Hmm, or even to cluster_size? When last cluster is unallocated. But cluster boundary is not required to be aligned to cluster size..
>
> Anyway, now I think it's wrong to restrict opening file with leaks of any kind. That breaks qemu-img convert, and that's not how other formats behave.
>
>>>
>>>> +
>>>> if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
>>>> /* Image was not closed correctly. The check is mandatory */
>>>> s->header_unclean = true;
>>>
>>>
>
>
Oops, ignore this. We are not about leaks (when file size > data_end), we are about wrong clusters when data_end > file_size.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] parallels: Out of image offset in BAT leads to image inflation
2022-08-23 9:34 ` Denis V. Lunev
@ 2022-08-23 9:47 ` Vladimir Sementsov-Ogievskiy
2022-08-23 10:02 ` Denis V. Lunev
0 siblings, 1 reply; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-08-23 9:47 UTC (permalink / raw)
To: Denis V. Lunev, Alexander Ivanov, qemu-devel
Cc: qemu-block, stefanha, kwolf, hreitz
On 8/23/22 12:34, Denis V. Lunev wrote:
> On 23.08.2022 08:58, Vladimir Sementsov-Ogievskiy wrote:
>> On 8/22/22 12:05, Alexander Ivanov wrote:
>>> data_end field in BDRVParallelsState is set to the biggest offset present
>>> in BAT. If this offset is outside of the image, any further write will create
>>> the cluster at this offset and/or the image will be truncated to this
>>> offset on close. This is definitely not correct.
>>> Raise an error in parallels_open() if data_end points outside the image and
>>> it is not a check (let the check to repaire the image).
>>>
>>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>>> ---
>>> block/parallels.c | 14 ++++++++++++++
>>> 1 file changed, 14 insertions(+)
>>>
>>> diff --git a/block/parallels.c b/block/parallels.c
>>> index a229c06f25..c245ca35cd 100644
>>> --- a/block/parallels.c
>>> +++ b/block/parallels.c
>>> @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>> BDRVParallelsState *s = bs->opaque;
>>> ParallelsHeader ph;
>>> int ret, size, i;
>>> + int64_t file_size;
>>> QemuOpts *opts = NULL;
>>> Error *local_err = NULL;
>>> char *buf;
>>> @@ -811,6 +812,19 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>> }
>>> }
>>> + file_size = bdrv_getlength(bs->file->bs);
>>> + if (file_size < 0) {
>>> + ret = file_size;
>>> + goto fail;
>>> + }
>>> +
>>> + file_size >>= BDRV_SECTOR_BITS;
>>> + if (s->data_end > file_size && !(flags & BDRV_O_CHECK)) {
>>> + error_setg(errp, "parallels: Offset in BAT is out of image");
>>> + ret = -EINVAL;
>>> + goto fail;
>>> + }
>>
>> If image is unaligned to sector size, and image size is less than s->data_end, but the difference itself is less than sector, the error message would be misleading.
>>
>> Should we consider "file_size = DIV_ROUND_UP(file_size, BDRV_SECTOR_SIZE)" instead of "file_size >>= BDRV_SECTOR_BITS"?
> That is a different check. We need to be sure that file_size is authentic,
> which worth additional check.
>
> At my opinion, this worth additional patch later on. Let us agree here,
> queue and proceed with further pending fixes.
>
> We should use something like the following
>
> data_start = le32_to_cpu(s->header->data_off);
> if (data_start == 0) {
> data_start = ROUND_UP(bat_entry_off(s->bat_size), BDRV_SECTOR_SIZE);
> }
> if ((file_size - data_start) % cluster_size != 0) {
> error();
> }
Do you think that we should error-out in such conditions? It doesn't break the spec, is it? Can the last cluster be half allocated?
>
>>
>> It's hardly possible to get such image on valid scenarios with Qemu (keeping in mind bdrv_truncate() call in parallels_close()). But it still may be possible to have such images produced by another software or by some failure path.
>>
>>
>>> +
>>> if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
>>> /* Image was not closed correctly. The check is mandatory */
>>> s->header_unclean = true;
>>
>>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] parallels: Out of image offset in BAT leads to image inflation
2022-08-23 9:20 ` Denis V. Lunev
@ 2022-08-23 9:49 ` Alexander Ivanov
2022-08-23 9:58 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 31+ messages in thread
From: Alexander Ivanov @ 2022-08-23 9:49 UTC (permalink / raw)
To: Denis V. Lunev, Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: qemu-block, stefanha, kwolf, hreitz
On 23.08.2022 11:20, Denis V. Lunev wrote:
> On 23.08.2022 09:23, Alexander Ivanov wrote:
>>
>> On 23.08.2022 08:58, Vladimir Sementsov-Ogievskiy wrote:
>>> On 8/22/22 12:05, Alexander Ivanov wrote:
>>>> data_end field in BDRVParallelsState is set to the biggest offset
>>>> present
>>>> in BAT. If this offset is outside of the image, any further write
>>>> will create
>>>> the cluster at this offset and/or the image will be truncated to this
>>>> offset on close. This is definitely not correct.
>>>> Raise an error in parallels_open() if data_end points outside the
>>>> image and
>>>> it is not a check (let the check to repaire the image).
>>>>
>>>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>>>> ---
>>>> block/parallels.c | 14 ++++++++++++++
>>>> 1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/block/parallels.c b/block/parallels.c
>>>> index a229c06f25..c245ca35cd 100644
>>>> --- a/block/parallels.c
>>>> +++ b/block/parallels.c
>>>> @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs,
>>>> QDict *options, int flags,
>>>> BDRVParallelsState *s = bs->opaque;
>>>> ParallelsHeader ph;
>>>> int ret, size, i;
>>>> + int64_t file_size;
>>>> QemuOpts *opts = NULL;
>>>> Error *local_err = NULL;
>>>> char *buf;
>>>> @@ -811,6 +812,19 @@ static int parallels_open(BlockDriverState
>>>> *bs, QDict *options, int flags,
>>>> }
>>>> }
>>>> + file_size = bdrv_getlength(bs->file->bs);
>>>> + if (file_size < 0) {
>>>> + ret = file_size;
>>>> + goto fail;
>>>> + }
>>>> +
>>>> + file_size >>= BDRV_SECTOR_BITS;
>>>> + if (s->data_end > file_size && !(flags & BDRV_O_CHECK)) {
>>>> + error_setg(errp, "parallels: Offset in BAT is out of image");
>>>> + ret = -EINVAL;
>>>> + goto fail;
>>>> + }
>>>
>>> If image is unaligned to sector size, and image size is less than
>>> s->data_end, but the difference itself is less than sector, the
>>> error message would be misleading.
>>>
>>> Should we consider "file_size = DIV_ROUND_UP(file_size,
>>> BDRV_SECTOR_SIZE)" instead of "file_size >>= BDRV_SECTOR_BITS"?
>>>
>>> It's hardly possible to get such image on valid scenarios with Qemu
>>> (keeping in mind bdrv_truncate() call in parallels_close()). But it
>>> still may be possible to have such images produced by another
>>> software or by some failure path.
>>>
>> I think you are right, it would be better to align image size up to
>> sector size.
>
> I would say that we need to align not on sector size but on cluster size.
> That would worth additional check.
I agree on it for "WithouFreSpacExt" image format.
For "WithoutFreeSpace" format there are two restrictions:
1) the result of (cluster offset - data offset) must be aligned to
cluster size:
2) data_off should be aligned to sector size (0 in data_off leads to the
same restriction).
So file size can be aligned on sector size, but not aligned on cluster size.
Should we check dependent on image format?
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] parallels: Out of image offset in BAT leads to image inflation
2022-08-22 9:05 ` [PATCH v5 1/9] parallels: Out of image offset in BAT leads to image inflation Alexander Ivanov
2022-08-23 6:58 ` Vladimir Sementsov-Ogievskiy
@ 2022-08-23 9:51 ` Denis V. Lunev
1 sibling, 0 replies; 31+ messages in thread
From: Denis V. Lunev @ 2022-08-23 9:51 UTC (permalink / raw)
To: Alexander Ivanov, qemu-devel
Cc: qemu-block, stefanha, vsementsov, kwolf, hreitz
On 22.08.2022 11:05, Alexander Ivanov wrote:
> data_end field in BDRVParallelsState is set to the biggest offset present
> in BAT. If this offset is outside of the image, any further write will create
> the cluster at this offset and/or the image will be truncated to this
> offset on close. This is definitely not correct.
> Raise an error in parallels_open() if data_end points outside the image and
> it is not a check (let the check to repaire the image).
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
> block/parallels.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index a229c06f25..c245ca35cd 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
> BDRVParallelsState *s = bs->opaque;
> ParallelsHeader ph;
> int ret, size, i;
> + int64_t file_size;
> QemuOpts *opts = NULL;
> Error *local_err = NULL;
> char *buf;
> @@ -811,6 +812,19 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
> }
> }
>
> + file_size = bdrv_getlength(bs->file->bs);
> + if (file_size < 0) {
> + ret = file_size;
> + goto fail;
> + }
> +
> + file_size >>= BDRV_SECTOR_BITS;
> + if (s->data_end > file_size && !(flags & BDRV_O_CHECK)) {
> + error_setg(errp, "parallels: Offset in BAT is out of image");
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> /* Image was not closed correctly. The check is mandatory */
> s->header_unclean = true;
Reviewed-by: Denis V. Lunev <den@openvz.org>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 7/9] parallels: Move check of leaks to a separate function
2022-08-23 9:40 ` Denis V. Lunev
@ 2022-08-23 9:56 ` Alexander Ivanov
0 siblings, 0 replies; 31+ messages in thread
From: Alexander Ivanov @ 2022-08-23 9:56 UTC (permalink / raw)
To: Denis V. Lunev, qemu-devel
Cc: qemu-block, stefanha, vsementsov, kwolf, hreitz
On 23.08.2022 11:40, Denis V. Lunev wrote:
> On 22.08.2022 11:05, Alexander Ivanov wrote:
>> We will add more and more checks so we need a better code structure
>> in parallels_co_check. Let each check performs in a separate loop
>> in a separate helper.
>>
>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>> ---
>> block/parallels.c | 87 +++++++++++++++++++++++++++++------------------
>> 1 file changed, 54 insertions(+), 33 deletions(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index bf074f247c..12e8d382f3 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -468,14 +468,13 @@ static int
>> parallels_check_outside_image(BlockDriverState *bs,
>> return 0;
>> }
>> -static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>> - BdrvCheckResult *res,
>> - BdrvCheckMode fix)
>> +static int parallels_check_leak(BlockDriverState *bs,
>> + BdrvCheckResult *res,
>> + BdrvCheckMode fix)
>> {
>> BDRVParallelsState *s = bs->opaque;
>> - int64_t size, prev_off, high_off;
>> - int ret;
>> - uint32_t i;
>> + int64_t size, off, high_off, count;
>> + int i, ret;
> This is not we have agreed on. 'i' should be uint32_t
> You have fixed parallels_co_check, but this needs
> the fix too.
Damn! Missed it when rebased on changed patches.
>
>> size = bdrv_getlength(bs->file->bs);
>> if (size < 0) {
>> @@ -483,41 +482,16 @@ static int coroutine_fn
>> parallels_co_check(BlockDriverState *bs,
>> return size;
>> }
>> - qemu_co_mutex_lock(&s->lock);
>> -
>> - parallels_check_unclean(bs, res, fix);
>> -
>> - ret = parallels_check_outside_image(bs, res, fix);
>> - if (ret < 0) {
>> - goto out;
>> - }
>> -
>> - res->bfi.total_clusters = s->bat_size;
>> - res->bfi.compressed_clusters = 0; /* compression is not
>> supported */
>> -
>> high_off = 0;
>> - prev_off = 0;
>> for (i = 0; i < s->bat_size; i++) {
>> - int64_t off = bat2sect(s, i) << BDRV_SECTOR_BITS;
>> - if (off == 0) {
>> - prev_off = 0;
>> - continue;
>> - }
>> -
>> - res->bfi.allocated_clusters++;
>> + off = bat2sect(s, i) << BDRV_SECTOR_BITS;
>> if (off > high_off) {
>> high_off = off;
>> }
>> -
>> - if (prev_off != 0 && (prev_off + s->cluster_size) != off) {
>> - res->bfi.fragmented_clusters++;
>> - }
>> - prev_off = off;
>> }
>> res->image_end_offset = high_off + s->cluster_size;
>> if (size > res->image_end_offset) {
>> - int64_t count;
>> count = DIV_ROUND_UP(size - res->image_end_offset,
>> s->cluster_size);
>> fprintf(stderr, "%s space leaked at the end of the image %"
>> PRId64 "\n",
>> fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
>> @@ -535,11 +509,12 @@ static int coroutine_fn
>> parallels_co_check(BlockDriverState *bs,
>> if (ret < 0) {
>> error_report_err(local_err);
>> res->check_errors++;
>> - goto out;
>> + return ret;
>> }
>> res->leaks_fixed += count;
>> }
>> }
>> +
>> /*
>> * If res->image_end_offset less than the file size,
>> * a leak was fixed and we should set the new offset to
>> s->data_end.
>> @@ -549,6 +524,52 @@ static int coroutine_fn
>> parallels_co_check(BlockDriverState *bs,
>> size = res->image_end_offset;
>> }
>> s->data_end = size >> BDRV_SECTOR_BITS;
>> +
>> + return 0;
>> +}
>> +
>> +static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>> + BdrvCheckResult *res,
>> + BdrvCheckMode fix)
>> +{
>> + BDRVParallelsState *s = bs->opaque;
>> + int64_t prev_off;
>> + int ret;
>> + uint32_t i;
>> +
>> + qemu_co_mutex_lock(&s->lock);
>> +
>> + parallels_check_unclean(bs, res, fix);
>> +
>> + ret = parallels_check_outside_image(bs, res, fix);
>> + if (ret < 0) {
>> + goto out;
>> + }
>> +
>> + ret = parallels_check_leak(bs, res, fix);
>> + if (ret < 0) {
>> + goto out;
>> + }
>> +
>> + res->bfi.total_clusters = s->bat_size;
>> + res->bfi.compressed_clusters = 0; /* compression is not
>> supported */
>> +
>> + prev_off = 0;
>> + for (i = 0; i < s->bat_size; i++) {
>> + int64_t off = bat2sect(s, i) << BDRV_SECTOR_BITS;
>> + if (off == 0) {
>> + prev_off = 0;
>> + continue;
>> + }
>> +
>> + res->bfi.allocated_clusters++;
>> +
>> + if (prev_off != 0 && (prev_off + s->cluster_size) != off) {
>> + res->bfi.fragmented_clusters++;
>> + }
>> + prev_off = off;
>> + }
>> +
>> out:
>> qemu_co_mutex_unlock(&s->lock);
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] parallels: Out of image offset in BAT leads to image inflation
2022-08-23 9:20 ` Denis V. Lunev
2022-08-23 9:49 ` Alexander Ivanov
@ 2022-08-23 9:58 ` Vladimir Sementsov-Ogievskiy
2022-08-23 10:11 ` Denis V. Lunev
2022-08-23 13:50 ` Alexander Ivanov
1 sibling, 2 replies; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-08-23 9:58 UTC (permalink / raw)
To: Denis V. Lunev, Alexander Ivanov, qemu-devel
Cc: qemu-block, stefanha, kwolf, hreitz
On 8/23/22 12:20, Denis V. Lunev wrote:
> On 23.08.2022 09:23, Alexander Ivanov wrote:
>>
>> On 23.08.2022 08:58, Vladimir Sementsov-Ogievskiy wrote:
>>> On 8/22/22 12:05, Alexander Ivanov wrote:
>>>> data_end field in BDRVParallelsState is set to the biggest offset present
>>>> in BAT. If this offset is outside of the image, any further write will create
>>>> the cluster at this offset and/or the image will be truncated to this
>>>> offset on close. This is definitely not correct.
>>>> Raise an error in parallels_open() if data_end points outside the image and
>>>> it is not a check (let the check to repaire the image).
>>>>
>>>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>>>> ---
>>>> block/parallels.c | 14 ++++++++++++++
>>>> 1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/block/parallels.c b/block/parallels.c
>>>> index a229c06f25..c245ca35cd 100644
>>>> --- a/block/parallels.c
>>>> +++ b/block/parallels.c
>>>> @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>>> BDRVParallelsState *s = bs->opaque;
>>>> ParallelsHeader ph;
>>>> int ret, size, i;
>>>> + int64_t file_size;
>>>> QemuOpts *opts = NULL;
>>>> Error *local_err = NULL;
>>>> char *buf;
>>>> @@ -811,6 +812,19 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>>> }
>>>> }
>>>> + file_size = bdrv_getlength(bs->file->bs);
>>>> + if (file_size < 0) {
>>>> + ret = file_size;
>>>> + goto fail;
>>>> + }
>>>> +
>>>> + file_size >>= BDRV_SECTOR_BITS;
>>>> + if (s->data_end > file_size && !(flags & BDRV_O_CHECK)) {
>>>> + error_setg(errp, "parallels: Offset in BAT is out of image");
>>>> + ret = -EINVAL;
>>>> + goto fail;
>>>> + }
>>>
>>> If image is unaligned to sector size, and image size is less than s->data_end, but the difference itself is less than sector, the error message would be misleading.
>>>
>>> Should we consider "file_size = DIV_ROUND_UP(file_size, BDRV_SECTOR_SIZE)" instead of "file_size >>= BDRV_SECTOR_BITS"?
>>>
>>> It's hardly possible to get such image on valid scenarios with Qemu (keeping in mind bdrv_truncate() call in parallels_close()). But it still may be possible to have such images produced by another software or by some failure path.
>>>
>> I think you are right, it would be better to align image size up to sector size.
>
> I would say that we need to align not on sector size but on cluster size.
> That would worth additional check.
And not simply align, as data_offset is not necessarily aligned to cluster size.
Finally, what should we check?
I suggest
diff --git a/block/parallels.c b/block/parallels.c
index 6d4ed77f16..b882ea1200 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -725,6 +725,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
BDRVParallelsState *s = bs->opaque;
ParallelsHeader ph;
int ret, size, i;
+ int64_t file_size;
QemuOpts *opts = NULL;
Error *local_err = NULL;
char *buf;
@@ -735,6 +736,11 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
return -EINVAL;
}
+ file_size = bdrv_getlength(bs->file->bs);
+ if (file_size < 0) {
+ return file_size;
+ }
+
ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph));
if (ret < 0) {
goto fail;
@@ -798,6 +804,13 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
for (i = 0; i < s->bat_size; i++) {
int64_t off = bat2sect(s, i);
+ if (off >= file_size) {
+ error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
+ "is larger than file size (%" PRIi64 ")",
+ off, i, file_size);
+ ret = -EINVAL;
+ goto fail;
+ }
if (off >= s->data_end) {
s->data_end = off + s->tracks;
}
- better error message, and we check exactly what's written in the spec (docs/interop/parallels.c):
Cluster offsets specified by BAT entries must meet the following requirements:
[...]
- the value must be lower than the desired file size,
--
Best regards,
Vladimir
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] parallels: Out of image offset in BAT leads to image inflation
2022-08-23 9:47 ` Vladimir Sementsov-Ogievskiy
@ 2022-08-23 10:02 ` Denis V. Lunev
0 siblings, 0 replies; 31+ messages in thread
From: Denis V. Lunev @ 2022-08-23 10:02 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, Alexander Ivanov, qemu-devel
Cc: qemu-block, stefanha, kwolf, hreitz
On 23.08.2022 11:47, Vladimir Sementsov-Ogievskiy wrote:
> On 8/23/22 12:34, Denis V. Lunev wrote:
>> On 23.08.2022 08:58, Vladimir Sementsov-Ogievskiy wrote:
>>> On 8/22/22 12:05, Alexander Ivanov wrote:
>>>> data_end field in BDRVParallelsState is set to the biggest offset
>>>> present
>>>> in BAT. If this offset is outside of the image, any further write
>>>> will create
>>>> the cluster at this offset and/or the image will be truncated to this
>>>> offset on close. This is definitely not correct.
>>>> Raise an error in parallels_open() if data_end points outside the
>>>> image and
>>>> it is not a check (let the check to repaire the image).
>>>>
>>>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>>>> ---
>>>> block/parallels.c | 14 ++++++++++++++
>>>> 1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/block/parallels.c b/block/parallels.c
>>>> index a229c06f25..c245ca35cd 100644
>>>> --- a/block/parallels.c
>>>> +++ b/block/parallels.c
>>>> @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs,
>>>> QDict *options, int flags,
>>>> BDRVParallelsState *s = bs->opaque;
>>>> ParallelsHeader ph;
>>>> int ret, size, i;
>>>> + int64_t file_size;
>>>> QemuOpts *opts = NULL;
>>>> Error *local_err = NULL;
>>>> char *buf;
>>>> @@ -811,6 +812,19 @@ static int parallels_open(BlockDriverState
>>>> *bs, QDict *options, int flags,
>>>> }
>>>> }
>>>> + file_size = bdrv_getlength(bs->file->bs);
>>>> + if (file_size < 0) {
>>>> + ret = file_size;
>>>> + goto fail;
>>>> + }
>>>> +
>>>> + file_size >>= BDRV_SECTOR_BITS;
>>>> + if (s->data_end > file_size && !(flags & BDRV_O_CHECK)) {
>>>> + error_setg(errp, "parallels: Offset in BAT is out of image");
>>>> + ret = -EINVAL;
>>>> + goto fail;
>>>> + }
>>>
>>> If image is unaligned to sector size, and image size is less than
>>> s->data_end, but the difference itself is less than sector, the
>>> error message would be misleading.
>>>
>>> Should we consider "file_size = DIV_ROUND_UP(file_size,
>>> BDRV_SECTOR_SIZE)" instead of "file_size >>= BDRV_SECTOR_BITS"?
>> That is a different check. We need to be sure that file_size is
>> authentic,
>> which worth additional check.
>>
>> At my opinion, this worth additional patch later on. Let us agree here,
>> queue and proceed with further pending fixes.
>>
>> We should use something like the following
>>
>> data_start = le32_to_cpu(s->header->data_off);
>> if (data_start == 0) {
>> data_start = ROUND_UP(bat_entry_off(s->bat_size),
>> BDRV_SECTOR_SIZE);
>> }
>> if ((file_size - data_start) % cluster_size != 0) {
>> error();
>> }
>
> Do you think that we should error-out in such conditions? It doesn't
> break the spec, is it? Can the last cluster be half allocated?
absolutely no!
We should adjust the spec on this.
Den
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] parallels: Out of image offset in BAT leads to image inflation
2022-08-23 9:58 ` Vladimir Sementsov-Ogievskiy
@ 2022-08-23 10:11 ` Denis V. Lunev
2022-08-24 8:50 ` Vladimir Sementsov-Ogievskiy
2022-08-23 13:50 ` Alexander Ivanov
1 sibling, 1 reply; 31+ messages in thread
From: Denis V. Lunev @ 2022-08-23 10:11 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, Alexander Ivanov, qemu-devel
Cc: qemu-block, stefanha, kwolf, hreitz
On 23.08.2022 11:58, Vladimir Sementsov-Ogievskiy wrote:
> On 8/23/22 12:20, Denis V. Lunev wrote:
>> On 23.08.2022 09:23, Alexander Ivanov wrote:
>>>
>>> On 23.08.2022 08:58, Vladimir Sementsov-Ogievskiy wrote:
>>>> On 8/22/22 12:05, Alexander Ivanov wrote:
>>>>> data_end field in BDRVParallelsState is set to the biggest offset
>>>>> present
>>>>> in BAT. If this offset is outside of the image, any further write
>>>>> will create
>>>>> the cluster at this offset and/or the image will be truncated to this
>>>>> offset on close. This is definitely not correct.
>>>>> Raise an error in parallels_open() if data_end points outside the
>>>>> image and
>>>>> it is not a check (let the check to repaire the image).
>>>>>
>>>>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>>>>> ---
>>>>> block/parallels.c | 14 ++++++++++++++
>>>>> 1 file changed, 14 insertions(+)
>>>>>
>>>>> diff --git a/block/parallels.c b/block/parallels.c
>>>>> index a229c06f25..c245ca35cd 100644
>>>>> --- a/block/parallels.c
>>>>> +++ b/block/parallels.c
>>>>> @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState
>>>>> *bs, QDict *options, int flags,
>>>>> BDRVParallelsState *s = bs->opaque;
>>>>> ParallelsHeader ph;
>>>>> int ret, size, i;
>>>>> + int64_t file_size;
>>>>> QemuOpts *opts = NULL;
>>>>> Error *local_err = NULL;
>>>>> char *buf;
>>>>> @@ -811,6 +812,19 @@ static int parallels_open(BlockDriverState
>>>>> *bs, QDict *options, int flags,
>>>>> }
>>>>> }
>>>>> + file_size = bdrv_getlength(bs->file->bs);
>>>>> + if (file_size < 0) {
>>>>> + ret = file_size;
>>>>> + goto fail;
>>>>> + }
>>>>> +
>>>>> + file_size >>= BDRV_SECTOR_BITS;
>>>>> + if (s->data_end > file_size && !(flags & BDRV_O_CHECK)) {
>>>>> + error_setg(errp, "parallels: Offset in BAT is out of
>>>>> image");
>>>>> + ret = -EINVAL;
>>>>> + goto fail;
>>>>> + }
>>>>
>>>> If image is unaligned to sector size, and image size is less than
>>>> s->data_end, but the difference itself is less than sector, the
>>>> error message would be misleading.
>>>>
>>>> Should we consider "file_size = DIV_ROUND_UP(file_size,
>>>> BDRV_SECTOR_SIZE)" instead of "file_size >>= BDRV_SECTOR_BITS"?
>>>>
>>>> It's hardly possible to get such image on valid scenarios with Qemu
>>>> (keeping in mind bdrv_truncate() call in parallels_close()). But it
>>>> still may be possible to have such images produced by another
>>>> software or by some failure path.
>>>>
>>> I think you are right, it would be better to align image size up to
>>> sector size.
>>
>> I would say that we need to align not on sector size but on cluster
>> size.
>> That would worth additional check.
>
> And not simply align, as data_offset is not necessarily aligned to
> cluster size.
>
> Finally, what should we check?
>
> I suggest
>
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 6d4ed77f16..b882ea1200 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -725,6 +725,7 @@ static int parallels_open(BlockDriverState *bs,
> QDict *options, int flags,
> BDRVParallelsState *s = bs->opaque;
> ParallelsHeader ph;
> int ret, size, i;
> + int64_t file_size;
> QemuOpts *opts = NULL;
> Error *local_err = NULL;
> char *buf;
> @@ -735,6 +736,11 @@ static int parallels_open(BlockDriverState *bs,
> QDict *options, int flags,
> return -EINVAL;
> }
>
> + file_size = bdrv_getlength(bs->file->bs);
> + if (file_size < 0) {
> + return file_size;
> + }
> +
> ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph));
> if (ret < 0) {
> goto fail;
> @@ -798,6 +804,13 @@ static int parallels_open(BlockDriverState *bs,
> QDict *options, int flags,
>
> for (i = 0; i < s->bat_size; i++) {
> int64_t off = bat2sect(s, i);
> + if (off >= file_size) {
Like this, especially >= check which we have had missed.
Though this would break the repair. We need additional
if (flags & BDRV_O_CHECK) {
continue;
}
No incorrect data_end assignment, which would be
very welcome.
Den
> + error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
> + "is larger than file size (%" PRIi64 ")",
> + off, i, file_size);
> + ret = -EINVAL;
> + goto fail;
> + }
> if (off >= s->data_end) {
> s->data_end = off + s->tracks;
> }
>
>
>
> - better error message, and we check exactly what's written in the
> spec (docs/interop/parallels.c):
>
>
> Cluster offsets specified by BAT entries must meet the following
> requirements:
> [...]
> - the value must be lower than the desired file size,
>
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] parallels: Out of image offset in BAT leads to image inflation
2022-08-23 9:58 ` Vladimir Sementsov-Ogievskiy
2022-08-23 10:11 ` Denis V. Lunev
@ 2022-08-23 13:50 ` Alexander Ivanov
2022-08-23 15:43 ` Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 31+ messages in thread
From: Alexander Ivanov @ 2022-08-23 13:50 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, Denis V. Lunev, qemu-devel
Cc: qemu-block, stefanha, kwolf, hreitz
On 23.08.2022 11:58, Vladimir Sementsov-Ogievskiy wrote:
> On 8/23/22 12:20, Denis V. Lunev wrote:
>> On 23.08.2022 09:23, Alexander Ivanov wrote:
>>>
>>> On 23.08.2022 08:58, Vladimir Sementsov-Ogievskiy wrote:
>>>> On 8/22/22 12:05, Alexander Ivanov wrote:
>>>>> data_end field in BDRVParallelsState is set to the biggest offset
>>>>> present
>>>>> in BAT. If this offset is outside of the image, any further write
>>>>> will create
>>>>> the cluster at this offset and/or the image will be truncated to this
>>>>> offset on close. This is definitely not correct.
>>>>> Raise an error in parallels_open() if data_end points outside the
>>>>> image and
>>>>> it is not a check (let the check to repaire the image).
>>>>>
>>>>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>>>>> ---
>>>>> block/parallels.c | 14 ++++++++++++++
>>>>> 1 file changed, 14 insertions(+)
>>>>>
>>>>> diff --git a/block/parallels.c b/block/parallels.c
>>>>> index a229c06f25..c245ca35cd 100644
>>>>> --- a/block/parallels.c
>>>>> +++ b/block/parallels.c
>>>>> @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState
>>>>> *bs, QDict *options, int flags,
>>>>> BDRVParallelsState *s = bs->opaque;
>>>>> ParallelsHeader ph;
>>>>> int ret, size, i;
>>>>> + int64_t file_size;
>>>>> QemuOpts *opts = NULL;
>>>>> Error *local_err = NULL;
>>>>> char *buf;
>>>>> @@ -811,6 +812,19 @@ static int parallels_open(BlockDriverState
>>>>> *bs, QDict *options, int flags,
>>>>> }
>>>>> }
>>>>> + file_size = bdrv_getlength(bs->file->bs);
>>>>> + if (file_size < 0) {
>>>>> + ret = file_size;
>>>>> + goto fail;
>>>>> + }
>>>>> +
>>>>> + file_size >>= BDRV_SECTOR_BITS;
>>>>> + if (s->data_end > file_size && !(flags & BDRV_O_CHECK)) {
>>>>> + error_setg(errp, "parallels: Offset in BAT is out of
>>>>> image");
>>>>> + ret = -EINVAL;
>>>>> + goto fail;
>>>>> + }
>>>>
>>>> If image is unaligned to sector size, and image size is less than
>>>> s->data_end, but the difference itself is less than sector, the
>>>> error message would be misleading.
>>>>
>>>> Should we consider "file_size = DIV_ROUND_UP(file_size,
>>>> BDRV_SECTOR_SIZE)" instead of "file_size >>= BDRV_SECTOR_BITS"?
>>>>
>>>> It's hardly possible to get such image on valid scenarios with Qemu
>>>> (keeping in mind bdrv_truncate() call in parallels_close()). But it
>>>> still may be possible to have such images produced by another
>>>> software or by some failure path.
>>>>
>>> I think you are right, it would be better to align image size up to
>>> sector size.
>>
>> I would say that we need to align not on sector size but on cluster
>> size.
>> That would worth additional check.
>
> And not simply align, as data_offset is not necessarily aligned to
> cluster size.
>
> Finally, what should we check?
>
> I suggest
>
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 6d4ed77f16..b882ea1200 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -725,6 +725,7 @@ static int parallels_open(BlockDriverState *bs,
> QDict *options, int flags,
> BDRVParallelsState *s = bs->opaque;
> ParallelsHeader ph;
> int ret, size, i;
> + int64_t file_size;
> QemuOpts *opts = NULL;
> Error *local_err = NULL;
> char *buf;
> @@ -735,6 +736,11 @@ static int parallels_open(BlockDriverState *bs,
> QDict *options, int flags,
> return -EINVAL;
> }
>
> + file_size = bdrv_getlength(bs->file->bs);
> + if (file_size < 0) {
> + return file_size;
> + }
> +
> ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph));
> if (ret < 0) {
> goto fail;
> @@ -798,6 +804,13 @@ static int parallels_open(BlockDriverState *bs,
> QDict *options, int flags,
>
> for (i = 0; i < s->bat_size; i++) {
> int64_t off = bat2sect(s, i);
> + if (off >= file_size) {
Do we really want to compare file size with cluster offset, not with
cluster end?
Also I would leave the comparison outside the loop, because I'm going to
move highest offset calculation to a helper (there are two places with
the same code).
> + error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
> + "is larger than file size (%" PRIi64 ")",
> + off, i, file_size);
> + ret = -EINVAL;
> + goto fail;
> + }
> if (off >= s->data_end) {
> s->data_end = off + s->tracks;
> }
>
>
>
> - better error message, and we check exactly what's written in the
> spec (docs/interop/parallels.c):
>
>
> Cluster offsets specified by BAT entries must meet the following
> requirements:
> [...]
> - the value must be lower than the desired file size,
>
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] parallels: Out of image offset in BAT leads to image inflation
2022-08-23 13:50 ` Alexander Ivanov
@ 2022-08-23 15:43 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-08-23 15:43 UTC (permalink / raw)
To: Alexander Ivanov, Denis V. Lunev, qemu-devel
Cc: qemu-block, stefanha, kwolf, hreitz
On 8/23/22 16:50, Alexander Ivanov wrote:
>
> On 23.08.2022 11:58, Vladimir Sementsov-Ogievskiy wrote:
>> On 8/23/22 12:20, Denis V. Lunev wrote:
>>> On 23.08.2022 09:23, Alexander Ivanov wrote:
>>>>
>>>> On 23.08.2022 08:58, Vladimir Sementsov-Ogievskiy wrote:
>>>>> On 8/22/22 12:05, Alexander Ivanov wrote:
>>>>>> data_end field in BDRVParallelsState is set to the biggest offset present
>>>>>> in BAT. If this offset is outside of the image, any further write will create
>>>>>> the cluster at this offset and/or the image will be truncated to this
>>>>>> offset on close. This is definitely not correct.
>>>>>> Raise an error in parallels_open() if data_end points outside the image and
>>>>>> it is not a check (let the check to repaire the image).
>>>>>>
>>>>>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>>>>>> ---
>>>>>> block/parallels.c | 14 ++++++++++++++
>>>>>> 1 file changed, 14 insertions(+)
>>>>>>
>>>>>> diff --git a/block/parallels.c b/block/parallels.c
>>>>>> index a229c06f25..c245ca35cd 100644
>>>>>> --- a/block/parallels.c
>>>>>> +++ b/block/parallels.c
>>>>>> @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>>>>> BDRVParallelsState *s = bs->opaque;
>>>>>> ParallelsHeader ph;
>>>>>> int ret, size, i;
>>>>>> + int64_t file_size;
>>>>>> QemuOpts *opts = NULL;
>>>>>> Error *local_err = NULL;
>>>>>> char *buf;
>>>>>> @@ -811,6 +812,19 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>>>>> }
>>>>>> }
>>>>>> + file_size = bdrv_getlength(bs->file->bs);
>>>>>> + if (file_size < 0) {
>>>>>> + ret = file_size;
>>>>>> + goto fail;
>>>>>> + }
>>>>>> +
>>>>>> + file_size >>= BDRV_SECTOR_BITS;
>>>>>> + if (s->data_end > file_size && !(flags & BDRV_O_CHECK)) {
>>>>>> + error_setg(errp, "parallels: Offset in BAT is out of image");
>>>>>> + ret = -EINVAL;
>>>>>> + goto fail;
>>>>>> + }
>>>>>
>>>>> If image is unaligned to sector size, and image size is less than s->data_end, but the difference itself is less than sector, the error message would be misleading.
>>>>>
>>>>> Should we consider "file_size = DIV_ROUND_UP(file_size, BDRV_SECTOR_SIZE)" instead of "file_size >>= BDRV_SECTOR_BITS"?
>>>>>
>>>>> It's hardly possible to get such image on valid scenarios with Qemu (keeping in mind bdrv_truncate() call in parallels_close()). But it still may be possible to have such images produced by another software or by some failure path.
>>>>>
>>>> I think you are right, it would be better to align image size up to sector size.
>>>
>>> I would say that we need to align not on sector size but on cluster size.
>>> That would worth additional check.
>>
>> And not simply align, as data_offset is not necessarily aligned to cluster size.
>>
>> Finally, what should we check?
>>
>> I suggest
>>
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index 6d4ed77f16..b882ea1200 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -725,6 +725,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>> BDRVParallelsState *s = bs->opaque;
>> ParallelsHeader ph;
>> int ret, size, i;
>> + int64_t file_size;
>> QemuOpts *opts = NULL;
>> Error *local_err = NULL;
>> char *buf;
>> @@ -735,6 +736,11 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>> return -EINVAL;
>> }
>>
>> + file_size = bdrv_getlength(bs->file->bs);
>> + if (file_size < 0) {
>> + return file_size;
>> + }
>> +
>> ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph));
>> if (ret < 0) {
>> goto fail;
>> @@ -798,6 +804,13 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>
>> for (i = 0; i < s->bat_size; i++) {
>> int64_t off = bat2sect(s, i);
>> + if (off >= file_size) {
>
> Do we really want to compare file size with cluster offset, not with cluster end?
the spec say:
Cluster offsets specified by BAT entries must meet the following requirements:
[...]
- the value must be lower than the desired file size,
>
> Also I would leave the comparison outside the loop, because I'm going to move highest offset calculation to a helper (there are two places with the same code).
benefits of check inside the loop:
1. we get good error message with BAT index and problematic offset
2. we are sure that data_end is produced by real cluster. After the loop you'll have to consider image with no clusters separately
>
>> + error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
>> + "is larger than file size (%" PRIi64 ")",
>> + off, i, file_size);
>> + ret = -EINVAL;
>> + goto fail;
>> + }
>> if (off >= s->data_end) {
>> s->data_end = off + s->tracks;
>> }
>>
>>
>>
>> - better error message, and we check exactly what's written in the spec (docs/interop/parallels.c):
>>
>>
>> Cluster offsets specified by BAT entries must meet the following requirements:
>> [...]
>> - the value must be lower than the desired file size,
>>
>>
>>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v5 1/9] parallels: Out of image offset in BAT leads to image inflation
2022-08-23 10:11 ` Denis V. Lunev
@ 2022-08-24 8:50 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-08-24 8:50 UTC (permalink / raw)
To: Denis V. Lunev, Alexander Ivanov, qemu-devel
Cc: qemu-block, stefanha, kwolf, hreitz
On 8/23/22 13:11, Denis V. Lunev wrote:
> On 23.08.2022 11:58, Vladimir Sementsov-Ogievskiy wrote:
>> On 8/23/22 12:20, Denis V. Lunev wrote:
>>> On 23.08.2022 09:23, Alexander Ivanov wrote:
>>>>
>>>> On 23.08.2022 08:58, Vladimir Sementsov-Ogievskiy wrote:
>>>>> On 8/22/22 12:05, Alexander Ivanov wrote:
>>>>>> data_end field in BDRVParallelsState is set to the biggest offset present
>>>>>> in BAT. If this offset is outside of the image, any further write will create
>>>>>> the cluster at this offset and/or the image will be truncated to this
>>>>>> offset on close. This is definitely not correct.
>>>>>> Raise an error in parallels_open() if data_end points outside the image and
>>>>>> it is not a check (let the check to repaire the image).
>>>>>>
>>>>>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>>>>>> ---
>>>>>> block/parallels.c | 14 ++++++++++++++
>>>>>> 1 file changed, 14 insertions(+)
>>>>>>
>>>>>> diff --git a/block/parallels.c b/block/parallels.c
>>>>>> index a229c06f25..c245ca35cd 100644
>>>>>> --- a/block/parallels.c
>>>>>> +++ b/block/parallels.c
>>>>>> @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>>>>> BDRVParallelsState *s = bs->opaque;
>>>>>> ParallelsHeader ph;
>>>>>> int ret, size, i;
>>>>>> + int64_t file_size;
>>>>>> QemuOpts *opts = NULL;
>>>>>> Error *local_err = NULL;
>>>>>> char *buf;
>>>>>> @@ -811,6 +812,19 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>>>>> }
>>>>>> }
>>>>>> + file_size = bdrv_getlength(bs->file->bs);
>>>>>> + if (file_size < 0) {
>>>>>> + ret = file_size;
>>>>>> + goto fail;
>>>>>> + }
>>>>>> +
>>>>>> + file_size >>= BDRV_SECTOR_BITS;
>>>>>> + if (s->data_end > file_size && !(flags & BDRV_O_CHECK)) {
>>>>>> + error_setg(errp, "parallels: Offset in BAT is out of image");
>>>>>> + ret = -EINVAL;
>>>>>> + goto fail;
>>>>>> + }
>>>>>
>>>>> If image is unaligned to sector size, and image size is less than s->data_end, but the difference itself is less than sector, the error message would be misleading.
>>>>>
>>>>> Should we consider "file_size = DIV_ROUND_UP(file_size, BDRV_SECTOR_SIZE)" instead of "file_size >>= BDRV_SECTOR_BITS"?
>>>>>
>>>>> It's hardly possible to get such image on valid scenarios with Qemu (keeping in mind bdrv_truncate() call in parallels_close()). But it still may be possible to have such images produced by another software or by some failure path.
>>>>>
>>>> I think you are right, it would be better to align image size up to sector size.
>>>
>>> I would say that we need to align not on sector size but on cluster size.
>>> That would worth additional check.
>>
>> And not simply align, as data_offset is not necessarily aligned to cluster size.
>>
>> Finally, what should we check?
>>
>> I suggest
>>
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index 6d4ed77f16..b882ea1200 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -725,6 +725,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>> BDRVParallelsState *s = bs->opaque;
>> ParallelsHeader ph;
>> int ret, size, i;
>> + int64_t file_size;
>> QemuOpts *opts = NULL;
>> Error *local_err = NULL;
>> char *buf;
>> @@ -735,6 +736,11 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>> return -EINVAL;
>> }
>>
>> + file_size = bdrv_getlength(bs->file->bs);
>> + if (file_size < 0) {
>> + return file_size;
>> + }
>> +
>> ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph));
>> if (ret < 0) {
>> goto fail;
>> @@ -798,6 +804,13 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>
>> for (i = 0; i < s->bat_size; i++) {
>> int64_t off = bat2sect(s, i);
>> + if (off >= file_size) {
> Like this, especially >= check which we have had missed.
> Though this would break the repair. We need additional
>
> if (flags & BDRV_O_CHECK) {
> continue;
> }
>
> No incorrect data_end assignment, which would be
> very welcome.
>
> Den
'continue' here will change the logic around data_end. We'll drop "wrong" clusters from calculation of data_end, and should check, how it affects further logic.
What about:
for (i = 0; i < s->bat_size; i++) {
int64_t off = bat2sect(s, i);
if (off >= file_size && !(flags & BDRV_O_CHECK)) {
error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
"is larger than file size (%" PRIi64 ")",
off, i, file_size);
ret = -EINVAL;
goto fail;
}
if (off >= s->data_end) {
s->data_end = off + s->tracks;
}
}
- this we simply add new error-out on no-O_CHECK path.
>
>> + error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
>> + "is larger than file size (%" PRIi64 ")",
>> + off, i, file_size);
>> + ret = -EINVAL;
>> + goto fail;
>> + }
>> if (off >= s->data_end) {
>> s->data_end = off + s->tracks;
>> }
>>
>>
>>
>> - better error message, and we check exactly what's written in the spec (docs/interop/parallels.c):
>>
>>
>> Cluster offsets specified by BAT entries must meet the following requirements:
>> [...]
>> - the value must be lower than the desired file size,
>>
>>
>>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2022-08-24 8:55 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-22 9:05 [PATCH v5 0/9] parallels: Refactor the code of images checks and fix a bug Alexander Ivanov
2022-08-22 9:05 ` [PATCH v5 1/9] parallels: Out of image offset in BAT leads to image inflation Alexander Ivanov
2022-08-23 6:58 ` Vladimir Sementsov-Ogievskiy
2022-08-23 7:23 ` Alexander Ivanov
2022-08-23 7:59 ` Vladimir Sementsov-Ogievskiy
2022-08-23 9:45 ` Vladimir Sementsov-Ogievskiy
2022-08-23 9:20 ` Denis V. Lunev
2022-08-23 9:49 ` Alexander Ivanov
2022-08-23 9:58 ` Vladimir Sementsov-Ogievskiy
2022-08-23 10:11 ` Denis V. Lunev
2022-08-24 8:50 ` Vladimir Sementsov-Ogievskiy
2022-08-23 13:50 ` Alexander Ivanov
2022-08-23 15:43 ` Vladimir Sementsov-Ogievskiy
2022-08-23 9:34 ` Denis V. Lunev
2022-08-23 9:47 ` Vladimir Sementsov-Ogievskiy
2022-08-23 10:02 ` Denis V. Lunev
2022-08-23 9:51 ` Denis V. Lunev
2022-08-22 9:05 ` [PATCH v5 2/9] parallels: Fix data_end field value in parallels_co_check() Alexander Ivanov
2022-08-23 7:38 ` Vladimir Sementsov-Ogievskiy
2022-08-23 9:00 ` Alexander Ivanov
2022-08-23 9:23 ` Denis V. Lunev
2022-08-22 9:05 ` [PATCH v5 3/9] parallels: create parallels_set_bat_entry_helper() to assign BAT value Alexander Ivanov
2022-08-22 9:05 ` [PATCH v5 4/9] parallels: Use generic infrastructure for BAT writing in parallels_co_check() Alexander Ivanov
2022-08-23 9:36 ` Denis V. Lunev
2022-08-22 9:05 ` [PATCH v5 5/9] parallels: Move check of unclean image to a separate function Alexander Ivanov
2022-08-22 9:05 ` [PATCH v5 6/9] parallels: Move check of cluster outside " Alexander Ivanov
2022-08-22 9:05 ` [PATCH v5 7/9] parallels: Move check of leaks " Alexander Ivanov
2022-08-23 9:40 ` Denis V. Lunev
2022-08-23 9:56 ` Alexander Ivanov
2022-08-22 9:05 ` [PATCH v5 8/9] parallels: Move statistic collection " Alexander Ivanov
2022-08-22 9:05 ` [PATCH v5 9/9] parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD Alexander Ivanov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.