Re: [Devel] [PATCH hci-8.1 3/3] drivers/md/dm-qcow2: do not revalidate inconsistent persistent bitmaps

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

>> @@ -865,6 +866,7 @@ static int qcow2_parse_header(struct dm_target *ti, struct qcow2 *qcow2,
>>   		max_len = upper->hdr.size;
>>   	}
>>   	ret = qcow2_check_convert_hdr(ti, raw_hdr, hdr, min_len, max_len);
>> +	raw_autoclear = be64_to_cpu(raw_hdr->autoclear_features);
>>   	kunmap(md->page);
>>   	if (ret < 0)
>>   		goto out;
> [Severity: Low]
> Is it correct to read autoclear_features from raw_hdr unconditionally here?
>
> qcow2_check_convert_hdr() returns early for version 2 images:
>
>      if (hdr->version == 2)
>          return 0;
>
> so it never parses or zeroes autoclear_features for them. For a v2 image,
> the field is not part of the header, and the bytes at that offset belong to
> the extension area or padding. The new code then treats those bytes as
> autoclear feature flags.
>
> The v3 case looks similar. header_length is only checked against
> sizeof(struct QCowHeader) on the compression path. A v3 image with a short
> header_length would also have the bytes at that offset read as autoclear
> flags.
>
> The new code can only clear QCOW2_AUTOCLEAR_BITMAPS, never set it, and the
> read stays inside the mapped page. The effect is therefore small.
>
> Would it be better to do the read and the mask only when hdr->version >= 3?
> That could go inside qcow2_check_convert_hdr(), next to the existing
> hdr->autoclear_features = 0.
No change needed: for v2 the value is never written back to disk, and a v3
header shorter than 104 bytes is invalid per the spec. The code can only
clear the bitmaps bit, never set it, so in the worst case bitmaps are 
treated
as inconsistent, which is safe.
>
> [ ... ]
>
>> @@ -914,6 +916,9 @@ static int qcow2_parse_header(struct dm_target *ti, struct qcow2 *qcow2,
>>   			goto out;
>>   		}
>>   	}
>> +	/* Bitmaps invalidated before must not become consistent again */
>> +	if (!(raw_autoclear & QCOW2_AUTOCLEAR_BITMAPS))
>> +		hdr->autoclear_features &= ~QCOW2_AUTOCLEAR_BITMAPS;
>>   	ret = 0;
>>   out:
>>   	return ret;
> [Severity: Low]
> For a v2 image, does this decision depend on a meaningless raw_autoclear
> value?
>
> The mask is applied based on whatever bytes sit at offset 88 of the header
> page, rather than on a real autoclear_features field. It should be
> conditional on hdr->version >= 3, as noted above.
For v2 the value is never used: hdr.autoclear_features is consumed only
by qcow2_set_image_file_features(), which returns early for v2, so whatever
bytes sit at offset 88 have no effect.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel