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

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] dm-qcow2: used-end shrink trusts refcounts without any in-kernel crash guard
- [Medium] dm-qcow2: unvalidated on-disk refcounts steer destructive truncation in qcow2_calc_used_end()
- [Medium] dm-qcow2: GFP_KERNEL md page allocs in postsuspend scan outside NOIO scope
--

commit e35086f1c84ad69710c30b217b7a28fd6da3991c
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.
On postsuspend, qcow2_truncate_unused_tail() now truncates the image file to
that used end instead of only dropping the preallocated area. The scan is
skipped after an md writeback error, and for a dirty image when
kernel_sets_dirty_bit is set.

> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
> index 27ae22495f37..d96532b3753e 100644
> --- a/drivers/md/dm-qcow2-map.c
> +++ b/drivers/md/dm-qcow2-map.c

[ ... ]

> +/*
> + * Return the end of the last cluster with non-zero refcount. Must be
> + * called on a suspended device after md writeback.
> + */
> +loff_t qcow2_calc_used_end(struct qcow2 *qcow2)
> +{

[ ... ]

> +		if (!r1_entry_is_valid(qcow2, get_u64_from_be_page(r1_md->page,
> +							r1.index_in_page)))
> +			return -EIO;

[Severity: Medium]
r1_entry_is_valid() only checks the reserved bits and the cluster alignment
of the R1 entry. Is the refblock offset checked anywhere against the file
size or against overlap with other metadata?

The open path does not check it either. qcow2_parse_header() and
qcow2_check_convert_hdr() do not validate R1 entries or refblock offsets.
The only reftable check there is:

    if (qcow2->reftable_max_file_size < qcow2->file_size)
            goto out;

If an R1 entry is zero or points at the wrong block, can the scan skip or
miss clusters that L2 still maps? The file would then be truncated below
live guest data.

A refblock beyond EOF fails the read, and the scan returns -EIO and falls
back to the old behaviour.

> +
> +		ret = calc_r2_page_id(qcow2, &r1, &r2);

[ ... ]

> +		for (i = r2.index_in_page; i >= 0; i--, pos -= clu_size) {
> +			if (get_r2_entry(qcow2, r2.md, i))
> +				return min_t(loff_t, pos + clu_size,
> +					     qcow2->file_preallocated_area_start);
> +		}

[Severity: Medium]
The used end comes only from the on-disk refcounts. There is no cross-check
against L1/L2 offsets. If a cluster is mapped by L2 but has refcount 0, for
example after a crash, doesn't this loop cut it off?

> +	}
> +
> +	return -EIO;
> +}

[ ... ]

> diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
> index 1b7a75239291..7587e8f7ac0b 100644
> --- 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 (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);
> +		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]
The only in-kernel guard before this truncate is the opened_dirty check. It
applies only when kernel_sets_dirty_bit is set, which defaults to false. A
v2 image has no dirty bit, because qcow2_set_image_file_features() returns
early for version 2, so the guard can never apply to it.

The commit message says "the scan is safe only because userspace checks the
image before an RW open". Is that precondition enforced anywhere in the
kernel?

The dirty-bit consistency 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;

qcow2_wants_check() reflects only md_writeback_error and truncate_error from
the current session.

Suppose a crash leaves a refcount at 0 for a cluster that L2 already
references, and the image is opened RW without a userspace check. Will
qcow2_truncate_safe() then permanently cut off mapped clusters?

The new end is stored into file_preallocated_area_start and file_size, so
the loss cannot be recovered. The old code only dropped the preallocated
area, so it could not remove mapped data.

Could the kernel refuse the scan for images whose consistency it cannot
verify, or could the commit message explain how the userspace check is
enforced?

[ ... ]

> @@ -1157,7 +1170,7 @@ static void qcow2_postsuspend(struct dm_target *ti)
>  	int ret;
>  
>  	qcow2_flush_deferred_activity_all(to_qcow2_target(ti));
> -	qcow2_truncate_preallocations(ti);
> +	qcow2_truncate_unused_tail(ti);

[Severity: Medium]
qcow2_truncate_unused_tail() now reaches qcow2_calc_used_end(), which loops
over qcow2_md_page_read_sync(). That calls qcow2_alloc_and_insert_md_page(),
which uses kmalloc(GFP_KERNEL) and alloc_page(GFP_KERNEL). The backing-file
read can also allocate memory.

The call chain is:

dm_suspend()->dm_table_postsuspend_targets()->qcow2_postsuspend()
  ->qcow2_truncate_unused_tail()->qcow2_calc_used_end()
    ->qcow2_md_page_read_sync()->qcow2_alloc_and_insert_md_page()

dm_suspend() does not wrap the postsuspend hooks in memalloc_noio_save().
Can direct reclaim triggered by these allocations wait on I/O to this same
suspended dm device, for example if swap is on it? That I/O could not
complete until resume, and resume is waiting for postsuspend.

The old postsuspend path allocated no md pages, so this exposure is new. I
did not find an in-tree user that swaps onto a qcow2 device, so this may be
theoretical.

Would wrapping qcow2_calc_used_end() or qcow2_postsuspend() in
memalloc_noio_save()/memalloc_noio_restore(), or using GFP_NOIO in
qcow2_alloc_and_insert_md_page(), be appropriate?

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

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