| Message ID | 1382298755-21383-1-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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?
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
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
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
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.
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(+)