[Devel] [PATCH hci-8.1 v2 0/3] dm-qcow2: fix merges not stopping the users of the images they change

[Devel] [PATCH hci-8.1 v2 0/3] dm-qcow2: fix merges not stopping the users of the images they change

[Denis V. Lunev]

Checking dm-qcow2 for the same problem showed that a merge does not
stop everything using the images it changes (VSTOR-153049,
VSTOR-153050):

- a backward merge of an inner image pauses that image, while reads
  enter through the top one. A read reaching the inner image as the
  merge starts oopses on a NULL qio->ext (patch 2);

- SEEK_HOLE/SEEK_DATA walks the images with nothing keeping them
  alive, so merge_backward complete frees an image under the walk
  (patch 3).

Patch 1 (VSTOR-153051) is the counterpart of dm-ploop patch 4, an
error path leaving state behind: after a failed refcount table
relocation, allocating writes past the old refcount coverage fail with
EIO until the device is suspended and resumed.

The series does not depend on the dm-ploop one.

Tested on dev-cl04, stock 6.12.0-211.52.1.22.11.vz10, dm-qcow2 built
out of tree, with a test-only kprobe module widening the windows:
- on the stock module, with 4K clusters and refcount_bits=64 so the
  reftable relocates at 1G, a failed md page insert left every later
  write past 1G failing with EIO; a seek went on with an image
  qcow2_destroy() had freed; an intermediate merge oopsed in
  submit_read_sliced_clu(). None of them reproduce with the series;
- the ploop pytest suite, qcow2 format, passes.

Changes in v2, after Sashiko's review:
- 1/3: a note in the commit message on why the error path does not
  unlink the pages from the writeback lists;
- 3/3: guard the walk with a per-target rw_semaphore, taken for write
  by merge_backward complete, instead of an inflight ref held over the
  walk. With the ref every inflight ref switch waited for the whole
  scan, including the one starting a backward merge with the top image
  paused, so guest I/O stalled for as long as the scan ran.
The seek reproducer and the qcow2 pytest suite were rerun on v2: a
merge now starts while a seek is held, and merge_backward complete
waits for the seek to finish before destroying the image.

v1: http://lore.virtuozzo.com/kernel/20261103083708.131379-1-den@openvz.org/

Denis V. Lunev (3):
  dm-qcow2: fix a failed refcount table relocation leaking md pages
  dm-qcow2: fix backward merge of an inner image racing its reads
  dm-qcow2: fix SEEK_HOLE/SEEK_DATA using an image freed by a merge

 drivers/md/dm-qcow2-cmd.c    | 28 ++++++++++++----------------
 drivers/md/dm-qcow2-map.c    | 18 +++++++++++++-----
 drivers/md/dm-qcow2-target.c |  1 +
 drivers/md/dm-qcow2.h        |  2 ++
 4 files changed, 28 insertions(+), 21 deletions(-)


base-commit: 09eb9a82fe872048d082391415d1cdeda2ac6879
-- 
2.53.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH hci-8.1 v2 1/3] dm-qcow2: fix a failed refcount table relocation leaking md pages

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

relocate_refcount_table() inserts md pages for the last cluster of the
new reftable and for the new refblocks, one page at a time from
pos + old_clus * clu_size up to r2_end. On error it frees pages from
end only, one per cluster, so the pages of the new reftable cluster
and all but the first page of each refblock cluster bigger than a page
stay in md_pages.

The header is rolled back, so the next allocation past the old refcount
coverage relocates the table again. Its first insert finds a leaked page
and fails with -EEXIST, the relocation returns -EIO, and every
allocating write beyond the old coverage fails with an I/O error until
the device is suspended and resumed or the table is reloaded. A failed
page allocation or a failed header write is enough to get there.

Free exactly the pages the allocation loop inserted.

Note for a future reader: the pages are freed without being unlinked
from the writeback lists. That is safe for the failures above, which
happen before any page in the range is dirtied. Pages get dirty only
after the header write, and the gotos after it are WARN_ON_ONCE()
branches that leave the on-disk header pointing at the new table. Those
want the image switched to read-only, as the TODO there says, not more
cleanup.

