On 09.10.18 00:14, Vladimir Sementsov-Ogievskiy wrote:
>
>
> On 10/09/2018 01:08 AM, Max Reitz wrote:
>> On 09.10.18 00:02, Vladimir Sementsov-Ogievskiy wrote:
>>>
>>>
>>> On 10/08/2018 11:51 PM, Max Reitz wrote:
>>>> On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Rewrite corrupted L2 table entry, which reference space out of
>>>>> underlying file.
>>>>>
>>>>> Make this L2 table entry read-as-all-zeros without any allocation.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
>>>>> ---
>>>>> block/qcow2-refcount.c | 32 ++++++++++++++++++++++++++++++++
>>>>> 1 file changed, 32 insertions(+)
>>>>>
>>>>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>>>>> index 3c004e5bfe..3de3768a3c 100644
>>>>> --- a/block/qcow2-refcount.c
>>>>> +++ b/block/qcow2-refcount.c
>>>>> @@ -1720,8 +1720,30 @@ static int check_refcounts_l2(BlockDriverState
>>>>> *bs, BdrvCheckResult *res,
>>>>> /* Mark cluster as used */
>>>>> csize = (((l2_entry >> s->csize_shift) & s->csize_mask) +
>>>>> 1) *
>>>>> BDRV_SECTOR_SIZE;
>>>>> + if (csize > s->cluster_size) {
>>>>> + ret = fix_l2_entry_to_zero(
>>>>> + bs, res, fix, l2_offset, i, active,
>>>>> + "compressed cluster larger than cluster: size
>>>>> 0x%"
>>>>> + PRIx64, csize);
>>>>> + if (ret < 0) {
>>>>> + goto fail;
>>>>> + }
>>>>> + continue;
>>>>> + }
>>>>> +
>>>>
>>>> This seems recoverable, isn't it? Can we not try to just limit the
>>>> csize, or decompress the cluster with the given csize from the given
>>>> offset, disregarding the cluster limit?
>>>
>>> Hm, you want to assume that csize is corrupted but coffset may be
>>> correct? Unlikely, I think.
>>
>> Better to reconstruct probably garbage data than to definitely garbage
>> data (all zeroes) is what I think.
>>
>>> So, to carefully repair csize, we should decompress one cluster (or one
>>> cluster - 1 byte) of data, trying to get one cluster of decompressed
>>> data. If we succeed, we know csize, or we can safely set it to one cluster.
>>
>> Yes.
>>
>>> Or we can just set csize = 1 cluster, if it is larger. And leave
>>> problems to real execution which will lead to EIO in worst case.
>>
>> Or this, yes.
>>
>>>>> coffset = l2_entry & s->cluster_offset_mask &
>>>>> ~(BDRV_SECTOR_SIZE - 1);
>>>>> + if (coffset >= bdrv_getlength(bs->file->bs)) {
>>>>> + ret = fix_l2_entry_to_zero(
>>>>> + bs, res, fix, l2_offset, i, active,
>>>>> + "compressed cluster out of file: offset 0x%"
>>>>> PRIx64,
>>>>> + coffset);
>>>>> + if (ret < 0) {
>>>>> + goto fail;
>>>>> + }
>>>>> + continue;
>>>>> + }
>>>>> +
>>>>> ret = qcow2_inc_refcounts_imrt(bs, res,
>>>>> refcount_table,
>>>>> refcount_table_size,
>>>>> coffset, csize);
>>>>> @@ -1748,6 +1770,16 @@ static int check_refcounts_l2(BlockDriverState
>>>>> *bs, BdrvCheckResult *res,
>>>>> {
>>>>> uint64_t offset = l2_entry & L2E_OFFSET_MASK;
>>>>>
>>>>> + if (offset >= bdrv_getlength(bs->file->bs)) {
>>>>> + ret = fix_l2_entry_to_zero(
>>>>> + bs, res, fix, l2_offset, i, active,
>>>>> + "cluster out of file: offset 0x%" PRIx64,
>>>>> offset);
>>>>> + if (ret < 0) {
>>>>> + goto fail;
>>>>> + }
>>>>> + continue;
>>>>> + }
>>>>> +
>>>>
>>>> These other two look OK, but they have another issue: If this is a v2
>>>> image, you cannot create zero clusters; so you'll have to unallocate the
>>>> cluster in that case.
>>>
>>>
>>> Oho, it's a problem. It may be unsafe to discard clusters, making
>>> backing image available through the holes. What discard do on v2?
>>> Zeroing or holes?
>>
>> Oh, right! discard on v2 punches a hole. So I see three ways:
>> (1) You can do the same and point to that bit of code, or
>> (2) You allocate a data cluster full of zeroes in case of v2, or
>> (3) You just error out.
>>
>> (3) doesn't seem like the worst option.
>
>> Amending the image to be v3 is
>> always possible and trivial.
>
> how to do it for corrupted image?Oh, yeah, you can't open a corrupted image, can you... I suppose we want a way to force-clear the flag anyway. :-) Max
signature.asc
Description: OpenPGP digital signature
