[Qemu-devel] [PATCH] qcow2 corruption: Fix alloc_cluster_link_l2

[Qemu-devel] [PATCH] qcow2 corruption: Fix alloc_cluster_link_l2

[Kevin Wolf]

This patch fixes a qcow2 corruption bug introduced in SVN Rev 5861. L2 tables
are big endian, so entries must be converted before being passed to functions.

This bug is easy to trigger. The following script will create and destroy a
qcow2 image (the header is gone after three loop iterations):

    #!/bin/bash
    qemu-img create -f qcow2 test.qcow 1M
    for i in $(seq 1 10); do
    qemu-system-x86_64 -hda test.qcow -monitor stdio > /dev/null 2>&1 <<EOF
    savevm test-$i
    quit
    EOF
    done

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

diff --git a/block-qcow2.c b/block-qcow2.c
index da8fb42..985214f 100644
--- a/block-qcow2.c
+++ b/block-qcow2.c
@@ -1007,7 +1007,7 @@ static int alloc_cluster_link_l2(BlockDriverState *bs, uint64_t cluster_offset,
         goto err;
 
     for (i = 0; i < j; i++)
-        free_any_clusters(bs, old_cluster[i], 1);
+        free_any_clusters(bs, be64_to_cpu(old_cluster[i]), 1);
 
     ret = 0;
 err:
-- 
1.6.0.6

[Qemu-devel] Re: [PATCH] qcow2 corruption: Fix alloc_cluster_link_l2

[Gleb Natapov]

On Wed, Apr 15, 2009 at 11:52:31AM +0200, Kevin Wolf wrote:
> This patch fixes a qcow2 corruption bug introduced in SVN Rev 5861. L2 tables
> are big endian, so entries must be converted before being passed to functions.
> 
> This bug is easy to trigger. The following script will create and destroy a
> qcow2 image (the header is gone after three loop iterations):
> 
>     #!/bin/bash
>     qemu-img create -f qcow2 test.qcow 1M
>     for i in $(seq 1 10); do
>     qemu-system-x86_64 -hda test.qcow -monitor stdio > /dev/null 2>&1 <<EOF
>     savevm test-$i
>     quit
>     EOF
>     done
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Gleb Natapov <gleb@redhat.com>

> ---
>  block-qcow2.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/block-qcow2.c b/block-qcow2.c
> index da8fb42..985214f 100644
> --- a/block-qcow2.c
> +++ b/block-qcow2.c
> @@ -1007,7 +1007,7 @@ static int alloc_cluster_link_l2(BlockDriverState *bs, uint64_t cluster_offset,
>          goto err;
>  
>      for (i = 0; i < j; i++)
> -        free_any_clusters(bs, old_cluster[i], 1);
> +        free_any_clusters(bs, be64_to_cpu(old_cluster[i]), 1);
>  
>      ret = 0;
>  err:
> -- 
> 1.6.0.6

--
			Gleb.

[Qemu-devel] Re: [PATCH] qcow2 corruption: Fix alloc_cluster_link_l2

[Consul]

Kevin Wolf wrote:
> This patch fixes a qcow2 corruption bug introduced in SVN Rev 5861. L2 tables
> are big endian, so entries must be converted before being passed to functions.
> 

Excellent! I think it is this bug who bit me more than once.
Acked-by: Alex Ivanov <void@aleksoft.net>

> This bug is easy to trigger. The following script will create and destroy a
> qcow2 image (the header is gone after three loop iterations):
> 
>     #!/bin/bash
>     qemu-img create -f qcow2 test.qcow 1M
>     for i in $(seq 1 10); do
>     qemu-system-x86_64 -hda test.qcow -monitor stdio > /dev/null 2>&1 <<EOF
>     savevm test-$i
>     quit
>     EOF
>     done
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block-qcow2.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/block-qcow2.c b/block-qcow2.c
> index da8fb42..985214f 100644
> --- a/block-qcow2.c
> +++ b/block-qcow2.c
> @@ -1007,7 +1007,7 @@ static int alloc_cluster_link_l2(BlockDriverState *bs, uint64_t cluster_offset,
>          goto err;
>  
>      for (i = 0; i < j; i++)
> -        free_any_clusters(bs, old_cluster[i], 1);
> +        free_any_clusters(bs, be64_to_cpu(old_cluster[i]), 1);
>  
>      ret = 0;
>  err:

Re: [Qemu-devel] Re: [PATCH] qcow2 corruption: Fix alloc_cluster_link_l2

[Marc Bevand]

On Wed, Apr 15, 2009 at 8:28 AM, Consul <void@aleksoft.net> wrote:
> Kevin Wolf wrote:
>>
>> This patch fixes a qcow2 corruption bug introduced in SVN Rev 5861. L2
>> tables
>> are big endian, so entries must be converted before being passed to
>> functions.
>>
>
> Excellent! I think it is this bug who bit me more than once.

Holly cow. Me too, that definitely sounds like the bug that bit me a
couple months ago:
http://article.gmane.org/gmane.comp.emulators.kvm.devel/28361

-marc

[Qemu-devel] Re: [PATCH] qcow2 corruption: Fix alloc_cluster_link_l2

[Anthony Liguori]

Kevin Wolf wrote:
> This patch fixes a qcow2 corruption bug introduced in SVN Rev 5861. L2 tables
> are big endian, so entries must be converted before being passed to functions.
>
> This bug is easy to trigger. The following script will create and destroy a
> qcow2 image (the header is gone after three loop iterations):
>
>     #!/bin/bash
>     qemu-img create -f qcow2 test.qcow 1M
>     for i in $(seq 1 10); do
>     qemu-system-x86_64 -hda test.qcow -monitor stdio > /dev/null 2>&1 <<EOF
>     savevm test-$i
>     quit
>     EOF
>     done
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Applied to stable and trunk.  Thanks!

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] qcow2 corruption: Fix alloc_cluster_link_l2

[Christoph Hellwig]

On Wed, Apr 15, 2009 at 11:52:31AM +0200, Kevin Wolf wrote:
> This patch fixes a qcow2 corruption bug introduced in SVN Rev 5861. L2 tables
> are big endian, so entries must be converted before being passed to functions.
> 
> This bug is easy to trigger. The following script will create and destroy a
> qcow2 image (the header is gone after three loop iterations):
> 
>     #!/bin/bash
>     qemu-img create -f qcow2 test.qcow 1M
>     for i in $(seq 1 10); do
>     qemu-system-x86_64 -hda test.qcow -monitor stdio > /dev/null 2>&1 <<EOF
>     savevm test-$i
>     quit
>     EOF
>     done
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block-qcow2.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/block-qcow2.c b/block-qcow2.c
> index da8fb42..985214f 100644
> --- a/block-qcow2.c
> +++ b/block-qcow2.c
> @@ -1007,7 +1007,7 @@ static int alloc_cluster_link_l2(BlockDriverState *bs, uint64_t cluster_offset,
>          goto err;
>  
>      for (i = 0; i < j; i++)
> -        free_any_clusters(bs, old_cluster[i], 1);
> +        free_any_clusters(bs, be64_to_cpu(old_cluster[i]), 1);

Maybe it's time to make use of sparse's endianess annotations in qcow2
to prevent further problems like this one?

Re: [Qemu-devel] [PATCH] qcow2 corruption: Fix alloc_cluster_link_l2

[Avi Kivity]

Christoph Hellwig wrote:
> Maybe it's time to make use of sparse's endianess annotations in qcow2
> to prevent further problems like this one

I'd much prefer strong type checking at compile time:

    typedef struct le32 {
        uint32_t blah;
    } le32;

    static inline le32 cpu_to_le32(uint32_t blah) { ... }
    static inline uint32_t le32_to_cpu(le32 blah) { ... }

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

Re: [Qemu-devel] [PATCH] qcow2 corruption: Fix alloc_cluster_link_l2

[Christoph Hellwig]

On Sun, Apr 26, 2009 at 05:14:03PM +0300, Avi Kivity wrote:
> Christoph Hellwig wrote:
> >Maybe it's time to make use of sparse's endianess annotations in qcow2
> >to prevent further problems like this one
> 
> I'd much prefer strong type checking at compile time:

sparse has strong typechecking at compiletime for the __bitwise types,
although you need the special sparse compiler to parse the annotations
:)

And the big advantage is that qemu already has the infrastructure for
it, while for your new structs you'd need various new helpers, including
for comparisms of disk endian types, etc.