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 <[email protected]> >>>> --- >>>> 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? > Maybe point the user to that option. > > Max >
