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

[no-reply@sashiko.virtuozzo.com]

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