[Qemu-devel] [PATCH] qcow2: Fix L1 table memory allocation

[Qemu-devel] [PATCH] qcow2: Fix L1 table memory allocation

[Kevin Wolf]

Contrary to what one could expect, the size of L1 tables is not cluster
aligned. So as we're writing whole sectors now instead of single entries,
we need to ensure that the L1 table in memory is large enough; otherwise
write would access memory after the end of the L1 table.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-cluster.c  |    2 +-
 block/qcow2-refcount.c |    2 +-
 block/qcow2.c          |    3 ++-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index d349655..057dac5 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -47,7 +47,7 @@ int qcow2_grow_l1_table(BlockDriverState *bs, int min_size)
 #endif
 
     new_l1_size2 = sizeof(uint64_t) * new_l1_size;
-    new_l1_table = qemu_mallocz(new_l1_size2);
+    new_l1_table = qemu_mallocz(align_offset(new_l1_size2, 512));
     memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t));
 
     /* write new table (align to cluster) */
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index d42c6e6..e6c857e 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -513,7 +513,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
     l1_size2 = l1_size * sizeof(uint64_t);
     l1_allocated = 0;
     if (l1_table_offset != s->l1_table_offset) {
-        l1_table = qemu_malloc(l1_size2);
+        l1_table = qemu_mallocz(align_offset(l1_size2, 512));
         l1_allocated = 1;
         if (bdrv_pread(s->hd, l1_table_offset,
                        l1_table, l1_size2) != l1_size2)
diff --git a/block/qcow2.c b/block/qcow2.c
index 20c114b..607329b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -208,7 +208,8 @@ static int qcow_open(BlockDriverState *bs, const char *filename, int flags)
     if (s->l1_size < s->l1_vm_state_index)
         goto fail;
     s->l1_table_offset = header.l1_table_offset;
-    s->l1_table = qemu_malloc(s->l1_size * sizeof(uint64_t));
+    s->l1_table = qemu_mallocz(
+        align_offset(s->l1_size * sizeof(uint64_t), 512));
     if (bdrv_pread(s->hd, s->l1_table_offset, s->l1_table, s->l1_size * sizeof(uint64_t)) !=
         s->l1_size * sizeof(uint64_t))
         goto fail;
-- 
1.6.0.6

Re: [Qemu-devel] [PATCH] qcow2: Fix L1 table memory allocation

[Avi Kivity]

On 07/07/2009 07:09 PM, Kevin Wolf wrote:
> Contrary to what one could expect, the size of L1 tables is not cluster
> aligned. So as we're writing whole sectors now instead of single entries,
> we need to ensure that the L1 table in memory is large enough; otherwise
> write would access memory after the end of the L1 table.
>
>
>       new_l1_size2 = sizeof(uint64_t) * new_l1_size;
> -    new_l1_table = qemu_mallocz(new_l1_size2);
> +    new_l1_table = qemu_mallocz(align_offset(new_l1_size2, 512));
>       memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t));
>    

Unrelated note: using qemu_memalign() here would reduce the copying for 
cache=none.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] qcow2: Fix L1 table memory allocation

[Kevin Wolf]

Avi Kivity schrieb:
> On 07/07/2009 07:09 PM, Kevin Wolf wrote:
>> Contrary to what one could expect, the size of L1 tables is not cluster
>> aligned. So as we're writing whole sectors now instead of single entries,
>> we need to ensure that the L1 table in memory is large enough; otherwise
>> write would access memory after the end of the L1 table.
>>
>>
>>       new_l1_size2 = sizeof(uint64_t) * new_l1_size;
>> -    new_l1_table = qemu_mallocz(new_l1_size2);
>> +    new_l1_table = qemu_mallocz(align_offset(new_l1_size2, 512));
>>       memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t));
>>    
> 
> Unrelated note: using qemu_memalign() here would reduce the copying for 
> cache=none.

Good point. I guess there are more places where we could use
qemu_memalign, so I would prefer to change them all in another patch.

Kevin

Re: [Qemu-devel] [PATCH] qcow2: Fix L1 table memory allocation

[Avi Kivity]

On 07/07/2009 07:32 PM, Kevin Wolf wrote:
>> Unrelated note: using qemu_memalign() here would reduce the copying for
>> cache=none.
>>      
>
> Good point. I guess there are more places where we could use
> qemu_memalign, so I would prefer to change them all in another patch.
>
>    

Definitely.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] qcow2: Fix L1 table memory allocation

[malc]

On Tue, 7 Jul 2009, Avi Kivity wrote:

> On 07/07/2009 07:09 PM, Kevin Wolf wrote:
> > Contrary to what one could expect, the size of L1 tables is not cluster
> > aligned. So as we're writing whole sectors now instead of single entries,
> > we need to ensure that the L1 table in memory is large enough; otherwise
> > write would access memory after the end of the L1 table.
> > 
> > 
> >       new_l1_size2 = sizeof(uint64_t) * new_l1_size;
> > -    new_l1_table = qemu_mallocz(new_l1_size2);
> > +    new_l1_table = qemu_mallocz(align_offset(new_l1_size2, 512));
> >       memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t));
> >    
> 
> Unrelated note: using qemu_memalign() here would reduce the copying for
> cache=none.
> 

Another unrelated note, qemu_memalign(and by extension qemu_vmalloc)
is NULL happy. And FWIW 487414f1cbd638beb0227c7da71fe7b8a821e155
removed NULL checks for qemu_memalgn and after that new code was added
that doesn't check for NULLs either (for instance e3f4e2a4 which has
bitten me).
   
-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] qcow2: Fix L1 table memory allocation

[Kevin Wolf]

malc schrieb:
> On Tue, 7 Jul 2009, Avi Kivity wrote:
> 
>> On 07/07/2009 07:09 PM, Kevin Wolf wrote:
>>> Contrary to what one could expect, the size of L1 tables is not cluster
>>> aligned. So as we're writing whole sectors now instead of single entries,
>>> we need to ensure that the L1 table in memory is large enough; otherwise
>>> write would access memory after the end of the L1 table.
>>>
>>>
>>>       new_l1_size2 = sizeof(uint64_t) * new_l1_size;
>>> -    new_l1_table = qemu_mallocz(new_l1_size2);
>>> +    new_l1_table = qemu_mallocz(align_offset(new_l1_size2, 512));
>>>       memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t));
>>>    
>> Unrelated note: using qemu_memalign() here would reduce the copying for
>> cache=none.
>>
> 
> Another unrelated note, qemu_memalign(and by extension qemu_vmalloc)
> is NULL happy. And FWIW 487414f1cbd638beb0227c7da71fe7b8a821e155
> removed NULL checks for qemu_memalgn and after that new code was added
> that doesn't check for NULLs either (for instance e3f4e2a4 which has
> bitten me).

Good to know that there still is an alternative that isn't broken.
Another good reason to switch to qemu_memalign.

Kevin