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

["Denis V. Lunev" <den@virtuozzo.com>]

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