[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v2 2/7] block/qcow2-refcount: avoid eating RAM
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [Qemu-block] [PATCH v2 2/7] block/qcow2-refcount: avoid eating RAM |
Date: |
Mon, 8 Oct 2018 20:17:43 +0000 |
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 <address@hidden>
>> ---
>> 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..
>
>> +
>> + 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
>
> Max
>
>> + res->corruptions++;
>> + return 0;
>> + }
>> +
>> start = start_of_cluster(s, offset);
>> last = start_of_cluster(s, offset + size - 1);
>> for(cluster_offset = start; cluster_offset <= last;
>>
>
>