[Devel] [PATCH] dm-qcow2: parse bitmap extension
Alexander Atanasov
alexander.atanasov at virtuozzo.com
Thu Feb 20 13:17:15 MSK 2025
Hello Andrey,
On 19.02.25 18:17, Andrey Zhadchenko wrote:
> If qcow2 file has only inactive bitmaps, we should not reset
> QCOW2_AUTOCLEAR_BITMAPS bit. Bitmaps are already allocated and
> we are not expected to change them.
> Still reset this bit if we encounter enabled bitmap.
In what situation this can happen? Why it is okay to clear the bit?
Can you add a comment with link for the documentation for this bitmaps
in the header along from where the structures are copied from so it is
there for reference.
>
> https://virtuozzo.atlassian.net/browse/VSTOR-99404
> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko at virtuozzo.com>
> ---
> drivers/md/dm-qcow2-target.c | 125 ++++++++++++++++++++++++++++++++++-
> drivers/md/dm-qcow2.h | 42 ++++++++++++
> 2 files changed, 165 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
> index 276eab9acc4f..9b85d79fb02c 100644
> --- a/drivers/md/dm-qcow2-target.c
> +++ b/drivers/md/dm-qcow2-target.c
> @@ -555,7 +555,7 @@ static int qcow2_check_convert_hdr(struct dm_target *ti,
> return 0;
>
> hdr->incompatible_features = be64_to_cpu(raw_hdr->incompatible_features);
> - hdr->autoclear_features = be64_to_cpu(raw_hdr->autoclear_features);
> + hdr->autoclear_features = 0;
> hdr->refcount_order = be32_to_cpu(raw_hdr->refcount_order);
> hdr->header_length = be32_to_cpu(raw_hdr->header_length);
>
> @@ -631,7 +631,7 @@ int qcow2_set_image_file_features(struct qcow2 *qcow2, bool dirty)
> return -EIO;
>
> raw_hdr = kmap(md->page);
> - qcow2->hdr.autoclear_features = raw_hdr->autoclear_features = 0;
> + raw_hdr->autoclear_features = cpu_to_be64(qcow2->hdr.autoclear_features);
> if (kernel_sets_dirty_bit) {
> if (dirty)
> raw_hdr->incompatible_features |= dirty_mask;
> @@ -709,12 +709,117 @@ static int qcow2_attach_file(struct dm_target *ti, struct qcow2_target *tgt,
> }
> ALLOW_ERROR_INJECTION(qcow2_attach_file, ERRNO);
>
> +static int qcow2_parse_bitmap_ext(struct dm_target *ti, struct qcow2 *qcow2,
> + struct Qcow2BitmapHeaderExt *ext)
> +{
> + struct Qcow2BitmapDirEntry entry;
> + u64 index, size, offset;
> + struct page **pages = NULL;
> + char *addr = NULL;
> + int i, ret;
> + bool clear_bitmaps = false;
> +
> + ext->bitmap_directory_offset = be64_to_cpu(ext->bitmap_directory_offset);
> + ext->bitmap_directory_size = be64_to_cpu(ext->bitmap_directory_size);
> + ext->nb_bitmaps = be32_to_cpu(ext->nb_bitmaps);
> +
> + if (!ext->nb_bitmaps ||
> + ext->nb_bitmaps > QCOW2_MAX_BITMAPS ||
> + ext->bitmap_directory_size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE ||
> + ext->bitmap_directory_offset % PAGE_SIZE)
> + return -EINVAL;
does it need a check if the offset is within the file ?
> +
> + index = ext->bitmap_directory_offset >> PAGE_SHIFT;
> + size = round_up(ext->bitmap_directory_size, PAGE_SIZE);
> +
> + addr = kvmalloc(size, GFP_KERNEL);
> + if (!addr)
> + return -ENOMEM;
> +
> + pages = kvmalloc((size / PAGE_SIZE) * sizeof(*pages), GFP_KERNEL);
kvmalloc_array
> + if (!pages) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + for (i = 0; i < size >> PAGE_SHIFT; i++)
> + pages[i] = virt_to_page(addr + i * PAGE_SIZE);
> +
> + ret = qcow2_rw_pages_sync(READ, qcow2, index, pages, size >> PAGE_SHIFT);
> + if (ret)
> + goto out;
> +
> + offset = 0;
> + while (offset < ext->bitmap_directory_size) {
> + memcpy(&entry, addr + offset, sizeof(entry));
check if offset+sizeof(entry) is less than size before accessing
> +
> + entry.flags = be32_to_cpu(entry.flags);
> + entry.name_size = be16_to_cpu(entry.name_size);
> + entry.extra_data_size = be32_to_cpu(entry.extra_data_size);
> +
> + if (entry.flags & BME_FLAG_AUTO)
> + clear_bitmaps = true;
> +
> + offset += round_up(sizeof(entry) + entry.name_size + entry.extra_data_size, 8);
> + }
> +
> + if (!clear_bitmaps)
> + qcow2->hdr.autoclear_features |= QCOW2_AUTOCLEAR_BITMAPS;
> +
> + ret = 0;
> +out:
> + kvfree(pages);
> + kvfree(addr);
> + return ret;
> +}
> +
> +static int qcow2_parse_extensions(struct dm_target *ti, struct qcow2 *qcow2,
> + loff_t start, loff_t end, void *addr)
> +{
> + loff_t offset;
> + struct QCowExtension ext;
> +
> + offset = start;
> + while (offset < end) {
needs a check if there is a complete sizeof(ext) left before end.
> + memcpy(&ext, (char *)addr + offset, sizeof(ext));
> + ext.magic = be32_to_cpu(ext.magic);
> + ext.len = be32_to_cpu(ext.len);
> + offset += sizeof(ext);
> +
> + switch (ext.magic) {
> + case QCOW2_EXT_MAGIC_BITMAPS:
> + struct Qcow2BitmapHeaderExt bitmaps_ext;
> +
> + if (ext.len != sizeof(bitmaps_ext)) {
> + QC_ERR(ti, "dm-qcow2: unexpected bitmap extension size\n");
> + return -EINVAL;
> + }
> +
> + memcpy(&bitmaps_ext, (char *)addr + offset,> +
sizeof(bitmaps_ext));
needs a check if there is space left before we go read past the end
Also void pointer arithmetic is okay in the kernel, it doesn't need a
cast here. But it is your choice whether or not to use it.
https://lore.kernel.org/lkml/CAHk-=whFKYMrF6euVvziW+drw7-yi1pYdf=uccnzJ8k09DoTXA@mail.gmail.com/t/#m983827708903c8c5bddf193343d392c9ed5af1a0
> + if (qcow2_parse_bitmap_ext(ti, qcow2, &bitmaps_ext))
> + return -EINVAL;
> + break;
> + case QCOW2_EXT_MAGIC_END:
> + return 0;
> + default:
> + /* Skip all other extensions */
> + break;
> + }
> +
> + offset += ((ext.len + 7) & ~7);
> + }
> +
> + return 0;
> +}
> +
> static int qcow2_parse_header(struct dm_target *ti, struct qcow2 *qcow2,
> struct qcow2 *upper, bool is_bottom)
> {
> struct QCowHeader *raw_hdr, *hdr = &qcow2->hdr;
> loff_t min_len, max_len, new_size;
> struct file *file = qcow2->file;
> + loff_t ext_start, ext_end;
> struct md_page *md;
> int ret;
>
> @@ -779,6 +884,22 @@ static int qcow2_parse_header(struct dm_target *ti, struct qcow2 *qcow2,
> ret = -EFBIG;
> if (qcow2->reftable_max_file_size < qcow2->file_size)
> goto out;
> +
> + ext_start = hdr->header_length;
> + if (hdr->backing_file_offset)
> + ext_end = hdr->backing_file_offset;
> + else
> + ext_end = PAGE_SIZE;
uint64_t backing_file_offset; i will assume that it can be a lot more
than a page size and i will guess that it must adjust ext_start position
not ext_end. can you clarify here ?
> +
> + if (ext_end > PAGE_SIZE) {
> + QC_INFO(ti, "dm-qcow2: extensions are too long, skipping them\n");
No need to have dm-qcow2: here it is added from QCOW2_FMT used in
QC_INFO/ERR .
> + } else {
> + ret = qcow2_parse_extensions(ti, qcow2, ext_start, ext_end, raw_hdr);
> + if (ret) {
> + QC_ERR(ti, "dm-qcow2: failed to parse extensions\n");
ditto
> + goto out;
> + }
> + }
> ret = 0;
> out:
> return ret;
> diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
> index 42f041a82a5b..d9bcbead0657 100644
> --- a/drivers/md/dm-qcow2.h
> +++ b/drivers/md/dm-qcow2.h
> @@ -43,6 +43,9 @@ struct QCowHeader {
> #define INCOMPATIBLE_FEATURES_EXTL2_BIT (1 << 4)
> uint64_t incompatible_features;
> uint64_t compatible_features;
> +
> +#define QCOW2_AUTOCLEAR_BITMAPS (1 << 0)
> +#define QCOW2_AUTOCLEAR_DATA_FILE_RAW (1 << 1)
> uint64_t autoclear_features;
>
> uint32_t refcount_order;
> @@ -57,6 +60,45 @@ struct QCowHeader {
> uint8_t padding[7];
> } __packed;
>
> +#define QCOW2_EXT_MAGIC_END 0
> +#define QCOW2_EXT_MAGIC_BITMAPS 0x23852875
> +
> +struct QCowExtension {
> + uint32_t magic;
> + uint32_t len;
> +} __packed;
> +
> +/* Bitmap header extension constraints */
> +#define QCOW2_MAX_BITMAPS 65535
> +#define QCOW2_MAX_BITMAP_DIRECTORY_SIZE (1024 * QCOW2_MAX_BITMAPS)
> +
> +struct Qcow2BitmapHeaderExt {
> + uint32_t nb_bitmaps;
> + uint32_t reserved32;
> + uint64_t bitmap_directory_size;
> + uint64_t bitmap_directory_offset;
> +} __packed;
> +
> +/* Bitmap directory entry flags */
> +#define BME_RESERVED_FLAGS 0xfffffffcU
> +#define BME_FLAG_IN_USE (1U << 0)
> +#define BME_FLAG_AUTO (1U << 1)
> +
> +struct 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 */
> +} __packed;
> +
> struct wb_desc {
> struct md_page *md;
> #define LX_INDEXES_PER_PAGE (PAGE_SIZE / sizeof(u64))
--
Regards,
Alexander Atanasov
More information about the Devel
mailing list