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