[Devel] [PATCH hci-8.1 v4 3/3] dm-qcow2: truncate unused tail of image on suspend

[Alexander Ivanov]

On 10/8/26 19:08, no-reply at sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] dm-qcow2: truncate_preallocations() trusts unvalidated on-disk refcounts as file length
> --
>
> commit e3b5d03194a42ef95bbb361e199ba3c183aed97e
> Author: Alexander Ivanov <alexander.ivanov at virtuozzo.com>
>
> dm-qcow2: truncate unused tail of image on suspend
>
> This patch adds qcow2_calc_used_end(), which scans the refcount blocks backward and returns the end of the last cluster with a non-zero refcount. qcow2_truncate_preallocations() now truncates the image file to that value on postsuspend. If the scan fails, or md writeback failed, it falls back to the old behaviour.
>
> Link: https://virtuozzo.atlassian.net/browse/VSTOR-148171
>
>> 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
> [ ... ]
>
>> +loff_t qcow2_calc_used_end(struct qcow2 *qcow2)
>> +{
> [ ... ]
>
>> +		for (i = r2.index_in_page; i >= 0; i--, pos -= clu_size) {
>> +			/* Last cluster is partial if size isn't cluster-aligned */
>> +			if (get_r2_entry(qcow2, r2.md, i))
>> +				return min_t(loff_t, pos + clu_size,
>> +					     qcow2->file_preallocated_area_start);
>> +		}
>> +	}
>> +
>> +	return -EIO;
>> +}
> [Severity: Medium]
> The used end here comes only from the on-disk refcounts. Can it be wrong when those refcounts are stale or corrupt?
>
> An image written by QEMU with lazy refcounts, or one left dirty after a crash, can legitimately have zero refcounts for clusters that are still in use. A corrupt or crafted image can too.
>
> In that case qcow2_calc_used_end() returns an end below clusters that are still referenced. Nothing cross-checks the result against the L1/L2 tables, the snapshot extents, or the header and refcount table/refblock offsets.
>
> Before this patch the truncate went only down to file_preallocated_area_start. That value is set to file_size at load and is only raised by this driver's own preallocation, so the old code never cut off referenced data.
>
>> 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
> [ ... ]
>
>> +	/* Refcounts on disk are stale after failed md writeback */
>> +	if (!tgt->md_writeback_error) {
>> +		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;
>> +	}
>> +
>>   	if (end == qcow2->file_size)
>>   		return;
>>   
>>   	ret = qcow2_truncate_safe(qcow2->file, end);
> [Severity: Medium]
> In qcow2_truncate_preallocations(), the value from qcow2_calc_used_end() is passed straight to qcow2_truncate_safe() as the new file length. The fallback only covers scan errors (-EIO and similar). It does not cover refcounts that are consistent to parse but logically wrong.
>
> The md_writeback_error check only covers this driver's own writeback failures. It does not cover refcounts that were already stale when the image was loaded.
>
> Is there a way to validate the result before truncating? For example, the shrink could be skipped when the dirty or lazy-refcount flag is set, or the used end could be checked against the L1/L2 and metadata extents.
>
> This ties into qcow2_check_convert_hdr(), where the dirty-bit check is commented out:
>
> //	if (!is_ro && kernel_sets_dirty_bit !=
> //	    !(hdr->incompatible_features & INCOMPATIBLE_FEATURES_DIRTY_BIT))
> //		return kernel_sets_dirty_bit ? -EUCLEAN : -ENOLCK;
>
> With that check disabled, images with stale refcounts are accepted at load time. I also don't see any other refcount consistency validation at load.
Before we return the image to the kernel we do "qemu-img check -r" and 
the refcounts must be consistent.