[Devel] [PATCH] dm-qcow2: parse bitmap extension
Andrey Zhadchenko
andrey.zhadchenko at virtuozzo.com
Thu Feb 20 14:52:54 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.
The bits are mandatory to clear if particular feature is not supported
and image is open for writing. So we should clear all bits we do not
know + only set QCOW2_AUTOCLEAR_BITMAPS if we think this will leave
bitmaps consistent (in our case if all present bitmaps are not recoding)
Sure, I will add the respective links.
>
>>
>> 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))
>
>
>
More information about the Devel
mailing list