Re: [Qemu-devel] [PATCH v2 2/7] block/qcow2-refcount: avoid eating RAM

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3044 bytes --]

On 08.10.18 22:22, Vladimir Sementsov-Ogievskiy wrote:
> 
> 
> On 10/08/2018 06:31 PM, Max Reitz wrote:
>> On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote:
>>> qcow2_inc_refcounts_imrt() (through realloc_refcount_array()) can eat
>>> an unpredictable amount of memory on corrupted table entries, which are
>>> referencing regions far beyond the end of file.
>>>
>>> Prevent this, by skipping such regions from further processing.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/qcow2-refcount.c | 14 ++++++++++++++
>>>   1 file changed, 14 insertions(+)
>>>
>>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>>> index 615847eb09..566c19fbfa 100644
>>> --- a/block/qcow2-refcount.c
>>> +++ b/block/qcow2-refcount.c
>>> @@ -1499,12 +1499,26 @@ int qcow2_inc_refcounts_imrt(BlockDriverState *bs, BdrvCheckResult *res,
>>>   {
>>>       BDRVQcow2State *s = bs->opaque;
>>>       uint64_t start, last, cluster_offset, k, refcount;
>>> +    int64_t file_len;
>>>       int ret;
>>>   
>>>       if (size <= 0) {
>>>           return 0;
>>>       }
>>>   
>>> +    file_len = bdrv_getlength(bs->file->bs);
>>> +    if (file_len < 0) {
>>> +        return file_len;
>>> +    }
>>
>> Doesn't this slow things down?  Can we not cache the length somewhere
>> and update it whenever the image is modified?
> 
> 
> hmm. bdrv_getlength is used everywhere in Qemu, and I don't think it is 
> good idea to improve it locally for these series. If we can improve it 
> somehow with a cache or something like this, it should be done for all 
> users and therefore it is outside of these series..

I wanted to write: Sure it's used everywhere, but usually that is before
someone performs some I/O, so it isn't too bad.  But this is a function
that's suppose to just increment a couple of values in memory, which is
different.

However, I put the "wanted to write" prefix there, because: I knew that
we already have a central cache for bdrv_getlength(), but it isn't used
when the block driver reports has_variable_length as true.  I thought
file-posix did that.  But it only does so for CD-ROM devices.

So I think it should be OK to call the function here, yes.

>>> +
>>> +    if (offset + size - file_len > s->cluster_size) {
>>> +        fprintf(stderr, "ERROR: counting reference for region exceeding the "
>>> +                "end of the file by more than one cluster: offset 0x%" PRIx64
>>> +                " size 0x%" PRIx64 "\n", offset, size);
>>
>> Why is one cluster OK?  Is there a specific case you're trying to catch
>> here?
> 
> raw file under qcow2 may be not aligned in real size to qcow2 cluster, 
> as I understand, it's normal for the last cluster to be semi-allocated

Ah, that's true, thanks.  I'd appreciate a comment here, though, and in
that case I think we don't need to check whether the reference is off by
more than a cluster, but whether it's off by a cluster or more (so >=).

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]