Fixes: 222b6f1291c1 ("dm-qcow2: Introduce driver to create block devices over QCOW2 files")
https://virtuozzo.atlassian.net/browse/VSTOR-153051
Feature: dm-qcow2: block device over QCOW2 files driver
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 drivers/md/dm-qcow2-map.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index 78ee7cf5e881..8ab7cb472560 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -2279,6 +2279,7 @@ static void set_reftable_in_raw_hdr(struct page *page0, loff_t pos, loff_t clus)
 static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
 {
 	loff_t i, old_pos, old_end, pos, end, r2_end, delta;
+	loff_t new_md_start, new_md_end;
 	u32 old_clus, clus, clu_size = qcow2->clu_size;
 	u32 r2_clus, bits = qcow2->refblock_bits;
 	unsigned long nr_pages, index;
@@ -2342,8 +2343,9 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
 		return ret;
 
 	/* Alloc R1/R2 pages covering clusters of new R1 and new R2 */
-	for (i = pos + (u64)old_clus * clu_size; i < r2_end; i += PAGE_SIZE) {
-		ret = qcow2_alloc_and_insert_md_page(qcow2, i >> PAGE_SHIFT, &md);
+	new_md_start = pos + (u64)old_clus * clu_size;
+	for (new_md_end = new_md_start; new_md_end < r2_end; new_md_end += PAGE_SIZE) {
+		ret = qcow2_alloc_and_insert_md_page(qcow2, new_md_end >> PAGE_SHIFT, &md);
 		if (ret < 0)
 			goto err_free_r2_pages;
 		spin_lock_irq(&qcow2->md_pages_lock);
@@ -2419,7 +2421,7 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
 	return 1;
 
 err_free_r2_pages:
-	for (i = end; i < r2_end; i += clu_size) {
+	for (i = new_md_start; i < new_md_end; i += PAGE_SIZE) {
 		ret = handle_md_page(qcow2, i >> PAGE_SHIFT, NULL, &md);
 		if (ret <= 0)
 			break;
-- 
2.53.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH hci-8.1 v2 2/3] dm-qcow2: fix backward merge of an inner image racing its reads

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

set_backward_merge_in_process() sets the flag of an image other than the
top one without stopping submission, as nothing writes to it. It is
still read: a read of a cluster the top image does not map goes on to
the lower image, and when that image is writable its worker tests the
flag twice for the qio. When the merge starts between the two tests, the
first one, in handle_metadata(), skips prepare_backward_merge() and
leaves qio->ext NULL, the second one, in process_one_qio(), takes the
merge path and submit_top_delta_read() dereferences qio->ext.

Pause submission and switch the inflight refs for every image. The
pause flag is only tested on the top image, where all qios enter, so
pause that one; clearing the flag of an inner image paused the inner
image itself, which held nothing back.

Fixes: 8e522723a085 ("dm-qcow2: allow specifying depth to merge intermediate images")
https://virtuozzo.atlassian.net/browse/VSTOR-153049
Feature: dm-qcow2: block device over QCOW2 files driver
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 drivers/md/dm-qcow2-cmd.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/md/dm-qcow2-cmd.c b/drivers/md/dm-qcow2-cmd.c
index e520d8fd6929..cb28cf6456df 100644
--- a/drivers/md/dm-qcow2-cmd.c
+++ b/drivers/md/dm-qcow2-cmd.c
@@ -149,34 +149,28 @@ static int qcow2_break_l1cow(struct qcow2_target *tgt, struct qcow2 *qcow2)
 static void set_backward_merge_in_process(struct qcow2_target *tgt,
 				     struct qcow2 *qcow2, bool set)
 {
+	struct qcow2 *top = tgt->top;
 	LIST_HEAD(list);
 
-	/*
-	 * There are no writes if it is not the top qcow2 image.
-	 * so we do not need to stop and flush requests when setting
-	 */
-	if (qcow2 != tgt->top && set) {
-		qcow2->backward_merge_in_process = set;
-		return;
-	}
-
 	/*
 	 * To avoid race between allocations and COWS
 	 * we completely stop queueing qios and wait
 	 * for pending qios. Lock is for visability.
+	 * A lower image has no writes, but it is read by
+	 * qios of the top one, so stop the top image even then.
 	 */
-	spin_lock_irq(&qcow2->deferred_lock);
-	qcow2->pause_submitting_qios = true;
-	spin_unlock_irq(&qcow2->deferred_lock);
+	spin_lock_irq(&top->deferred_lock);
+	top->pause_submitting_qios = true;
+	spin_unlock_irq(&top->deferred_lock);
 	qcow2_inflight_ref_switch(tgt);
 
 	/* queue is stopped */
-	spin_lock_irq(&qcow2->deferred_lock);
+	spin_lock_irq(&top->deferred_lock);
 	WARN_ON_ONCE(qcow2->backward_merge_in_process == set);
 	qcow2->backward_merge_in_process = set;
-	qcow2->pause_submitting_qios = false;
-	list_splice_init(&qcow2->paused_qios, &list);
-	spin_unlock_irq(&qcow2->deferred_lock);
+	top->pause_submitting_qios = false;
+	list_splice_init(&top->paused_qios, &list);
+	spin_unlock_irq(&top->deferred_lock);
 
 	qcow2_submit_embedded_qios(tgt, &list);
 }
-- 
2.53.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH hci-8.1 v2 3/3] dm-qcow2: fix SEEK_HOLE/SEEK_DATA using an image freed by a merge

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

qcow2_llseek_hole() starts at tgt->top and follows qcow2->lower, parsing
the metadata of each image, with nothing keeping the images alive. dm
core holds only the table SRCU around the call, which does not keep a
target message out. merge_backward complete unlinks the merged image,
waits in qcow2_inflight_ref_switch() for the qios holding refs and
destroys the image, so a seek already walking it goes on with a freed
struct qcow2, its md pages and its file. A SEEK_DATA scan of a large
sparse image keeps the walk running long enough to hit it.

Hold a per-target rw_semaphore for read over the walk, and for write
while merge_backward complete unlinks and destroys the image. An
inflight ref held over the walk would also do, but every inflight ref
switch would then wait for the whole scan, including the one starting a
backward merge with the top image paused, which stalls guest I/O for as
long as the scan runs.

Fixes: 412d5a4f7bf1 ("dm/dm-qcow2: add llseek_hole")
https://virtuozzo.atlassian.net/browse/VSTOR-153050
Feature: dm-qcow2: block device over QCOW2 files driver
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 drivers/md/dm-qcow2-cmd.c    |  2 ++
 drivers/md/dm-qcow2-map.c    | 10 ++++++++--
 drivers/md/dm-qcow2-target.c |  1 +
 drivers/md/dm-qcow2.h        |  2 ++
 4 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-qcow2-cmd.c b/drivers/md/dm-qcow2-cmd.c
index cb28cf6456df..d9a6c823b817 100644
--- a/drivers/md/dm-qcow2-cmd.c
+++ b/drivers/md/dm-qcow2-cmd.c
@@ -373,6 +373,7 @@ static int qcow2_merge_backward_complete(struct qcow2_target *tgt)
 	if (tgt->backward_merge.state != BACKWARD_MERGE_WAIT_COMPLETION)
 		return -EBUSY;
 
+	down_write(&tgt->seek_rwsem);
 	qcow2 = *tgt->backward_merge.pqcow2;
 	*tgt->backward_merge.pqcow2 = qcow2->lower;
 
@@ -394,6 +395,7 @@ static int qcow2_merge_backward_complete(struct qcow2_target *tgt)
 			QC_ERR(tgt->ti, "Can't unuse merged img (%d)", ret);
 	}
 	qcow2_destroy(qcow2);
+	up_write(&tgt->seek_rwsem);
 
 	tgt->backward_merge.state = BACKWARD_MERGE_STOPPED;
 	__backward_merge_update_stage(tgt, BACKWARD_MERGE_STAGE_NONE);
diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index 8ab7cb472560..b180d8ecf7b4 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -4841,18 +4841,24 @@ static int qcow2_llseek_hole_qio(struct qio *qio, int whence, loff_t *result)
 
 loff_t qcow2_llseek_hole(struct dm_target *ti, loff_t offset, int whence)
 {
-	struct qcow2 *qcow2 = to_qcow2_target(ti)->top;
+	struct qcow2_target *tgt = to_qcow2_target(ti);
 	loff_t result = -EINVAL;
+	struct qcow2 *qcow2;
 	struct qio *qio;
 	int ret;
 
+	down_read(&tgt->seek_rwsem);
+	qcow2 = tgt->top;
 	qio = alloc_seek_qio(qcow2, NULL, qcow2->hdr.size);
-	if (!qio)
+	if (!qio) {
+		up_read(&tgt->seek_rwsem);
 		return -ENOMEM;
+	}
 
 	seek_qio_set_sector(qio, to_sector(offset));
 
 	ret = qcow2_llseek_hole_qio(qio, whence, &result);
+	up_read(&tgt->seek_rwsem);
 	/* In case of error remap ENXIO as it have special meaning for llseek */
 	if (ret < 0)
 		return (ret == -ENXIO) ? -EINVAL : ret;
diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
index be09615eb8e2..343957730022 100644
--- a/drivers/md/dm-qcow2-target.c
+++ b/drivers/md/dm-qcow2-target.c
@@ -491,6 +491,7 @@ static struct qcow2_target *alloc_qcow2_target(struct dm_target *ti)
 	init_completion(&tgt->inflight_ref_comp);
 	spin_lock_init(&tgt->event_lock);
 	mutex_init(&tgt->ctl_mutex);
+	init_rwsem(&tgt->seek_rwsem);
 	init_waitqueue_head(&tgt->service_wq);
 	INIT_WORK(&tgt->event_work, qcow2_event_work);
 	INIT_LIST_HEAD(&tgt->enospc_qios);
diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
index d9b8c38e093f..de685300cb48 100644
--- a/drivers/md/dm-qcow2.h
+++ b/drivers/md/dm-qcow2.h
@@ -212,6 +212,8 @@ struct qcow2_target {
 	struct work_struct event_work;
 	spinlock_t event_lock;
 	struct mutex ctl_mutex;
+	/* Keeps merge_backward complete from freeing images a seek walks */
+	struct rw_semaphore seek_rwsem;
 
 	struct qcow2_backward_merge backward_merge;
 };
-- 
2.53.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v2 1/3] dm-qcow2: fix a failed refcount table relocation leaking md pages

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dm-qcow2: err_free_r2_pages frees dirty md pages still on writeback lists
--

commit 774fe387096c63a6d4048c14687fd00ee87e6e1a
Author: Denis V. Lunev <den@openvz.org>

dm-qcow2: fix a failed refcount table relocation leaking md pages

This patch changes the error path of relocate_refcount_table() so that it
frees exactly the md pages the allocation loop inserted. Before, it freed
only one page per cluster starting from end. The patch tracks the range with
new_md_start and new_md_end and frees every page in that range.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-153051

> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
> --- a/drivers/md/dm-qcow2-map.c
> +++ b/drivers/md/dm-qcow2-map.c

[ ... ]

> @@ -2342,8 +2343,9 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
>  		return ret;
>  
>  	/* Alloc R1/R2 pages covering clusters of new R1 and new R2 */
> -	for (i = pos + (u64)old_clus * clu_size; i < r2_end; i += PAGE_SIZE) {
> -		ret = qcow2_alloc_and_insert_md_page(qcow2, i >> PAGE_SHIFT, &md);
> +	new_md_start = pos + (u64)old_clus * clu_size;
> +	for (new_md_end = new_md_start; new_md_end < r2_end; new_md_end += PAGE_SIZE) {
> +		ret = qcow2_alloc_and_insert_md_page(qcow2, new_md_end >> PAGE_SHIFT, &md);
>  		if (ret < 0)
>  			goto err_free_r2_pages;

[ ... ]

> @@ -2419,7 +2421,7 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
>  	return 1;
>  
>  err_free_r2_pages:
> -	for (i = end; i < r2_end; i += clu_size) {
> +	for (i = new_md_start; i < new_md_end; i += PAGE_SIZE) {
>  		ret = handle_md_page(qcow2, i >> PAGE_SHIFT, NULL, &md);
>  		if (ret <= 0)
>  			break;
>  		spin_lock_irq(&qcow2->md_pages_lock);
>  		qcow2_md_page_erase(qcow2, md);
>  		spin_unlock_irq(&qcow2->md_pages_lock);
>  		qcow2_free_md_page(md);
>  	}

[Severity: Medium]
Can this cleanup free md pages that are still linked on a writeback list?

The loop now erases and frees every page in [new_md_start, new_md_end).
It does not unlink md->wb_link, and it does not check MD_DIRTY or
MD_WRITEBACK.

Several gotos to err_free_r2_pages come after the header write, once pages
in this range may already be dirty:

  - the renumber loop, when qcow2_md_page_renumber() returns NULL
    (md_page_try_insert() failure, or error injection through
    ALLOW_ERROR_INJECTION)
  - the "Connect new R2 to new R1" loop, on calc_refcounters_map() or
    handle_md_page() failure
  - the mark_cluster_used() and mark_cluster_unused() loops, on
    qcow2_handle_r1r2_maps() failure. This includes the non-WARN -EIO
    from the "refblock=%llu, while no snapshots" sanity check.

By then qcow2_md_make_dirty() may have run on pages in the range:

qcow2_md_make_dirty() {
    if (!(md->status & MD_DIRTY)) {
        md->status |= MD_DIRTY;
        list_add_tail(&md->wb_link, head);
        ...
        mod_timer(&qcow2->slow_wb_timer, jiffies + WB_TIMEOUT_JI);

qcow2_md_page_erase() is only an rb_erase(). qcow2_free_md_page() is only
put_page() plus kfree(). Neither touches wb_link.

A freed struct md_page could then stay on wb_batch_list or
slow_wb_batch_list. A later writeback pass or slow_wb_timer would touch
freed memory.

The commit message says this is safe because "the gotos after it are
WARN_ON_ONCE() branches" that "want the image switched to read-only".
Is that accurate?

There is still no switch to read-only here, only the "TODO: switch to RO"
comment followed by return -EIO.

Some of the late branches are not WARN_ON_ONCE() branches. One example is
the -EIO from the "while no snapshots" check in qcow2_handle_r1r2_maps().

R2 pages could already be dirtied by mark_cluster_used() before this patch.
The R1 tail page and the additional pages of each R2 cluster are newly
exposed to this cleanup.

Would it make sense to unlink wb_link for dirty pages in this loop, or to
skip freeing the pages once the header write has succeeded?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261103090813.166986-1-den@openvz.org?part=1

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v2 2/3] dm-qcow2: fix backward merge of an inner image racing its reads

[no-reply]

> From: Denis V. Lunev <den@openvz.org>
> 
> set_backward_merge_in_process() sets the flag of an image other than the
> top one without stopping submission, as nothing writes to it. It is
> still read: a read of a cluster the top image does not map goes on to
> the lower image, and when that image is writable its worker tests the
> flag twice for the qio. When the merge starts between the two tests, the
> first one, in handle_metadata(), skips prepare_backward_merge() and
> leaves qio->ext NULL, the second one, in process_one_qio(), takes the
> merge path and submit_top_delta_read() dereferences qio->ext.
> 
> Pause submission and switch the inflight refs for every image. The
> pause flag is only tested on the top image, where all qios enter, so
> pause that one; clearing the flag of an inner image paused the inner
> image itself, which held nothing back.
> 
> Fixes: 8e522723a085 ("dm-qcow2: allow specifying depth to merge intermediate images")
> https://virtuozzo.atlassian.net/browse/VSTOR-153049
> Feature: dm-qcow2: block device over QCOW2 files driver
> Signed-off-by: Denis V. Lunev <den@openvz.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261103090813.166986-1-den@openvz.org?part=2


_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v2 3/3] dm-qcow2: fix SEEK_HOLE/SEEK_DATA using an image freed by a merge

[no-reply]

> From: Denis V. Lunev <den@openvz.org>
> 
> qcow2_llseek_hole() starts at tgt->top and follows qcow2->lower, parsing
> the metadata of each image, with nothing keeping the images alive. dm
> core holds only the table SRCU around the call, which does not keep a
> target message out. merge_backward complete unlinks the merged image,
> waits in qcow2_inflight_ref_switch() for the qios holding refs and
> destroys the image, so a seek already walking it goes on with a freed
> struct qcow2, its md pages and its file. A SEEK_DATA scan of a large
> sparse image keeps the walk running long enough to hit it.
> 
> Hold a per-target rw_semaphore for read over the walk, and for write
> while merge_backward complete unlinks and destroys the image. An
> inflight ref held over the walk would also do, but every inflight ref
> switch would then wait for the whole scan, including the one starting a
> backward merge with the top image paused, which stalls guest I/O for as
> long as the scan runs.
> 
> Fixes: 412d5a4f7bf1 ("dm/dm-qcow2: add llseek_hole")
> https://virtuozzo.atlassian.net/browse/VSTOR-153050
> Feature: dm-qcow2: block device over QCOW2 files driver
> Signed-off-by: Denis V. Lunev <den@openvz.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261103090813.166986-1-den@openvz.org?part=3


_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v2 1/3] dm-qcow2: fix a failed refcount table relocation leaking md pages

[Denis V. Lunev]

On 11/3/26 10:13, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] dm-qcow2: err_free_r2_pages frees dirty md pages still on writeback lists
> --
>
> commit 774fe387096c63a6d4048c14687fd00ee87e6e1a
> Author: Denis V. Lunev <den@openvz.org>
>
> dm-qcow2: fix a failed refcount table relocation leaking md pages
>
> This patch changes the error path of relocate_refcount_table() so that it
> frees exactly the md pages the allocation loop inserted. Before, it freed
> only one page per cluster starting from end. The patch tracks the range with
> new_md_start and new_md_end and frees every page in that range.
>
> Link: https://virtuozzo.atlassian.net/browse/VSTOR-153051
>
>> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
>> --- a/drivers/md/dm-qcow2-map.c
>> +++ b/drivers/md/dm-qcow2-map.c
> [ ... ]
>
>> @@ -2342,8 +2343,9 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
>>  		return ret;
>>  
>>  	/* Alloc R1/R2 pages covering clusters of new R1 and new R2 */
>> -	for (i = pos + (u64)old_clus * clu_size; i < r2_end; i += PAGE_SIZE) {
>> -		ret = qcow2_alloc_and_insert_md_page(qcow2, i >> PAGE_SHIFT, &md);
>> +	new_md_start = pos + (u64)old_clus * clu_size;
>> +	for (new_md_end = new_md_start; new_md_end < r2_end; new_md_end += PAGE_SIZE) {
>> +		ret = qcow2_alloc_and_insert_md_page(qcow2, new_md_end >> PAGE_SHIFT, &md);
>>  		if (ret < 0)
>>  			goto err_free_r2_pages;
> [ ... ]
>
>> @@ -2419,7 +2421,7 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
>>  	return 1;
>>  
>>  err_free_r2_pages:
>> -	for (i = end; i < r2_end; i += clu_size) {
>> +	for (i = new_md_start; i < new_md_end; i += PAGE_SIZE) {
>>  		ret = handle_md_page(qcow2, i >> PAGE_SHIFT, NULL, &md);
>>  		if (ret <= 0)
>>  			break;
>>  		spin_lock_irq(&qcow2->md_pages_lock);
>>  		qcow2_md_page_erase(qcow2, md);
>>  		spin_unlock_irq(&qcow2->md_pages_lock);
>>  		qcow2_free_md_page(md);
>>  	}
> [Severity: Medium]
> Can this cleanup free md pages that are still linked on a writeback list?
>
> The loop now erases and frees every page in [new_md_start, new_md_end).
> It does not unlink md->wb_link, and it does not check MD_DIRTY or
> MD_WRITEBACK.
>
> Several gotos to err_free_r2_pages come after the header write, once pages
> in this range may already be dirty:
>
>   - the renumber loop, when qcow2_md_page_renumber() returns NULL
>     (md_page_try_insert() failure, or error injection through
>     ALLOW_ERROR_INJECTION)
>   - the "Connect new R2 to new R1" loop, on calc_refcounters_map() or
>     handle_md_page() failure
>   - the mark_cluster_used() and mark_cluster_unused() loops, on
>     qcow2_handle_r1r2_maps() failure. This includes the non-WARN -EIO
>     from the "refblock=%llu, while no snapshots" sanity check.
>
> By then qcow2_md_make_dirty() may have run on pages in the range:
>
> qcow2_md_make_dirty() {
>     if (!(md->status & MD_DIRTY)) {
>         md->status |= MD_DIRTY;
>         list_add_tail(&md->wb_link, head);
>         ...
>         mod_timer(&qcow2->slow_wb_timer, jiffies + WB_TIMEOUT_JI);
>
> qcow2_md_page_erase() is only an rb_erase(). qcow2_free_md_page() is only
> put_page() plus kfree(). Neither touches wb_link.
>
> A freed struct md_page could then stay on wb_batch_list or
> slow_wb_batch_list. A later writeback pass or slow_wb_timer would touch
> freed memory.
>
> The commit message says this is safe because "the gotos after it are
> WARN_ON_ONCE() branches" that "want the image switched to read-only".
> Is that accurate?
>
> There is still no switch to read-only here, only the "TODO: switch to RO"
> comment followed by return -EIO.
>
> Some of the late branches are not WARN_ON_ONCE() branches. One example is
> the -EIO from the "while no snapshots" check in qcow2_handle_r1r2_maps().
>
> R2 pages could already be dirtied by mark_cluster_used() before this patch.
> The R1 tail page and the additional pages of each R2 cluster are newly
> exposed to this cleanup.
>
> Would it make sense to unlink wb_link for dirty pages in this loop, or to
> skip freeing the pages once the header write has succeeded?
>
the pages are freed without being unlinked from 
the writeback lists. That is safe for the failures above, which happen 
before any page in the range is dirtied. Pages get dirty only after the 
header write, and the gotos after it are WARN_ON_ONCE() branches that leave 
the on-disk header pointing at the new table. Those want the image switched 
to read-only, as the TODO there says, not more cleanup.

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v2 0/3] dm-qcow2: fix merges not stopping the users of the images they change

[Andrey Zhadchenko]

Reviewed-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

On 11/3/26 10:08, Denis V. Lunev wrote:
> Checking dm-qcow2 for the same problem showed that a merge does not
> stop everything using the images it changes (VSTOR-153049,
> VSTOR-153050):
> 
> - a backward merge of an inner image pauses that image, while reads
>    enter through the top one. A read reaching the inner image as the
>    merge starts oopses on a NULL qio->ext (patch 2);
> 
> - SEEK_HOLE/SEEK_DATA walks the images with nothing keeping them
>    alive, so merge_backward complete frees an image under the walk
>    (patch 3).
> 
> Patch 1 (VSTOR-153051) is the counterpart of dm-ploop patch 4, an
> error path leaving state behind: after a failed refcount table
> relocation, allocating writes past the old refcount coverage fail with
> EIO until the device is suspended and resumed.
> 
> The series does not depend on the dm-ploop one.
> 
> Tested on dev-cl04, stock 6.12.0-211.52.1.22.11.vz10, dm-qcow2 built
> out of tree, with a test-only kprobe module widening the windows:
> - on the stock module, with 4K clusters and refcount_bits=64 so the
>    reftable relocates at 1G, a failed md page insert left every later
>    write past 1G failing with EIO; a seek went on with an image
>    qcow2_destroy() had freed; an intermediate merge oopsed in
>    submit_read_sliced_clu(). None of them reproduce with the series;
> - the ploop pytest suite, qcow2 format, passes.
> 
> Changes in v2, after Sashiko's review:
> - 1/3: a note in the commit message on why the error path does not
>    unlink the pages from the writeback lists;
> - 3/3: guard the walk with a per-target rw_semaphore, taken for write
>    by merge_backward complete, instead of an inflight ref held over the
>    walk. With the ref every inflight ref switch waited for the whole
>    scan, including the one starting a backward merge with the top image
>    paused, so guest I/O stalled for as long as the scan ran.
> The seek reproducer and the qcow2 pytest suite were rerun on v2: a
> merge now starts while a seek is held, and merge_backward complete
> waits for the seek to finish before destroying the image.
> 
> v1: http://lore.virtuozzo.com/kernel/20261103083708.131379-1-den@openvz.org/
> 
> Denis V. Lunev (3):
>    dm-qcow2: fix a failed refcount table relocation leaking md pages
>    dm-qcow2: fix backward merge of an inner image racing its reads
>    dm-qcow2: fix SEEK_HOLE/SEEK_DATA using an image freed by a merge
> 
>   drivers/md/dm-qcow2-cmd.c    | 28 ++++++++++++----------------
>   drivers/md/dm-qcow2-map.c    | 18 +++++++++++++-----
>   drivers/md/dm-qcow2-target.c |  1 +
>   drivers/md/dm-qcow2.h        |  2 ++
>   4 files changed, 28 insertions(+), 21 deletions(-)
> 
> 
> base-commit: 09eb9a82fe872048d082391415d1cdeda2ac6879

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v2 1/3] dm-qcow2: fix a failed refcount table relocation leaking md pages

[Konstantin Khorenko]

I won't apply this version, please send v2 with the changes below.

On 11/3/26 10:08, Denis V. Lunev wrote:
>  err_free_r2_pages:
> -	for (i = end; i < r2_end; i += clu_size) {
> +	for (i = new_md_start; i < new_md_end; i += PAGE_SIZE) {
>  		ret = handle_md_page(qcow2, i >> PAGE_SHIFT, NULL, &md);
>  		if (ret <= 0)
>  			break;

Item 1 of the cover letter reply: after a successful header write, do
not free the new md pages in this loop. Send the gotos that follow the
header write to a label after the loop that only returns -EIO, and
update the "Note for a future reader" to match.

This version should not go in as is: it trades one rare failure for a
worse one.

  +------------------------+----------------------+--------------------+
  |                        | without this patch   | with this patch    |
  +------------------------+----------------------+--------------------+
  | a page allocation or   | every write past the | fixed              |
  | the header write fails | old refcount         |                    |
  | during the relocation  | coverage gets EIO    |                    |
  |                        | until suspend/resume |                    |
  +------------------------+----------------------+--------------------+
  | an image with 4K       | WARN + EIO           | use-after-free: a  |
  | clusters and           |                      | page still on the  |
  | refcount_bits=64 grows |                      | writeback list is  |
  | to about 511G          |                      | freed, memory gets |
  | (refcount_bits=32: 2T) |                      | corrupted          |
  +------------------------+----------------------+--------------------+

EIO is visible and suspend/resume cures it. Memory corruption can crash
the node or silently damage someone else's data. Both cases are rare,
but the second one is normal work with a valid image, no failure needed.

How the second row happens: one of the gotos after the header write is
reachable on a valid image, and by then this loop frees a dirty page.
With 4K clusters and refcount_bits=64 the relocation at about 511G makes
the new reftable fill a whole refcount group, so that group gets no
refblock and the mark-used loop fails at i = pos (refcount_bits=32 gets
there at about 2T).

The connect loop has already dirtied the page at new_md_start and queued
it for writeback. Before this patch that page leaked and the device got
EIO. With the patch it is freed while still on the writeback list, and
the slow writeback timer then works on freed memory.

Item 6 of the cover letter reply, a separate patch: the missing refblock
for the group of pos is a bug of its own, there since the driver was
added. The connect loop gives a refblock only to the groups the new
refblocks sit in.

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v2 3/3] dm-qcow2: fix SEEK_HOLE/SEEK_DATA using an image freed by a merge

[Konstantin Khorenko]

I won't apply this version, please send v2 with the changes below.

On 11/3/26 10:08, Denis V. Lunev wrote:
> +	down_write(&tgt->seek_rwsem);
>  	qcow2 = *tgt->backward_merge.pqcow2;
>  	*tgt->backward_merge.pqcow2 = qcow2->lower;

Item 4 of the cover letter reply: say in the message that merge_backward
complete now waits for a running seek while it holds ctl_mutex. Guest
I/O is no longer held, but every target message and presuspend queue
behind ctl_mutex for as long as the scan runs. That is a fair price,
the message just has to say it.

> +	down_read(&tgt->seek_rwsem);
> +	qcow2 = tgt->top;

Item 7 of the cover letter reply, a separate patch: when an md page read
of a seek fails, the waiting seek qio is ended, but nothing completes
SEEK_QIO_DATA(qio)->compl, so the seek sleeps forever. That is older
than this patch, but now the seek sleeps holding seek_rwsem, so the next
merge_backward complete and every message after it hang under ctl_mutex
too.

With the message fixed:

Reviewed-by: Konstantin Khorenko <khorenko@virtuozzo.com>

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v2 2/3] dm-qcow2: fix backward merge of an inner image racing its reads

[Konstantin Khorenko]

I won't apply this version, please send v2 with the changes below.

On 11/3/26 10:08, Denis V. Lunev wrote:
> Pause submission and switch the inflight refs for every image. The
> pause flag is only tested on the top image, where all qios enter, so
> pause that one; clearing the flag of an inner image paused the inner
> image itself, which held nothing back.

Item 3 of the cover letter reply: say here that guest I/O to the top
image is now paused for the inflight ref switch when a merge of an
inner image starts or stops. That is fine, a merge of the top image
always cost the same, but the message should say it.

> +	 * A lower image has no writes, but it is read by
> +	 * qios of the top one, so stop the top image even then.

Item 2 of the cover letter reply: write "no guest writes". A writable
inner image is still written by qcow2_break_l1cow() and by the merge
itself.

With that fixed:

Reviewed-by: Konstantin Khorenko <khorenko@virtuozzo.com>

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v2 0/3] dm-qcow2: fix merges not stopping the users of the images they change

[Konstantin Khorenko]

I won't apply this version, please send v2 with the changes below.

On 11/3/26 10:08, Denis V. Lunev wrote:
> Denis V. Lunev (3):
>   dm-qcow2: fix a failed refcount table relocation leaking md pages
>   dm-qcow2: fix backward merge of an inner image racing its reads
>   dm-qcow2: fix SEEK_HOLE/SEEK_DATA using an image freed by a merge

What to change for v2:

1. Patch 1, code: after a successful header write, the error gotos
   must not free the new md pages. Send them to a label after the free
   loop that only returns -EIO, and update the "Note for a future
   reader" to match.
2. Patch 2, code comment: write "no guest writes" instead of "no
   writes".
3. Patch 2, commit message: say that guest I/O to the top image is now
   paused for the inflight ref switch when a merge of an inner image
   starts or stops.
4. Patch 3, commit message: say that merge_backward complete now waits
   for a running seek while it holds ctl_mutex.
5. Cover letter: fix the bullet on patch 2 quoted below.

Separate patches, not needed for v2:

6. relocate_refcount_table(): the refcount group of the new reftable
   gets no refblock when the table fills that whole group.
7. qcow2_llseek_hole(): a seek waits forever when one of its md page
   reads fails.

The details are in the replies to each patch.

> - a backward merge of an inner image pauses that image, while reads
>   enter through the top one. A read reaching the inner image as the
>   merge starts oopses on a NULL qio->ext (patch 2);

Item 5: before the series the start of an inner merge paused nothing,
only the stop paused the inner image. Say that the start paused nothing.

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel