[pve-devel] PATCH -- bug in backup/restore for non-64K aligned disks + host mem leak to backups

[Lars Ellenberg]

A patch suggestion for the issue described below.

The interesting question is:
why are those "beyond end of device" blocks not zeroed out anyways,
and where does their content come from, given that we have seen
content that does NOT exist in the original image, it appears to be host
memory "garbage".

Still: these blocks should just be masked out.

Note: not even compile tested ...
So you may or may not have to fix some minor issues with this,
but it should show clearly where I suspect the issue is,
and how it could be fixed.

It looks like the reader already handles it "good enough",
as long as the last stored 4k block starts below end-of-device,
and remaining 4k blocks in that 64k "cluster" are masked out.
This patch makes sure that is the case.

This is still only 4k "clean", not 512 byte sector clean,
you may need to add an additional memset there for "within last 4k
block but beyond end-of-device sectors", if you deem that necessary.

Adding a few qemu_vfree for good measure.

Cheers,

    Lars

------------------------------------------------------------------------
diff -ru orig/vma-writer.c new/vma-writer.c
--- orig/vma-writer.c	2020-06-17 00:16:03.847547709 +0200
+++ new/vma-writer.c	2020-06-18 12:34:51.792459756 +0200
@@ -638,9 +638,11 @@
     if (buf) {
         int i;
         int bit = 1;
+        uint64_t byte_offset = cluster_num * VMA_CLUSTER_SIZE;
         for (i = 0; i < 16; i++) {
             const unsigned char *vmablock = buf + (i*VMA_BLOCK_SIZE);
-            if (!buffer_is_zero(vmablock, VMA_BLOCK_SIZE)) {
+            if (byte_offset < vmaw->stream_info[dev_id].size
+            &&  !buffer_is_zero(vmablock, VMA_BLOCK_SIZE)) {
                 mask |= bit;
                 memcpy(vmaw->outbuf + vmaw->outbuf_pos, vmablock,
                        VMA_BLOCK_SIZE);
@@ -651,6 +653,7 @@
                 *zero_bytes += VMA_BLOCK_SIZE;
             }
 
+            byte_offset += VMA_BLOCK_SIZE;
             bit = bit << 1;
         }
     } else {
@@ -767,5 +770,7 @@
         g_checksum_free(vmaw->md5csum);
     }
 
+    qemu_vfree(vmaw->headerbuf);
+    qemu_vfree(vmaw->outbuf);
     g_free(vmaw);
 }
diff -ru orig/vma.c new/vma.c
--- orig/vma.c	2020-06-17 00:16:03.847547709 +0200
+++ new/vma.c	2020-06-18 12:39:08.223091537 +0200
@@ -565,6 +565,7 @@
             g_warning("vma_writer_close failed %s", error_get_pretty(err));
         }
     }
+    qemu_vfree(buf);
 }
 
 static int create_archive(int argc, char **argv)
@@ -732,6 +733,7 @@
         g_error("creating vma archive failed");
     }
 
+    vma_writer_destroy(vmaw);
     return 0;
 }
------------------------------------------------------------------------
 

On Wed, Jun 17, 2020 at 03:04:27PM +0200, Roland Kammerer wrote:
> Dear Proxmox developers,
> 
> I hope it is fine to report this here, otherwise I can repost on
> pve-user.
> 
> I initially noticed that backup/restore has a bug when the disk size is
> not a multiple of 64K for DRBD devices. Due to DRBD meta-data the user
> visible size is almost certainly not 64K aligned. But this is not
> limited to DRBD and fairly easy to reproduce on plain LVM. Files used to
> reproduce can be found here[1].
> 
> - create a VG that is 1k aligned as in "vgcreate -s 1k backup
>   /dev/whatever"
> - add it to /etc/pve/storage.cfg
> 
> I created the storage via the GUI, there might be an easier CLI way:
> 
> - create a VM on the 1k aligned storage with 1 (GB) of space. Everything
>   else was set to defaults.
> - lvextend -L +4k /dev/backup/vm-100-disk-0 # GUI still shows an even
>   1GB
> - extend the disk via the GUI (1 additional GB); now the GUI shows
>   2097156K
> - zcat /tmp/breaksmox.img.gz - | dd of=/dev/backup/vm-100-disk-0 \
>   ;sync;sync;sync
> 
> >From that I created a backup (also via the GUI), all defaults except no
> compression.
> 
> On "vma verify" this fails:
> vma verify /tmp/breaksmox.vma
> vma: verify failed - got wrong block address - write beyond end
> Trace/breakpoint trap
> 
> For me every install of an Alpine Linux on such an "unaligned" disk
> triggers this bug (I always did a zero out before the install). The
> image from [1] is just zero outed at the beginning after the install to
> reduce the size. I did not try to minimize that, I just zeroed a bit
> less than 2GB of the installed image.
> 
> Here some observations where I *think* things go wrong: The interesting
> part is here:
> 
> xxd -s 301568 /tmp/breaksmox.vma
> 
> If you look at this last VMA Extent Header, the block info with number
> 0x8000 has a mask of 0xe1fe, and garbage data. I don't think there
> should be this (random) garbage at the end.
> 
> What is also also slightly worrisome is that I had images/backups where
> the garbage part of the backup contained data that could not be found in
> the image itself, so most likely this also leaks host memory to the
> backups.
> 
> Failing for non 64K aligned disks was also reported here[2].
> 
> Best, rck
> 
> [1] https://packages.linbit.com/rck/breaksmox/
> [2] https://forum.proxmox.com/threads/how-do-i-restore-a-vm-with-a-non-64k-block-disk.58863/