Re: [Devel] [PATCH hci-8.1 v7 6/6] 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-to-used-end trusts on-disk refcounts with no in-kernel guard by default
--

commit 7e932ded12fef03394ffccf5b1a73618dc7cd205
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() then truncates the image file to that point on postsuspend. The scan is skipped after md or L1/L2 writeback errors. It is also skipped for an image opened dirty, but only when kernel_sets_dirty_bit is set.

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);
> +		}

[Severity: Medium]
The truncation point returned here comes directly from the on-disk
refcounts. Can those refcounts be below real usage after a crash, since
L2 is written before refcounts?

If so, qcow2_calc_used_end() returns an end that is too small. The caller
then truncates the file below clusters that L2 still references. That
would be silent data loss, and it could happen in a session with no guest
writes.

> 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

[ ... ]

> +	if (tgt->md_writeback_error) {
> +		QC_INFO(ti, "Skip shrinking to used end after md writeback error");
> +	} else if (tgt->lx_writeback_error) {
> +		QC_INFO(ti, "Skip shrinking to used end after reverted L1/L2 writeback");
> +	} else if (kernel_sets_dirty_bit && qcow2->opened_dirty) {
> +		QC_INFO(ti, "Skip shrinking to used end of dirty image");
> +	} else {
> +		used_end = qcow2_calc_used_end(qcow2);

[Severity: Medium]
The dirty-image guard in qcow2_truncate_preallocations() only applies when
kernel_sets_dirty_bit is true, and that parameter defaults to false. With
the default, qcow2_parse_metadata() still records qcow2->opened_dirty, but
nothing consults it.

The open-time dirty bit check in qcow2_check_convert_hdr() is commented
out:

    //	if (!is_ro && kernel_sets_dirty_bit !=
    //	    !(hdr->incompatible_features & INCOMPATIBLE_FEATURES_DIRTY_BIT))
    //		return kernel_sets_dirty_bit ? -EUCLEAN : -ENOLCK;

So in the default configuration, can a crashed image that has not been
checked be opened RW? If it is, the postsuspend, remove or table swap path
would run qcow2_calc_used_end() and truncate the file.

The commit message says userspace checks the image before opening it RW.
Is that requirement documented or enforced anywhere in the repository?

find_unused_cluster() also trusts the refcounts. It only does so when a
guest write needs a cluster, though. This truncation runs earlier and can
cut off clusters that no write touches.

> +		if (used_end < 0)
> +			QC_ERR(ti, "Can't find used end: %pe", ERR_PTR(used_end));
> +		else
> +			end = used_end;
> +	}

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

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