Re: [Qemu-block] [PATCH v2 7/7] block/qcow2-refcount: fix out-of-file L2 entries to be read-as-zero

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
>> ---
>>   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.

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.

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.

> 
>>               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?


> 
> Max
>