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

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

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

This case is covered in the commit message (third paragraph).


>> +	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;
>> +	}
The guard is intentionally limited to kernel_sets_dirty_bit: in the
default mode userspace sets the dirty bit on every RW open, so
opened_dirty carries no information about consistency there.
The open-time check has been commented out since the driver
was introduced; this patch doesn't change that.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel