Re: [Qemu-devel] [PATCH v2 2/7] block/qcow2-refcount: avoid eating RAM
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
>>> ---
>>> 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
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 2/7] block/qcow2-refcount: avoid eating RAM
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
>> ---
>> 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;
>>
>
>
Re: [Qemu-devel] [PATCH v2 2/7] block/qcow2-refcount: avoid eating RAM
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
>> ---
>> 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;
>>
>
>
Re: [Qemu-devel] [PATCH v2 2/7] block/qcow2-refcount: avoid eating RAM
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
> ---
> 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?
> +
> +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?
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;
>
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH v2 2/7] block/qcow2-refcount: avoid eating RAM
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
---
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;
+}
+
+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);
+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;
--
2.11.1
