On 01.10.2016 17:34, Max Reitz wrote:
On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
Create block/qcow2-bitmap.c
Add data structures and constraints accordingly to docs/specs/qcow2.txt
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
---
block/Makefile.objs | 2 +-
block/qcow2-bitmap.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
block/qcow2.h | 29 +++++++++++++++++++++++++++++
3 files changed, 77 insertions(+), 1 deletion(-)
create mode 100644 block/qcow2-bitmap.c
diff --git a/block/Makefile.objs b/block/Makefile.objs
index fa4d8b8..0f661bb 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -1,5 +1,5 @@
block-obj-y += raw_bsd.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o vvfat.o
dmg.o
-block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o
qcow2-cache.o
+block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o
qcow2-cache.o qcow2-bitmap.o
block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
block-obj-y += qed-check.o
block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
new file mode 100644
index 0000000..cd18b07
--- /dev/null
+++ b/block/qcow2-bitmap.c
@@ -0,0 +1,47 @@
+/*
+ * Bitmaps for the QCOW version 2 format
+ *
+ * Copyright (c) 2014-2016 Vladimir Sementsov-Ogievskiy
+ *
+ * This file is derived from qcow2-snapshot.c, original copyright:
+ * Copyright (c) 2004-2006 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+/* NOTICE: BME here means Bitmaps Extension and used as a namespace for
+ * _internal_ constants. Please do not use this _internal_ abbreviation for
+ * other needs and/or outside of this file. */
+
+/* Bitmap directory entry constraints */
+#define BME_MAX_TABLE_SIZE 0x8000000
+#define BME_MAX_PHYS_SIZE 0x20000000 /* 512 mb */
I suppose BME_MAX_TABLE_SIZE (8M) is greater than BME_MAX_PHYS_SIZE (512
MB) divided by the cluster size (>= 512; 512 MB / cluster_size <= 1 MB)
because fully zero or one clusters do not require any physical space?
Makes some sense, but I can see that this might make give some trouble
when trying to serialize overly large bitmaps. But I guess that comes
later in this series, so I'll wait for that point.
Another thing is that 512 MB is rather big. It gets worse: The bitmap
may only require 512 MB on disk, but with a maximum table size of 8 MB,
it can require up to 8M * cluster_size in memory (with just 64 MB of
disk space!) by using the "read as all zeroes" or "read as all ones"
flags. With the default cluster size of 64 kB, this would be 512 GB in
RAM. That sounds bad to me.
Well, it is probably fine as long as the bitmap is not auto-loaded...
But we do have a flag for exactly that. So it seems to me that a
manipulated image can easily consume huge amounts of RAM on the host.
So I think we also need some sane limitation on the in-RAM size of a
bitmap (which is BME_MAX_TABLE_SIZE * cluster_size, as far as I
understand). The question of course is, what is sane? For a server
system with no image manipulation possible from the outside, 1 GB may be
completely fine. But imagine you download some qcow2 image to your
laptop. Then, 1 GB may not be fine, actually.
Maybe it would make sense to use a runtime-adjustable limit here?
Actualy BME_MAX_PHYS_SIZE is this limit:
in check_constraints we have
uint64_t phys_bitmap_bytes =
(uint64_t)h->bitmap_table_size * s->cluster_size;
...
(phys_bitmap_bytes > BME_MAX_PHYS_SIZE) ||
+#define BME_MAX_GRANULARITY_BITS 31
+#define BME_MIN_GRANULARITY_BITS 9
+#define BME_MAX_NAME_SIZE 1023
+
+/* Bitmap directory entry flags */
+#define BME_RESERVED_FLAGS 0xffffffff
+
+/* bits [1, 8] U [56, 63] are reserved */
+#define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001fe
ull suffix is missing.
+
+typedef enum BitmapType {
+ BT_DIRTY_TRACKING_BITMAP = 1
+} BitmapType;
diff --git a/block/qcow2.h b/block/qcow2.h
index b36a7bf..0480b8b 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -52,6 +52,10 @@
* space for snapshot names and IDs */
#define QCOW_MAX_SNAPSHOTS_SIZE (1024 * QCOW_MAX_SNAPSHOTS)
+/* Bitmap header extension constraints */
+#define QCOW_MAX_DIRTY_BITMAPS 65535
+#define QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE (1024 * QCOW_MAX_DIRTY_BITMAPS)
+
/* indicate that the refcount of the referenced cluster is exactly one. */
#define QCOW_OFLAG_COPIED (1ULL << 63)
/* indicate that the cluster is compressed (they never have the copied flag)
*/
@@ -142,6 +146,22 @@ typedef struct QEMU_PACKED QCowSnapshotHeader {
/* name follows */
} QCowSnapshotHeader;
+/* Qcow2BitmapDirEntry is actually a bitmap directory entry */
This comment doesn't make a whole lot of sense now that this structure
is indeed called what it "actually is". ;-)
+typedef struct QEMU_PACKED Qcow2BitmapDirEntry {
+ /* header is 8 byte aligned */
+ uint64_t bitmap_table_offset;
+
+ uint32_t bitmap_table_size;
+ uint32_t flags;
+
+ uint8_t type;
+ uint8_t granularity_bits;
+ uint16_t name_size;
+ uint32_t extra_data_size;
+ /* extra data follows */
+ /* name follows */
+} Qcow2BitmapDirEntry;
+
typedef struct QEMU_PACKED QCowSnapshotExtraData {
uint64_t vm_state_size_large;
uint64_t disk_size;
@@ -222,6 +242,15 @@ typedef uint64_t Qcow2GetRefcountFunc(const void
*refcount_array,
typedef void Qcow2SetRefcountFunc(void *refcount_array,
uint64_t index, uint64_t value);
+/* Be careful, Qcow2BitmapHeaderExt is not an extension of Qcow2BitmapDirEntry, it
+ * is Qcow2 header extension */
And this makes even less sense now.
(These comments don't stop me from giving an R-b, but I'm not so sure
about the constants...)
Max
+typedef struct Qcow2BitmapHeaderExt {
+ uint32_t nb_bitmaps;
+ uint32_t reserved32;
+ uint64_t bitmap_directory_size;
+ uint64_t bitmap_directory_offset;
+} QEMU_PACKED Qcow2BitmapHeaderExt;
+
typedef struct BDRVQcow2State {
int cluster_bits;
int cluster_size;
--
Best regards,
Vladimir
