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

[no-reply@sashiko.virtuozzo.com]

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@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.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261008170200.646208-1-alexander.ivanov@virtuozzo.com?part=3

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel