[Qemu-devel] [PATCH] qcow2: Unset zero_beyond_eof in save_vmstate

[Qemu-devel] [PATCH] qcow2: Unset zero_beyond_eof in save_vmstate

[Max Reitz]

Saving the VM state is done using bdrv_pwrite. This function may perform
a read-modify-write, which in this case results in data being read from
beyond the end of the virtual disk. Since we are actually trying to
access an area which is not a part of the virtual disk, zero_beyond_eof
has to be set to false before performing the partial write, otherwise
the VM state may become corrupted.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
Follow-up to (depends on):
 - qcow2: Restore total_sectors value in save_vmstate
---
 block/qcow2.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 5c05bb5..3e11f25 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1941,12 +1941,15 @@ static int qcow2_save_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
     BDRVQcowState *s = bs->opaque;
     int64_t total_sectors = bs->total_sectors;
     int growable = bs->growable;
+    bool zero_beyond_eof = bs->zero_beyond_eof;
     int ret;
 
     BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_SAVE);
     bs->growable = 1;
+    bs->zero_beyond_eof = false;
     ret = bdrv_pwritev(bs, qcow2_vm_state_offset(s) + pos, qiov);
     bs->growable = growable;
+    bs->zero_beyond_eof = zero_beyond_eof;
     // bdrv_co_do_writev will have increased the total_sectors value to include
     // the VM state - the VM state is however not an actual part of the block
     // device, therefore, we need to restore the old value.
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] qcow2: Unset zero_beyond_eof in save_vmstate

[Eric Blake]

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

On 10/20/2013 08:52 PM, Max Reitz wrote:
> Saving the VM state is done using bdrv_pwrite. This function may perform
> a read-modify-write, which in this case results in data being read from
> beyond the end of the virtual disk. Since we are actually trying to
> access an area which is not a part of the virtual disk, zero_beyond_eof
> has to be set to false before performing the partial write, otherwise
> the VM state may become corrupted.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> Follow-up to (depends on):
>  - qcow2: Restore total_sectors value in save_vmstate

Reviewed-by: Eric Blake <eblake@redhat.com>

Do you have test cases that demonstrate the corruption pre-patch?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH] qcow2: Unset zero_beyond_eof in save_vmstate

[Max Reitz]

On 2013-10-21 22:37, Eric Blake wrote:
> On 10/20/2013 08:52 PM, Max Reitz wrote:
>> Saving the VM state is done using bdrv_pwrite. This function may perform
>> a read-modify-write, which in this case results in data being read from
>> beyond the end of the virtual disk. Since we are actually trying to
>> access an area which is not a part of the virtual disk, zero_beyond_eof
>> has to be set to false before performing the partial write, otherwise
>> the VM state may become corrupted.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> Follow-up to (depends on):
>>   - qcow2: Restore total_sectors value in save_vmstate
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> Do you have test cases that demonstrate the corruption pre-patch?

I could write a test case for the other patch, but for this one it would 
probably be rather difficult. What I did to bisect the bug was just 
starting a VM over and over while saving a snapshot at some time during 
boot-up and trying to load that snapshot again. Sometimes, qemu itself 
would report a corrupted VM state, but most of the time, the guest 
simply hang or paniced. This is something I can detect interactively, 
but I don't know if I could write a test for this (at least not for 
hanging).

Max

Re: [Qemu-devel] [PATCH] qcow2: Unset zero_beyond_eof in save_vmstate

[Max Reitz]

On 2013-10-21 22:37, Eric Blake wrote:
> On 10/20/2013 08:52 PM, Max Reitz wrote:
>> Saving the VM state is done using bdrv_pwrite. This function may perform
>> a read-modify-write, which in this case results in data being read from
>> beyond the end of the virtual disk. Since we are actually trying to
>> access an area which is not a part of the virtual disk, zero_beyond_eof
>> has to be set to false before performing the partial write, otherwise
>> the VM state may become corrupted.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> Follow-up to (depends on):
>>   - qcow2: Restore total_sectors value in save_vmstate
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> Do you have test cases that demonstrate the corruption pre-patch?

Okay, so it is possible to test for this after all; I've just sent a 
follow-up adding such a test. Thanks for pointing that out. ;-)

Max

Re: [Qemu-devel] [PATCH] qcow2: Unset zero_beyond_eof in save_vmstate

[Kevin Wolf]

Am 21.10.2013 um 22:37 hat Eric Blake geschrieben:
> On 10/20/2013 08:52 PM, Max Reitz wrote:
> > Saving the VM state is done using bdrv_pwrite. This function may perform
> > a read-modify-write, which in this case results in data being read from
> > beyond the end of the virtual disk. Since we are actually trying to
> > access an area which is not a part of the virtual disk, zero_beyond_eof
> > has to be set to false before performing the partial write, otherwise
> > the VM state may become corrupted.
> > 
> > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > ---
> > Follow-up to (depends on):
> >  - qcow2: Restore total_sectors value in save_vmstate
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks, applied to the block branch.

Kevin