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

[Alexander Ivanov <alexander.ivanov@virtuozzo.com>]

>> +/*
>> + * 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.
The scan trusts refcounts, as the rest of the driver does: normal I/O does
not check R1 entries against other metadata either. Checking R1 offsets
would not help much. A zeroed R1 entry looks the same as a range with
no refblock, and a valid R1 entry can still point at a refblock full of 
zeros.
Only a walk over all L2 tables would catch these. Refcounts that disagree
with L2 are left to the image check before an RW open, the same check
that covers refcounts left stale by a crash. I added this to the commit 
message.
>
>> +
>> +		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?
The driver already trusts on-disk refcounts: find_unused_cluster() hands out
any refcount-0 cluster, so an image whose refcounts are below real usage is
corrupted by the first allocating write regardless of this patch. The 
only new
exposure is a session with no guest writes; that case is covered by the
opened_dirty guard with kernel_sets_dirty_bit, and by the userspace check
before RW open otherwise, as the commit message describes. Cross-checking
against L2 would mean walking every L2 table on each suspend and still
would not replace an image check.
>
>> +	}
>> +
>> +	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?
>
The precondition is not enforced in the kernel, and the kernel cannot verify
it cheaply: in the default mode userspace sets the dirty bit on every RW
open, so v2 and v3 are equally indistinguishable, and the only reliable
check is a walk over all L2 tables on every suspend. It is not a new
precondition either. find_unused_cluster() reuses any cluster with zero
refcount and prepare_unused_cluster() punches a hole in it, so the scenario
you describe already destroys mapped data on the next allocation. The
scan widens the damage of a violated invariant, it doesn't introduce one.
>> @@ -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?
Not new: qcow2_preresume() already allocates md pages with GFP_KERNEL
via qcow2_parse_metadata() on the same suspended device, and postsuspend
already did fs I/O (truncate, header write).
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel