On 7/16/26 17:35, Denis V. Lunev wrote:
> This email originated from an IP that might not be authorized by the domain
> it was sent from.
> Do not click links or open attachments unless it is an email you expected to
> receive.
> qcow2_do_close() -> qcow2_inactivate() clears the dirty bit with a
> plain write to bs->file, unconditionally. A read-only node can still
> be dirty, inherited from an earlier writable session, and that write
> then hits a missing BLK_PERM_WRITE and asserts in
> bdrv_co_write_req_prepare() (block/io.c) on an entirely ordinary
> close -- closing is expected, the dirty bit on a read-only node
> is not.
>
> Skip the clear for read-only nodes, same as read access already does.
> Any other still-dirty node keeps the unguarded write: it is expected
> to hold write permission, and a missing one there is a bug worth
> seeing.
>
> Signed-off-by: Denis V. Lunev <[email protected]>
> CC: Kevin Wolf <[email protected]>
> CC: Hanna Reitz <[email protected]>
> ---
> block/qcow2.c | 6 +++++-
> tests/qemu-iotests/039 | 11 +++++++++++
> tests/qemu-iotests/039.out | 3 +++
> 3 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 2ab6ddd7b0..adb177e6b6 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2870,7 +2870,11 @@ static int GRAPH_RDLOCK
> qcow2_inactivate(BlockDriverState *bs)
> strerror(-ret));
> }
>
> - if (result == 0) {
> + /*
> + * A read-only node cannot resolve an inherited dirty bit here;
> + * leave it dirty, same as plain read access already does.
> + */
> + if (result == 0 && !bdrv_is_read_only(bs)) {
> qcow2_mark_clean(bs);
> }
>
> diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
> index e43e7026ce..94a8bfe754 100755
> --- a/tests/qemu-iotests/039
> +++ b/tests/qemu-iotests/039
> @@ -84,6 +84,17 @@ $QEMU_IO -r -c "read -P 0x5a 0 512" "$TEST_IMG" |
> _filter_qemu_io
> # The dirty bit must be set
> _qcow2_dump_header | grep incompatible_features
>
> +echo
> +echo "== Read-only open must not crash on close =="
> +
> +# We must not try to write the QCOW2 header to a read-only image.
> +$QEMU_IMG info --image-opts \
> +
> "driver=$IMGFMT,read-only=on,file.driver=file,file.filename=$TEST_IMG,file.read-only=off"
> \
> + > /dev/null
> +
> +# The dirty bit must still be set: this open never wrote any guest data
> +_qcow2_dump_header | grep incompatible_features
> +
> echo
> echo "== Repairing the image file must succeed =="
>
> diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
> index 8fdbcc528a..c66361128f 100644
> --- a/tests/qemu-iotests/039.out
> +++ b/tests/qemu-iotests/039.out
> @@ -24,6 +24,9 @@ read 512/512 bytes at offset 0
> 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> incompatible_features [0]
>
> +== Read-only open must not crash on close ==
> +incompatible_features [0]
> +
> == Repairing the image file must succeed ==
> ERROR cluster 5 refcount=0 reference=1
> Rebuilding refcount structure
Actually the problem is more subtle. The following command line results in
qemu-storage-daemon \
--blockdev
'{"driver":"file","filename":"dirty.qcow2","aio":"threads","node-name":"file0","cache":{"direct":false,"no-flush":false},"auto-read-only":true,"discard":"unmap"}'
\
--blockdev
'{"node-name":"fmt0","read-only":true,"discard":"unmap","cache":{"direct":false,"no-flush":false},"driver":"qcow2","file":"file0"}'
\
--chardev socket,id=qmp,path=master.sock,server=on,wait=off \
--monitor chardev=qmp
via query-named-block-nodes:
node-name=fmt0 drv=qcow2 ro=True
node-name=file0 drv=file ro=False
which would come to exactly configuration being asserted in the test
and this setup is pretty legal at my opinion. In production we have
had something lengthier with more snapshots.
File descriptor in turn is opened in a correct way - with O_RDONLY
that is why the problem was hidden for a lot of time.
Would it be more reasonable to disallow to set dirty rather than
to ignore is open question to me. Playing with permissions and
changing bdrv_is_read_only() seems more dangerous.
Also worth to note that bdrv_co_write_req_prepare() has wrong check.
It should check file descriptor rather than administrative permission.
This looks toooooooooo complex to me :(
Den
