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

[Devel] [PATCH hci-8.1 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 without an inflight ref, 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.

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 | 26 ++++++++++----------------
 drivers/md/dm-qcow2-map.c | 19 ++++++++++++++-----
 2 files changed, 24 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 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.

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

Re: [Devel] [PATCH hci-8.1 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: relocate_refcount_table() error path frees md pages still queued on wb lists
--

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

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

This commit changes the error path of relocate_refcount_table() so that it
frees exactly the md pages the allocation loop inserted. Previously it freed
only one page per cluster starting from end. The new range is
[new_md_start, new_md_end) in PAGE_SIZE steps.

> 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

[ ... ]

> @@ -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 frees every page in the inserted range. That range includes
the last new-R1 cluster pages and all pages of multi-page refblock
clusters. The loop calls qcow2_md_page_erase() and qcow2_free_md_page(),
but it never does list_del(&md->wb_link) and never clears MD_DIRTY.

qcow2_md_page_erase() only does rb_erase(). qcow2_free_md_page() only does
WARN_ON_ONCE(md->wbd || md->lockd), put_page() and kfree().

Pages in this range can be dirtied before the error label is reached.
mark_cluster_used() calls qcow2_md_make_dirty() on the new R2 pages:

mark_cluster_used()
    set_r2_entry(qcow2, r2_md, r2_index_in_page, 1);
    ...
    qcow2_md_make_dirty(qcow2, r2_md, true);

qcow2_md_make_dirty() then links the page onto the writeback list:

    if (!(md->status & MD_DIRTY)) {
        md->status |= MD_DIRTY;
        list_add_tail(&md->wb_link, head);

Consider a failure in the mark-unused loop after the mark-used loop has
run:

relocate_refcount_table()
    mark_cluster_used()            /* new R2 pages dirty and queued */
    qcow2_handle_r1r2_maps() ...   /* WARN_ON_ONCE(ret <= 0) */
    goto err_free_r2_pages

The same applies to the other late gotos after the header write: the
renumber failure, the calc_refcounters_map() < 0 case, and the
WARN_ON_ONCE(ret <= 0) checks in the connect and mark-used loops.

The freed md pages would then remain linked on slow_wb_batch_list or
wb_batch_list. When the writeback worker runs, it would walk the stale
entries:

submit_metadata_writeback()
    md = list_first_entry_or_null(&qcow2->wb_batch_list,
                                  struct md_page, wb_link);
    ...
    list_del_init(&md->wb_link);

Could that be a use-after-free or list corruption?

I could not construct a concrete trigger, since these paths are guarded by
WARN_ON_ONCE. The allocation-failure and header-write-failure cases named
in the commit message are fine, because no page in the range is dirty yet.

The cached hdr.refcount_table_offset and hdr.refcount_table_clusters are
also not rolled back at this label, and the on-disk header has already been
rewritten. The existing "/* TODO: switch to RO */" comment covers this.

Should the cleanup unlink wb_link and clear MD_DIRTY for each page before
freeing it?

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

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

[Devel] [PATCH hci-8.1 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

Re: [Devel] [PATCH hci-8.1 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/20261103083708.131379-1-den@openvz.org?part=2


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

[Devel] [PATCH hci-8.1 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, without an inflight ref. 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 an inflight ref for the whole walk and take the top image from
qcow2_ref_inc() under it.

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-map.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index 8ab7cb472560..358a71f75cb4 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -4841,18 +4841,25 @@ 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;
+	u8 ref_index;
 	int ret;
 
+	/* Keep merge_backward complete from freeing images under the walk */
+	qcow2 = qcow2_ref_inc(tgt, &ref_index);
 	qio = alloc_seek_qio(qcow2, NULL, qcow2->hdr.size);
-	if (!qio)
+	if (!qio) {
+		qcow2_ref_dec(tgt, ref_index);
 		return -ENOMEM;
+	}
 
 	seek_qio_set_sector(qio, to_sector(offset));
 
 	ret = qcow2_llseek_hole_qio(qio, whence, &result);
+	qcow2_ref_dec(tgt, ref_index);
 	/* In case of error remap ENXIO as it have special meaning for llseek */
 	if (ret < 0)
 		return (ret == -ENXIO) ? -EINVAL : ret;
-- 
2.53.0

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

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

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dm-qcow2: qcow2_llseek_hole() holds inflight ref for whole walk, stalling ref switch
--

commit 042e70787bbad4a7f53038a208c72c6e24e2bfb5
Author: Denis V. Lunev <den@openvz.org>

dm-qcow2: fix SEEK_HOLE/SEEK_DATA using an image freed by a merge

This patch makes qcow2_llseek_hole() take an inflight reference with
qcow2_ref_inc() before it starts walking the image chain. It drops the
reference with qcow2_ref_dec() after the walk finishes. This is meant to
stop merge_backward complete from freeing an image while the walk is still
using it.

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

> 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

[ ... ]

> @@ -4841,18 +4841,25 @@ 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;
> +	u8 ref_index;
>  	int ret;
>  
> +	/* Keep merge_backward complete from freeing images under the walk */
> +	qcow2 = qcow2_ref_inc(tgt, &ref_index);
>  	qio = alloc_seek_qio(qcow2, NULL, qcow2->hdr.size);
> -	if (!qio)
> +	if (!qio) {
> +		qcow2_ref_dec(tgt, ref_index);
>  		return -ENOMEM;
> +	}
>  
>  	seek_qio_set_sector(qio, to_sector(offset));
>  
>  	ret = qcow2_llseek_hole_qio(qio, whence, &result);
> +	qcow2_ref_dec(tgt, ref_index);

[Severity: Medium]
This isn't a bug in the use-after-free fix itself, but the inflight ref
is now held across the whole qcow2_llseek_hole_qio() walk.  That walk is
unbounded for a large sparse image and sleeps in wait_for_completion()
while metadata pages are read.

Several callers of qcow2_inflight_ref_switch() have to drain that ref
first, so each of them now waits for the whole scan:

qcow2_merge_backward_complete()
  qcow2_inflight_ref_switch()
    wait_for_completion(comp)

This runs under ctl_mutex, so other control messages queue behind it.

set_backward_merge_in_process()
  top->pause_submitting_qios = true;
  qcow2_inflight_ref_switch()

While this waits, qcow2_submit_embedded_qio() parks new requests on
paused_qios. Can guest I/O to the target stall until the seek finishes?

qcow2_flush_deferred_activity()
  qcow2_inflight_ref_switch()

This is reached from suspend and teardown. dm_llseek_hole() checks
dm_suspended_md() only before the walk, so a suspend that starts
mid-walk would wait for the whole scan.

I did not find a deadlock. The seek's metadata reads call qcow2_ref_inc()
after the index flip and get the other slot, and the worker does not wait
on the ref. So the stall is bounded by the length of the walk.

Could the commit message mention this cost? Would it be reasonable to
drop and retake the ref per chunk and revalidate the top image, so
the ref is not held for the whole scan?

>  	/* In case of error remap ENXIO as it have special meaning for llseek */
>  	if (ret < 0)
>  		return (ret == -ENXIO) ? -EINVAL : ret;

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

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