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

[Vladimir Sementsov-Ogievskiy]

10.10.2018 19:55, Vladimir Sementsov-Ogievskiy wrote:
> 10.10.2018 19:39, Vladimir Sementsov-Ogievskiy wrote:
>> 17.08.2018 15: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;
>>> +            }
>>> +
>>>               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;
>>> +            }
>>> +
>>>               if (flags & CHECK_FRAG_INFO) {
>>>                   res->bfi.allocated_clusters++;
>>>                   if (next_contiguous_offset &&
>>
>> hmm, interesting question here: in case of misaligned l2 entry, we 
>> zero it out only for QCOW2_CLUSTER_ZERO_ALLOC, but not for normal 
>> clusters? Why? I think it is ok to mark as zero misaligned normal 
>> cluster l2 entry, otherwise we'll have fatal corruption on any 
>> operation to this cluster.
>>
>
> or we can just align them down.
>

and why do we calculate refcounts for corrupted l2 entry? Is it correct, 
to consider data range referenced by this entry, if we'll never success 
in writing or reading this data?

-- 
Best regards,
Vladimir