[Devel] [PATCH] dm-qcow2: parse bitmap extension
Andrey Zhadchenko
andrey.zhadchenko at virtuozzo.com
Thu Feb 20 15:19:18 MSK 2025
On 2/20/25 11:17, Alexander Atanasov wrote:
> 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 ?
Technically no. I think qcow2_rw_pages_sync() will just zero-fill
leftover pages..But I will add the check just in case.
>
>> +
>> + 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 ?
qcow2_parse_header maps and reads only first metadata page. So I have
limited ext_end to that value just in case. backing_file_offset is
expected to follow just after header and extensions. Realistically,
given the extensions currently must be unique, ext_len would never reach
even 4096.
>
>> +
>> + 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))
>
>
>
More information about the Devel
mailing list