Re: [Devel] [PATCH hci-8.1 3/3] drivers/md/dm-qcow2: do not revalidate inconsistent persistent bitmaps

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dm-qcow2: qcow2_parse_header() reads autoclear_features from v2 headers
--

commit 023e7aaebe247a5bb8465f4b52b55cce93eb8686
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

drivers/md/dm-qcow2: do not revalidate inconsistent persistent bitmaps

This patch makes qcow2_parse_header() read autoclear_features from the raw
on-disk header. If the bitmaps bit is clear in the image, it clears that bit
in the parsed header after the extensions are parsed. The goal is to stop a
writable open from marking an inconsistent bitmaps extension consistent
again.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-138887

> diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
> --- a/drivers/md/dm-qcow2-target.c
> +++ b/drivers/md/dm-qcow2-target.c

[ ... ]

> @@ -865,6 +866,7 @@ static int qcow2_parse_header(struct dm_target *ti, struct qcow2 *qcow2,
>  		max_len = upper->hdr.size;
>  	}
>  	ret = qcow2_check_convert_hdr(ti, raw_hdr, hdr, min_len, max_len);
> +	raw_autoclear = be64_to_cpu(raw_hdr->autoclear_features);
>  	kunmap(md->page);
>  	if (ret < 0)
>  		goto out;

[Severity: Low]
Is it correct to read autoclear_features from raw_hdr unconditionally here?

qcow2_check_convert_hdr() returns early for version 2 images:

    if (hdr->version == 2)
        return 0;

so it never parses or zeroes autoclear_features for them. For a v2 image,
the field is not part of the header, and the bytes at that offset belong to
the extension area or padding. The new code then treats those bytes as
autoclear feature flags.

The v3 case looks similar. header_length is only checked against
sizeof(struct QCowHeader) on the compression path. A v3 image with a short
header_length would also have the bytes at that offset read as autoclear
flags.

The new code can only clear QCOW2_AUTOCLEAR_BITMAPS, never set it, and the
read stays inside the mapped page. The effect is therefore small.

Would it be better to do the read and the mask only when hdr->version >= 3?
That could go inside qcow2_check_convert_hdr(), next to the existing
hdr->autoclear_features = 0.

[ ... ]

> @@ -914,6 +916,9 @@ static int qcow2_parse_header(struct dm_target *ti, struct qcow2 *qcow2,
>  			goto out;
>  		}
>  	}
> +	/* Bitmaps invalidated before must not become consistent again */
> +	if (!(raw_autoclear & QCOW2_AUTOCLEAR_BITMAPS))
> +		hdr->autoclear_features &= ~QCOW2_AUTOCLEAR_BITMAPS;
>  	ret = 0;
>  out:
>  	return ret;

[Severity: Low]
For a v2 image, does this decision depend on a meaningless raw_autoclear
value?

The mask is applied based on whatever bytes sit at offset 88 of the header
page, rather than on a real autoclear_features field. It should be
conditional on hdr->version >= 3, as noted above.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023132008.2304901-1-alexander.ivanov@virtuozzo.com?part=3

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel