[PATCH for-5.0] qcow2: Check request size in qcow2_co_pwritev_compressed_part()

[PATCH for-5.0] qcow2: Check request size in qcow2_co_pwritev_compressed_part()

[Alberto Garcia]

When issuing a compressed write request the number of bytes must be a
multiple of the cluster size.

With the current code such requests are allowed and we hit an
assertion:

   $ qemu-img create -f qcow2 img.qcow2 1M
   $ qemu-io -c 'write -c 0 32k' img.qcow2

   qemu-io: block/qcow2.c:4257: qcow2_co_pwritev_compressed_task:
   Assertion `bytes == s->cluster_size || (bytes < s->cluster_size &&
              (offset + bytes == bs->total_sectors << BDRV_SECTOR_BITS))' failed.
   Aborted

This patch fixes a regression introduced in 0d483dce38

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 2bb536b014..923ad428f0 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4345,7 +4345,7 @@ qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
         return bdrv_co_truncate(bs->file, len, false, PREALLOC_MODE_OFF, NULL);
     }
 
-    if (offset_into_cluster(s, offset)) {
+    if (offset_into_cluster(s, offset | bytes)) {
         return -EINVAL;
     }
 
-- 
2.20.1

Re: [PATCH for-5.0] qcow2: Check request size in qcow2_co_pwritev_compressed_part()

[Eric Blake]

On 4/3/20 11:57 AM, Alberto Garcia wrote:
> When issuing a compressed write request the number of bytes must be a
> multiple of the cluster size.
> 
> With the current code such requests are allowed and we hit an
> assertion:
> 
>     $ qemu-img create -f qcow2 img.qcow2 1M
>     $ qemu-io -c 'write -c 0 32k' img.qcow2
> 
>     qemu-io: block/qcow2.c:4257: qcow2_co_pwritev_compressed_task:
>     Assertion `bytes == s->cluster_size || (bytes < s->cluster_size &&
>                (offset + bytes == bs->total_sectors << BDRV_SECTOR_BITS))' failed.
>     Aborted
> 
> This patch fixes a regression introduced in 0d483dce38
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>   block/qcow2.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 2bb536b014..923ad428f0 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -4345,7 +4345,7 @@ qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
>           return bdrv_co_truncate(bs->file, len, false, PREALLOC_MODE_OFF, NULL);
>       }
>   
> -    if (offset_into_cluster(s, offset)) {
> +    if (offset_into_cluster(s, offset | bytes)) {
>           return -EINVAL;
>       }
>   
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH for-5.0] qcow2: Check request size in qcow2_co_pwritev_compressed_part()

[Vladimir Sementsov-Ogievskiy]

03.04.2020 20:40, Eric Blake wrote:
> On 4/3/20 11:57 AM, Alberto Garcia wrote:
>> When issuing a compressed write request the number of bytes must be a
>> multiple of the cluster size.
>>
>> With the current code such requests are allowed and we hit an
>> assertion:
>>
>>     $ qemu-img create -f qcow2 img.qcow2 1M
>>     $ qemu-io -c 'write -c 0 32k' img.qcow2
>>
>>     qemu-io: block/qcow2.c:4257: qcow2_co_pwritev_compressed_task:
>>     Assertion `bytes == s->cluster_size || (bytes < s->cluster_size &&
>>                (offset + bytes == bs->total_sectors << BDRV_SECTOR_BITS))' failed.
>>     Aborted
>>
>> This patch fixes a regression introduced in 0d483dce38
>>
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>> ---
>>   block/qcow2.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 2bb536b014..923ad428f0 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -4345,7 +4345,7 @@ qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
>>           return bdrv_co_truncate(bs->file, len, false, PREALLOC_MODE_OFF, NULL);
>>       }
>> -    if (offset_into_cluster(s, offset)) {
>> +    if (offset_into_cluster(s, offset | bytes)) {
>>           return -EINVAL;
>>       }
>>
> 

This will break compressed write to the tail of unaligned to cluster_size image, which is possible as I understand.

Check should make an exception for this case, like the assertion does:

len = bdrv_getlength(bs);
if (offset_into_cluster(s, offset) || (offset_into_cluster(bytes) && offset + bytes != len)) {
   return -EINVAL;
}


-- 
Best regards,
Vladimir

Re: [PATCH for-5.0] qcow2: Check request size in qcow2_co_pwritev_compressed_part()

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200403165752.18009-1-berto@igalia.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

Not run: 259
Failures: 053
Failed 1 of 116 iotests
make: *** [check-tests/check-block.sh] Error 1
make: *** Waiting for unfinished jobs....
  TEST    check-qtest-aarch64: tests/qtest/qos-test
Traceback (most recent call last):
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=bbedd703733d47e09453ee5d9ae135e1', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-u4oill6e/src/docker-src.2020-04-03-22.39.14.28831:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=bbedd703733d47e09453ee5d9ae135e1
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-u4oill6e/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    14m1.546s
user    0m8.497s


The full log is available at
http://patchew.org/logs/20200403165752.18009-1-berto@igalia.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH for-5.0] qcow2: Check request size in qcow2_co_pwritev_compressed_part()

[Andrey Shinkevich]

________________________________________
From: Alberto Garcia <berto@igalia.com>
Sent: Friday, April 3, 2020 7:57 PM
To: qemu-devel@nongnu.org
Cc: Alberto Garcia; qemu-block@nongnu.org; Andrey Shinkevich; Max Reitz; Kevin Wolf; Vladimir Sementsov-Ogievskiy; Pavel Butsykin
Subject: [PATCH for-5.0] qcow2: Check request size in qcow2_co_pwritev_compressed_part()

When issuing a compressed write request the number of bytes must be a
multiple of the cluster size.

With the current code such requests are allowed and we hit an
assertion:

   $ qemu-img create -f qcow2 img.qcow2 1M
   $ qemu-io -c 'write -c 0 32k' img.qcow2

   qemu-io: block/qcow2.c:4257: qcow2_co_pwritev_compressed_task:
   Assertion `bytes == s->cluster_size || (bytes < s->cluster_size &&
              (offset + bytes == bs->total_sectors << BDRV_SECTOR_BITS))' failed.
   Aborted

This patch fixes a regression introduced in 0d483dce38

The condition that QEMU supports writing compressed data of the size equal to one cluster was introduced with earlier patches.

Andrey

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 2bb536b014..923ad428f0 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4345,7 +4345,7 @@ qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
         return bdrv_co_truncate(bs->file, len, false, PREALLOC_MODE_OFF, NULL);
     }

-    if (offset_into_cluster(s, offset)) {
+    if (offset_into_cluster(s, offset | bytes)) {
         return -EINVAL;
     }

--
2.20.1