<!DOCTYPE html><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<blockquote type="cite" cite="mid:010701a16396969c-15e1ad1c-bcda-4be0-9724-879749c1dd00-000000@eu-central-1.amazonses.com">
<div class="moz-text-plain" wrap="true" graphical-quote="true" style="font-family: -moz-fixed; font-size: 12px;" lang="x-unicode">
<blockquote type="cite" style="">
<pre wrap="" class="moz-quote-pre">@@ -1363,6 +1367,8 @@ static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
if (cleared)
continue; /* no cluster was allocated */
+ /* L1/L2 on disk may point to the cluster */
+ wbd->md->status |= MD_LX_REVERTED;
spin_unlock(&qcow2->md_pages_lock);
pos &= ~LX_REFCOUNT_EXACTLY_ONE;
/*
* R1/R2 should be cached, since we was able
* to submit cluster allocation.
*/
ret = qcow2_handle_r1r2_maps(qcow2, pos, NULL, &r1, &r2, false);
if (WARN_ON_ONCE(ret <= 0))
continue;
mark_cluster_unused(qcow2, r2.md, r2.index_in_page, pos);
spin_lock(&qcow2->md_pages_lock);
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">[Severity: Medium]
Can the continue taken on WARN_ON_ONCE(ret <= 0) leave this loop running
without md_pages_lock?
The lock is dropped just above with spin_unlock(), and it is only retaken
by the spin_lock() at the bottom of the loop body, which the continue
skips. qcow2_handle_r1r2_maps() can return a value <= 0 in several cases:
qcow2_handle_r1r2_maps()
__handle_r1r2_maps()
calc_refcounters_map() fails with -EIO
calc_r2_page_id() returns -EIO or -ENOENT
__handle_md_page() returns 0 when the page is not cached and
qio is NULL
"refblock=%llu, while no snapshots" returns -EIO
After the continue, the next iteration calls get_u64_from_be_page() and
set_u64_to_be_page() on wbd->md->page without the lock. The caller
complete_wbd() then does:
spin_lock_irqsave(&qcow2->md_pages_lock, flags);
revert_l_entries_update(qcow2, wbd);
clear_writeback_status(qcow2, wbd->md, wbd->ret, &wait_list, &end_list);
spin_unlock_irqrestore(&qcow2->md_pages_lock, flags);
So clear_writeback_status() also runs unlocked, and the final unlock is
unbalanced. The comment says R1/R2 should be cached, but nothing enforces
that. This patch adds the MD_LX_REVERTED assignment in this same
unlock window and relies on this error path. Should the lock be retaken
before the continue?
</pre>
<blockquote type="cite" style="">
<pre wrap="" class="moz-quote-pre">@@ -2387,21 +2463,21 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">[ ... ]
</pre>
<blockquote type="cite" style="">
<pre wrap="" class="moz-quote-pre">@@ -2412,7 +2488,7 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
for (i = pos; i < r2_end; i += clu_size) {
ret = qcow2_handle_r1r2_maps(qcow2, i, NULL, &r1, &r2, false);
if (WARN_ON_ONCE(ret <= 0))
- goto err_free_r2_pages;
+ goto err_hdr_written;
mark_cluster_used(qcow2, r2.md, r2.index_in_page);
}
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">[ ... ]
</pre>
<blockquote type="cite" style="">
<pre wrap="" class="moz-quote-pre">+err_hdr_written:
+ /* Header on disk points to new R1, refcounts may be inconsistent */
+ QC_ERR(qcow2->tgt->ti, "failed to relocate refcount table");
+ qcow2->tgt->md_writeback_error = true;
err_free_r2_pages:
for (i = end; i < r2_end; i += clu_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);
}
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">[Severity: Medium]
Can this cleanup free R2 md pages that are still linked on
slow_wb_batch_list?
Earlier in the function, mark_cluster_used() calls
qcow2_md_make_dirty(qcow2, r2_md, true). That sets MD_DIRTY, does
list_add_tail(&md->wb_link, &qcow2->slow_wb_batch_list), and may arm
slow_wb_timer. The R2 pages for the clusters in [end, r2_end) are the
ones the cleanup loop frees.
If a later WARN_ON_ONCE(ret <= 0) check fails, the code reaches
err_hdr_written and falls into err_free_r2_pages. There,
qcow2_md_page_erase() only does rb_erase(&md->node, &qcow2->md_pages).
qcow2_free_md_page() then frees the page. Neither one does
list_del(&md->wb_link) or clears MD_DIRTY.
The freed md_page stays on slow_wb_batch_list. The timer callback or the
worker can later splice that list and read md->status and md->wbd of the
freed md. Could the dirty pages be unlinked from the writeback list
before they are freed here? This patch routes more failure paths into
this shared cleanup.
</pre>
</div>
</blockquote>
1, 2: pre-existing, reachable only via WARN paths; this patch
neither adds<br>
new jumps into err_free_r2_pages nor sets MD_LX_REVERTED outside the<br>
lock. Will be fixed separately.
<blockquote type="cite" cite="mid:010701a16396969c-15e1ad1c-bcda-4be0-9724-879749c1dd00-000000@eu-central-1.amazonses.com">
<div class="moz-text-plain" wrap="true" graphical-quote="true" style="font-family: -moz-fixed; font-size: 12px;" lang="x-unicode">
<blockquote type="cite" style="">
<pre wrap="" class="moz-quote-pre">diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
--- a/drivers/md/dm-qcow2-target.c
+++ b/drivers/md/dm-qcow2-target.c
@@ -170,6 +170,7 @@ static void qcow2_free_md_pages_tree(struct rb_root *root)
void qcow2_flush_deferred_activity(struct qcow2_target *tgt, struct qcow2 *qcow2)
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">[ ... ]
</pre>
<blockquote type="cite" style="">
<pre wrap="" class="moz-quote-pre">@@ -191,13 +192,17 @@ void qcow2_flush_deferred_activity(struct qcow2_target *tgt, struct qcow2 *qcow2
node; node = rb_next(node)) {
md = rb_entry(node, struct md_page, node);
/* FIXME: call md_make_dirty() and try once again? */
- if (md->status & MD_WRITEBACK_ERROR) {
- QC_ERR(tgt->ti, "Failed to write dirty pages");
- tgt->md_writeback_error = true;
- break;
- }
+ if (md->status & MD_WRITEBACK_ERROR)
+ wb_error = true;
+ if (md->status & MD_LX_REVERTED)
+ tgt->lx_writeback_error = true;
}
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">[Severity: Medium]
Can this set the sticky tgt->lx_writeback_error from an image that is
about to be destroyed?
qcow2_flush_deferred_activity() is also called from a non-suspend path:
qcow2_merge_backward_complete()
qcow2_flush_deferred_activity(tgt, qcow2); /* Delayed md pages */
qcow2->lower = NULL;
...
qcow2_destroy(qcow2);
If a page of the image being removed from the chain is still marked
MD_LX_REVERTED, the target-wide flag is set. The image and its md pages
are then freed, but the flag stays set. Nothing clears it.
qcow2_wants_check() now includes the flag, so qcow2_preresume() fails
with:
if (qcow2_wants_check(tgt)) {
QC_ERR(ti, "image check and target reload are required");
return -EIO;
}
The surviving image is then refused resume, and
qcow2_truncate_preallocations() skips the shrink, even though the stale
page no longer exists. The existing md_writeback_error has the same
imprecision on this path, but is that intended for the new flag as well?
</pre>
</div>
</blockquote>
Intentional, matches md_writeback_error on the same path; it only
fires<br>
after a failed L1/L2 writeback during the merge.
<blockquote type="cite" cite="mid:010701a16396969c-15e1ad1c-bcda-4be0-9724-879749c1dd00-000000@eu-central-1.amazonses.com">
<div class="moz-text-plain" wrap="true" graphical-quote="true" style="font-family: -moz-fixed; font-size: 12px;" lang="x-unicode">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite" style="">
<pre wrap="" class="moz-quote-pre">@@ -1048,11 +1053,34 @@ static void qcow2_truncate_preallocations(struct dm_target *ti)
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">[ ... ]
</pre>
<blockquote type="cite" style="">
<pre wrap="" class="moz-quote-pre">+ if (tgt->md_writeback_error) {
+ QC_INFO(ti, "Skip shrinking to used end after md writeback error");
+ } else if (tgt->lx_writeback_error) {
+ QC_INFO(ti, "Skip shrinking to used end after reverted L1/L2 writeback");
+ } else if (kernel_sets_dirty_bit &&
+ (qcow2->hdr.incompatible_features &
+ INCOMPATIBLE_FEATURES_DIRTY_BIT)) {
+ QC_INFO(ti, "Skip shrinking to used end of dirty image");
+ } else {
+ used_end = qcow2_calc_used_end(qcow2);
+ if (used_end < 0)
+ QC_ERR(ti, "Can't find used end: %pe", ERR_PTR(used_end));
+ else
+ end = used_end;
+ }
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">[Severity: Medium]
Is this guard enough to protect against truncating below clusters that
L1/L2 still reference after a crash?
The only protection against stale refcounts is:
kernel_sets_dirty_bit && (qcow2->hdr.incompatible_features &
INCOMPATIBLE_FEATURES_DIRTY_BIT)
kernel_sets_dirty_bit is a static bool that defaults to false, so this
branch is inactive by default. In qcow2_check_convert_hdr(), v2 images
return early:
if (hdr->version == 2)
return 0;
hdr->incompatible_features = be64_to_cpu(raw_hdr->incompatible_features);
That return comes before incompatible_features is read, so the field
stays 0 and the guard can never fire for v2 images. The open-time
dirty-bit check in qcow2_check_convert_hdr() is commented out:
// if (!is_ro && kernel_sets_dirty_bit !=
// !(hdr->incompatible_features & INCOMPATIBLE_FEATURES_DIRTY_BIT))
// return kernel_sets_dirty_bit ? -EUCLEAN : -ENOLCK;
So the kernel does not enforce that the image was checked before an RW
open.
If refcounts are below real usage after a crash, qcow2_calc_used_end()
returns an end below clusters that L2 still points to. It is computed
purely from on-disk R2 entries. qcow2_truncate_safe() then cuts those
clusters off, even on a plain suspend, remove or table swap with no guest
writes in the session.
The commit message says find_unused_cluster() already relies on the same
userspace contract. That path is only reached once a guest write needs a
new cluster, while this truncation runs on every RW suspend, remove and
reload.
Should the shrink be skipped or enforced in-kernel for v2 images and for
the default kernel_sets_dirty_bit setup?
<div class="moz-txt-sig">
</div></pre>
</div>
</blockquote>
Covered in the commit message: userspace checks the image before<br>
an RW open, including v2 images; find_unused_cluster() relies on the<br>
same contract.
</body>
</html>