[Qemu-devel] [PATCH v5 0/3] Fix qcow2+luks corruption introduced by commit 8ac0f15f335

[Qemu-devel] [PATCH v5 0/3] Fix qcow2+luks corruption introduced by commit 8ac0f15f335

[Maxim Levitsky]

Commit 8ac0f15f335 accidently broke the COW of non changed areas
of newly allocated clusters, when the write spans multiple clusters,
and needs COW both prior and after the write.
This results in 'after' COW area being encrypted with wrong
sector address, which render it corrupted.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1745922

CC: qemu-stable <qemu-stable@nongnu.org>

V2: grammar, spelling and code style fixes.
V3: more fixes after the review.
V4: addressed review comments from Max Reitz,
    and futher refactored the qcow2_co_encrypt to just take full host and guest offset
    which simplifies everything.

V5: reworked the patches so one of them fixes the bug
    only and other one is just refactoring

Best regards,
	Maxim Levitsky

Maxim Levitsky (3):
  Fix qcow2+luks corruption introduced by commit 8ac0f15f335
  block/qcow2: refactor threaded encryption code
  qemu-iotests: Add test for bz #1745922

 block/qcow2-cluster.c      | 29 +++++++-----
 block/qcow2-threads.c      | 62 ++++++++++++++++++++------
 block/qcow2.c              |  5 ++-
 block/qcow2.h              |  8 ++--
 tests/qemu-iotests/263     | 91 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/263.out | 40 +++++++++++++++++
 tests/qemu-iotests/group   |  2 +
 7 files changed, 206 insertions(+), 31 deletions(-)
 create mode 100755 tests/qemu-iotests/263
 create mode 100644 tests/qemu-iotests/263.out

-- 
2.17.2

[Qemu-devel] [PATCH v5 1/3] Fix qcow2+luks corruption introduced by commit 8ac0f15f335

[Maxim Levitsky]

This fixes subtle corruption introduced by luks threaded encryption
in commit 8ac0f15f335

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1745922

The corruption happens when we do a write that
   * writes to two or more unallocated clusters at once
   * doesn't fully cover the first sector
   * doesn't fully cover the last sector

In this case, when allocating the new clusters we COW both areas
prior to the write and after the write, and we encrypt them.

The above mentioned commit accidentally made it so we encrypt the
second COW area using the physical cluster offset of the first area.

The problem is that offset_in_cluster in do_perform_cow_encrypt
can be larger that the cluster size, thus cluster_offset
will no longer point to the start of the cluster at which encrypted
area starts.

Next patch in this series will refactor the code to avoid all these
assumptions.

In the bugreport that was triggered by rebasing a luks image to new,
zero filled base, which lot of such writes, and causes some files
with zero areas to contain garbage there instead.
But as described above it can happen elsewhere as well


Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 block/qcow2-cluster.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index dcacd3c450..bfeb0241d7 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -474,9 +474,10 @@ static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
         assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
         assert((bytes & ~BDRV_SECTOR_MASK) == 0);
         assert(s->crypto);
-        if (qcow2_co_encrypt(bs, cluster_offset,
-                             src_cluster_offset + offset_in_cluster,
-                             buffer, bytes) < 0) {
+        if (qcow2_co_encrypt(bs,
+                start_of_cluster(s, cluster_offset + offset_in_cluster),
+                src_cluster_offset + offset_in_cluster,
+                buffer, bytes) < 0) {
             return false;
         }
     }
-- 
2.17.2

[Qemu-devel] [PATCH v5 2/3] block/qcow2: refactor threaded encryption code

[Maxim Levitsky]

Change do_perform_cow_encrypt and its callee qcow2_co_encrypt
to just receive full host and guest offsets and in pariticular
remove the offset_in_cluster parameter of do_perform_cow_encrypt,
since it is misleading, because that offset can be larger than
cluster size currently.

Also document the qcow2_co_encrypt arguments to prevent
that bug from happening again

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 block/qcow2-cluster.c | 30 ++++++++++++---------
 block/qcow2-threads.c | 62 ++++++++++++++++++++++++++++++++++---------
 block/qcow2.c         |  5 ++--
 block/qcow2.h         |  8 +++---
 4 files changed, 73 insertions(+), 32 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index bfeb0241d7..e87a4637fd 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -463,21 +463,21 @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
 }
 
 static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
-                                                uint64_t src_cluster_offset,
-                                                uint64_t cluster_offset,
-                                                unsigned offset_in_cluster,
+                                                uint64_t guest_offset,
+                                                uint64_t host_offset,
                                                 uint8_t *buffer,
                                                 unsigned bytes)
 {
     if (bytes && bs->encrypted) {
         BDRVQcow2State *s = bs->opaque;
-        assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
-        assert((bytes & ~BDRV_SECTOR_MASK) == 0);
+
+        assert(QEMU_IS_ALIGNED(guest_offset, BDRV_SECTOR_SIZE));
+        assert(QEMU_IS_ALIGNED(host_offset, BDRV_SECTOR_SIZE));
+        assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
         assert(s->crypto);
-        if (qcow2_co_encrypt(bs,
-                start_of_cluster(s, cluster_offset + offset_in_cluster),
-                src_cluster_offset + offset_in_cluster,
-                buffer, bytes) < 0) {
+
+        if (qcow2_co_encrypt(bs, host_offset, guest_offset,
+            buffer, bytes) < 0) {
             return false;
         }
     }
@@ -891,11 +891,15 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta *m)
 
     /* Encrypt the data if necessary before writing it */
     if (bs->encrypted) {
-        if (!do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
-                                    start->offset, start_buffer,
+        if (!do_perform_cow_encrypt(bs,
+                                    m->offset + start->offset,
+                                    m->alloc_offset + start->offset,
+                                    start_buffer,
                                     start->nb_bytes) ||
-            !do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
-                                    end->offset, end_buffer, end->nb_bytes)) {
+            !do_perform_cow_encrypt(bs,
+                                    m->offset + end->offset,
+                                    m->alloc_offset + end->offset,
+                                    end_buffer, end->nb_bytes)) {
             ret = -EIO;
             goto fail;
         }
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 3b1e63fe41..9646243a9b 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -234,15 +234,15 @@ static int qcow2_encdec_pool_func(void *opaque)
 }
 
 static int coroutine_fn
-qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
-                  uint64_t offset, void *buf, size_t len, Qcow2EncDecFunc func)
+qcow2_co_encdec(BlockDriverState *bs, uint64_t host_offset,
+                uint64_t guest_offset, void *buf, size_t len,
+                Qcow2EncDecFunc func)
 {
     BDRVQcow2State *s = bs->opaque;
+
     Qcow2EncDecData arg = {
         .block = s->crypto,
-        .offset = s->crypt_physical_offset ?
-                      file_cluster_offset + offset_into_cluster(s, offset) :
-                      offset,
+        .offset = s->crypt_physical_offset ? host_offset : guest_offset,
         .buf = buf,
         .len = len,
         .func = func,
@@ -251,18 +251,54 @@ qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
     return qcow2_co_process(bs, qcow2_encdec_pool_func, &arg);
 }
 
+
+/*
+ * qcow2_co_encrypt()
+ *
+ * Encrypts one or more contiguous aligned sectors
+ *
+ * @host_offset - underlying storage offset of the first sector of the
+ * data to be encrypted
+
+ * @guest_offset - guest (virtual) offset of the first sector of the
+ * data to be encrypted
+ *
+ * @buf - buffer with the data to encrypt, that after encryption
+ *        will be written to the underlying storage device at
+ *        @host_offset
+ *
+ * @len - length of the buffer (must be a BDRV_SECTOR_SIZE multiple)
+ *
+ * Depending on the encryption method, @host_cluster_offset and/or @guest_offset
+ * may be used for generating the initialization vector for
+ * encryption.
+ *
+ * Note that while the whole range must be aligned on sectors, it
+ * does not have to be aligned on clusters and can also cross cluster
+ * boundaries
+ *
+ */
 int coroutine_fn
-qcow2_co_encrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
-                 uint64_t offset, void *buf, size_t len)
+qcow2_co_encrypt(BlockDriverState *bs, uint64_t host_offset,
+                 uint64_t guest_offset, void *buf, size_t len)
 {
-    return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
-                             qcrypto_block_encrypt);
+    return qcow2_co_encdec(bs, host_offset, guest_offset, buf, len,
+                           qcrypto_block_encrypt);
 }
 
+
+/*
+ * qcow2_co_decrypt()
+ *
+ * Decrypts one or more contiguous aligned sectors
+ * Similar to qcow2_co_encrypt
+ *
+ */
+
 int coroutine_fn
-qcow2_co_decrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
-                 uint64_t offset, void *buf, size_t len)
+qcow2_co_decrypt(BlockDriverState *bs, uint64_t host_offset,
+                 uint64_t guest_offset, void *buf, size_t len)
 {
-    return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
-                             qcrypto_block_decrypt);
+    return qcow2_co_encdec(bs, host_offset, guest_offset, buf, len,
+                           qcrypto_block_decrypt);
 }
diff --git a/block/qcow2.c b/block/qcow2.c
index 57734f20cf..ac768092bb 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2069,7 +2069,8 @@ static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs,
 
                 assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
                 assert((cur_bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
-                if (qcow2_co_decrypt(bs, cluster_offset, offset,
+                if (qcow2_co_decrypt(bs, cluster_offset + offset_in_cluster,
+                                     offset,
                                      cluster_data, cur_bytes) < 0) {
                     ret = -EIO;
                     goto fail;
@@ -2288,7 +2289,7 @@ static coroutine_fn int qcow2_co_pwritev_part(
             qemu_iovec_to_buf(qiov, qiov_offset + bytes_done,
                               cluster_data, cur_bytes);
 
-            if (qcow2_co_encrypt(bs, cluster_offset, offset,
+            if (qcow2_co_encrypt(bs, cluster_offset + offset_in_cluster, offset,
                                  cluster_data, cur_bytes) < 0) {
                 ret = -EIO;
                 goto out_unlocked;
diff --git a/block/qcow2.h b/block/qcow2.h
index 998bcdaef1..a488d761ff 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -758,10 +758,10 @@ ssize_t coroutine_fn
 qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
                     const void *src, size_t src_size);
 int coroutine_fn
-qcow2_co_encrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
-                 uint64_t offset, void *buf, size_t len);
+qcow2_co_encrypt(BlockDriverState *bs, uint64_t host_offset,
+                 uint64_t guest_offset, void *buf, size_t len);
 int coroutine_fn
-qcow2_co_decrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
-                 uint64_t offset, void *buf, size_t len);
+qcow2_co_decrypt(BlockDriverState *bs, uint64_t host_offset,
+                 uint64_t guest_offset, void *buf, size_t len);
 
 #endif
-- 
2.17.2

[Qemu-devel] [PATCH v5 3/3] qemu-iotests: Add test for bz #1745922

[Maxim Levitsky]

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 tests/qemu-iotests/263     | 91 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/263.out | 40 +++++++++++++++++
 tests/qemu-iotests/group   |  2 +
 3 files changed, 133 insertions(+)
 create mode 100755 tests/qemu-iotests/263
 create mode 100644 tests/qemu-iotests/263.out

diff --git a/tests/qemu-iotests/263 b/tests/qemu-iotests/263
new file mode 100755
index 0000000000..d2c030fae9
--- /dev/null
+++ b/tests/qemu-iotests/263
@@ -0,0 +1,91 @@
+#!/usr/bin/env bash
+#
+# Test encrypted write that crosses cluster boundary of two unallocated clusters
+# Based on 188
+#
+# Copyright (C) 2019 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mlevitsk@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto generic
+_supported_os Linux
+
+
+size=1M
+
+SECRET="secret,id=sec0,data=astrochicken"
+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
+
+
+_run_test()
+{
+	echo "== reading the whole image =="
+	$QEMU_IO --object $SECRET -c "read -P 0 0 $size" --image-opts "$1" | _filter_qemu_io | _filter_testdir
+
+	echo
+	echo "== write two 512 byte sectors on a cluster boundary =="
+	$QEMU_IO --object $SECRET -c "write -P 0xAA 0xFE00 0x400" --image-opts "$1" | _filter_qemu_io | _filter_testdir
+
+	echo
+	echo "== verify that the rest of the image is not changed =="
+	$QEMU_IO --object $SECRET -c "read -P 0x00 0x00000 0xFE00" --image-opts "$1" | _filter_qemu_io | _filter_testdir
+	$QEMU_IO --object $SECRET -c "read -P 0xAA 0x0FE00 0x400" --image-opts "$1" | _filter_qemu_io | _filter_testdir
+	$QEMU_IO --object $SECRET -c "read -P 0x00 0x10200 0xEFE00" --image-opts "$1" | _filter_qemu_io | _filter_testdir
+
+}
+
+
+echo
+echo "testing LUKS qcow2 encryption"
+echo
+
+_make_test_img --object $SECRET -o "encrypt.format=luks,encrypt.key-secret=sec0,encrypt.iter-time=10,cluster_size=64K" $size
+_run_test "driver=$IMGFMT,encrypt.key-secret=sec0,file.filename=$TEST_IMG"
+_cleanup_test_img
+
+echo
+echo "testing legacy AES qcow2 encryption"
+echo
+
+
+_make_test_img --object $SECRET -o "encrypt.format=aes,encrypt.key-secret=sec0,cluster_size=64K" $size
+_run_test "driver=$IMGFMT,encrypt.key-secret=sec0,file.filename=$TEST_IMG"
+_cleanup_test_img
+
+
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/263.out b/tests/qemu-iotests/263.out
new file mode 100644
index 0000000000..0c982c55cb
--- /dev/null
+++ b/tests/qemu-iotests/263.out
@@ -0,0 +1,40 @@
+QA output created by 263
+
+testing LUKS qcow2 encryption
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10
+== reading the whole image ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== write two 512 byte sectors on a cluster boundary ==
+wrote 1024/1024 bytes at offset 65024
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verify that the rest of the image is not changed ==
+read 65024/65024 bytes at offset 0
+63.500 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 1024/1024 bytes at offset 65024
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 982528/982528 bytes at offset 66048
+959.500 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+testing legacy AES qcow2 encryption
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 encrypt.format=aes encrypt.key-secret=sec0
+== reading the whole image ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== write two 512 byte sectors on a cluster boundary ==
+wrote 1024/1024 bytes at offset 65024
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verify that the rest of the image is not changed ==
+read 65024/65024 bytes at offset 0
+63.500 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 1024/1024 bytes at offset 65024
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 982528/982528 bytes at offset 66048
+959.500 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 6082c74806..8d58729ee1 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -274,5 +274,7 @@
 257 rw
 258 rw quick
 262 rw quick migration
+263 rw quick
 265 rw auto quick
 266 rw quick
+>>>>>>> patched
-- 
2.17.2

Re: [Qemu-devel] [PATCH v5 1/3] Fix qcow2+luks corruption introduced by commit 8ac0f15f335

[Vladimir Sementsov-Ogievskiy]

13.09.2019 18:28, Maxim Levitsky wrote:
> This fixes subtle corruption introduced by luks threaded encryption
> in commit 8ac0f15f335
> 
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1745922
> 
> The corruption happens when we do a write that
>     * writes to two or more unallocated clusters at once
>     * doesn't fully cover the first sector
>     * doesn't fully cover the last sector
> 
> In this case, when allocating the new clusters we COW both areas
> prior to the write and after the write, and we encrypt them.
> 
> The above mentioned commit accidentally made it so we encrypt the
> second COW area using the physical cluster offset of the first area.
> 
> The problem is that offset_in_cluster in do_perform_cow_encrypt
> can be larger that the cluster size, thus cluster_offset
> will no longer point to the start of the cluster at which encrypted
> area starts.
> 
> Next patch in this series will refactor the code to avoid all these
> assumptions.
> 
> In the bugreport that was triggered by rebasing a luks image to new,
> zero filled base, which lot of such writes, and causes some files
> with zero areas to contain garbage there instead.
> But as described above it can happen elsewhere as well
> 
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   block/qcow2-cluster.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index dcacd3c450..bfeb0241d7 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -474,9 +474,10 @@ static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
>           assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
>           assert((bytes & ~BDRV_SECTOR_MASK) == 0);
>           assert(s->crypto);
> -        if (qcow2_co_encrypt(bs, cluster_offset,
> -                             src_cluster_offset + offset_in_cluster,
> -                             buffer, bytes) < 0) {
> +        if (qcow2_co_encrypt(bs,
> +                start_of_cluster(s, cluster_offset + offset_in_cluster),
> +                src_cluster_offset + offset_in_cluster,
> +                buffer, bytes) < 0) {
>               return false;
>           }
>       }
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v5 2/3] block/qcow2: refactor threaded encryption code

[Vladimir Sementsov-Ogievskiy]

13.09.2019 18:28, Maxim Levitsky wrote:
> Change do_perform_cow_encrypt and its callee qcow2_co_encrypt
> to just receive full host and guest offsets and in pariticular
> remove the offset_in_cluster parameter of do_perform_cow_encrypt,
> since it is misleading, because that offset can be larger than
> cluster size currently.
> 
> Also document the qcow2_co_encrypt arguments to prevent
> that bug from happening again
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>   block/qcow2-cluster.c | 30 ++++++++++++---------
>   block/qcow2-threads.c | 62 ++++++++++++++++++++++++++++++++++---------
>   block/qcow2.c         |  5 ++--
>   block/qcow2.h         |  8 +++---
>   4 files changed, 73 insertions(+), 32 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index bfeb0241d7..e87a4637fd 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -463,21 +463,21 @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
>   }
>   
>   static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
> -                                                uint64_t src_cluster_offset,
> -                                                uint64_t cluster_offset,
> -                                                unsigned offset_in_cluster,
> +                                                uint64_t guest_offset,
> +                                                uint64_t host_offset,
>                                                   uint8_t *buffer,
>                                                   unsigned bytes)
>   {
>       if (bytes && bs->encrypted) {
>           BDRVQcow2State *s = bs->opaque;
> -        assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
> -        assert((bytes & ~BDRV_SECTOR_MASK) == 0);
> +
> +        assert(QEMU_IS_ALIGNED(guest_offset, BDRV_SECTOR_SIZE));
> +        assert(QEMU_IS_ALIGNED(host_offset, BDRV_SECTOR_SIZE));
> +        assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
>           assert(s->crypto);
> -        if (qcow2_co_encrypt(bs,
> -                start_of_cluster(s, cluster_offset + offset_in_cluster),
> -                src_cluster_offset + offset_in_cluster,
> -                buffer, bytes) < 0) {
> +
> +        if (qcow2_co_encrypt(bs, host_offset, guest_offset,
> +            buffer, bytes) < 0) {

strange alignment of second line of the condition.. [1]

>               return false;
>           }
>       }
> @@ -891,11 +891,15 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta *m)
>   
>       /* Encrypt the data if necessary before writing it */
>       if (bs->encrypted) {
> -        if (!do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
> -                                    start->offset, start_buffer,
> +        if (!do_perform_cow_encrypt(bs,
> +                                    m->offset + start->offset,
> +                                    m->alloc_offset + start->offset,
> +                                    start_buffer,
>                                       start->nb_bytes) ||
> -            !do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
> -                                    end->offset, end_buffer, end->nb_bytes)) {
> +            !do_perform_cow_encrypt(bs,
> +                                    m->offset + end->offset,
> +                                    m->alloc_offset + end->offset,
> +                                    end_buffer, end->nb_bytes)) {

Looking at this now, I think that do_perform_cow_encrypt can be dropped at all,
as it's now just an extra wrapper, sending same parameters to qcow2_co_encrypt.

I'll send a follow-up, if this patch goes as is.

>               ret = -EIO;
>               goto fail;
>           }
> diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
> index 3b1e63fe41..9646243a9b 100644
> --- a/block/qcow2-threads.c
> +++ b/block/qcow2-threads.c
> @@ -234,15 +234,15 @@ static int qcow2_encdec_pool_func(void *opaque)
>   }
>   
>   static int coroutine_fn
> -qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
> -                  uint64_t offset, void *buf, size_t len, Qcow2EncDecFunc func)
> +qcow2_co_encdec(BlockDriverState *bs, uint64_t host_offset,
> +                uint64_t guest_offset, void *buf, size_t len,
> +                Qcow2EncDecFunc func)
>   {
>       BDRVQcow2State *s = bs->opaque;
> +
>       Qcow2EncDecData arg = {
>           .block = s->crypto,
> -        .offset = s->crypt_physical_offset ?
> -                      file_cluster_offset + offset_into_cluster(s, offset) :
> -                      offset,
> +        .offset = s->crypt_physical_offset ? host_offset : guest_offset,
>           .buf = buf,
>           .len = len,
>           .func = func,
> @@ -251,18 +251,54 @@ qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
>       return qcow2_co_process(bs, qcow2_encdec_pool_func, &arg);
>   }
>   
> +
> +/*
> + * qcow2_co_encrypt()
> + *
> + * Encrypts one or more contiguous aligned sectors
> + *
> + * @host_offset - underlying storage offset of the first sector of the
> + * data to be encrypted
> +

asterisk missed

> + * @guest_offset - guest (virtual) offset of the first sector of the
> + * data to be encrypted
> + *
> + * @buf - buffer with the data to encrypt, that after encryption
> + *        will be written to the underlying storage device at
> + *        @host_offset
> + *
> + * @len - length of the buffer (must be a BDRV_SECTOR_SIZE multiple)
> + *
> + * Depending on the encryption method, @host_cluster_offset and/or @guest_offset

s/host_cluster_offset/host_offset

With at least alignment[1] and s/host_cluster_offset/host_offset/ fixed:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> + * may be used for generating the initialization vector for
> + * encryption.
> + *
> + * Note that while the whole range must be aligned on sectors, it
> + * does not have to be aligned on clusters and can also cross cluster
> + * boundaries
> + *

hmm, extra empty line IMHO.

> + */
>   int coroutine_fn
> -qcow2_co_encrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
> -                 uint64_t offset, void *buf, size_t len)
> +qcow2_co_encrypt(BlockDriverState *bs, uint64_t host_offset,
> +                 uint64_t guest_offset, void *buf, size_t len)
>   {
> -    return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
> -                             qcrypto_block_encrypt);
> +    return qcow2_co_encdec(bs, host_offset, guest_offset, buf, len,
> +                           qcrypto_block_encrypt);
>   }
>   
> +
> +/*
> + * qcow2_co_decrypt()
> + *
> + * Decrypts one or more contiguous aligned sectors
> + * Similar to qcow2_co_encrypt
> + *

and this one.

> + */
> +
>   int coroutine_fn
> -qcow2_co_decrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
> -                 uint64_t offset, void *buf, size_t len)
> +qcow2_co_decrypt(BlockDriverState *bs, uint64_t host_offset,
> +                 uint64_t guest_offset, void *buf, size_t len)
>   {
> -    return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
> -                             qcrypto_block_decrypt);
> +    return qcow2_co_encdec(bs, host_offset, guest_offset, buf, len,
> +                           qcrypto_block_decrypt);
>   }
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 57734f20cf..ac768092bb 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2069,7 +2069,8 @@ static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs,
>   
>                   assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
>                   assert((cur_bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> -                if (qcow2_co_decrypt(bs, cluster_offset, offset,
> +                if (qcow2_co_decrypt(bs, cluster_offset + offset_in_cluster,
> +                                     offset,
>                                        cluster_data, cur_bytes) < 0) {
>                       ret = -EIO;
>                       goto fail;
> @@ -2288,7 +2289,7 @@ static coroutine_fn int qcow2_co_pwritev_part(
>               qemu_iovec_to_buf(qiov, qiov_offset + bytes_done,
>                                 cluster_data, cur_bytes);
>   
> -            if (qcow2_co_encrypt(bs, cluster_offset, offset,
> +            if (qcow2_co_encrypt(bs, cluster_offset + offset_in_cluster, offset,
>                                    cluster_data, cur_bytes) < 0) {
>                   ret = -EIO;
>                   goto out_unlocked;
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 998bcdaef1..a488d761ff 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -758,10 +758,10 @@ ssize_t coroutine_fn
>   qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
>                       const void *src, size_t src_size);
>   int coroutine_fn
> -qcow2_co_encrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
> -                 uint64_t offset, void *buf, size_t len);
> +qcow2_co_encrypt(BlockDriverState *bs, uint64_t host_offset,
> +                 uint64_t guest_offset, void *buf, size_t len);
>   int coroutine_fn
> -qcow2_co_decrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
> -                 uint64_t offset, void *buf, size_t len);
> +qcow2_co_decrypt(BlockDriverState *bs, uint64_t host_offset,
> +                 uint64_t guest_offset, void *buf, size_t len);
>   
>   #endif
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v5 3/3] qemu-iotests: Add test for bz #1745922

[Vladimir Sementsov-Ogievskiy]

13.09.2019 18:28, Maxim Levitsky wrote:
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>   tests/qemu-iotests/263     | 91 ++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/263.out | 40 +++++++++++++++++
>   tests/qemu-iotests/group   |  2 +
>   3 files changed, 133 insertions(+)
>   create mode 100755 tests/qemu-iotests/263
>   create mode 100644 tests/qemu-iotests/263.out
> 
> diff --git a/tests/qemu-iotests/263 b/tests/qemu-iotests/263
> new file mode 100755
> index 0000000000..d2c030fae9
> --- /dev/null
> +++ b/tests/qemu-iotests/263

[..]

> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -274,5 +274,7 @@
>   257 rw
>   258 rw quick
>   262 rw quick migration
> +263 rw quick
>   265 rw auto quick
>   266 rw quick
> +>>>>>>> patched
> 

last line is a mistake.

also, test failed for me:

263      fail       [19:21:35] [19:21:35]                    output mismatch (see 263.out.bad)
--- /work/src/qemu/maxim-luks/tests/qemu-iotests/263.out        2019-09-13 19:11:45.464727427 +0300
+++ /work/src/qemu/maxim-luks/tests/qemu-iotests/263.out.bad    2019-09-13 19:21:35.535381253 +0300
@@ -2,6 +2,7 @@

  testing LUKS qcow2 encryption

+qemu-img: TEST_DIR/t.IMGFMT: No crypto library supporting PBKDF in this build: Function not implemented
  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10
  == reading the whole image ==
  read 1048576/1048576 bytes at offset 0
Failures: 263
Failed 1 of 1 tests


and if reconfigure with
  --enable-gnutls --enable-gcrypt --enable-vhost-crypto --enable-crypto-afalg
(don't know which one is actually needed)
it works..

so, we at least should skip the test if it's unsupported


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v5 2/3] block/qcow2: refactor threaded encryption code

[Maxim Levitsky]

On Fri, 2019-09-13 at 16:11 +0000, Vladimir Sementsov-Ogievskiy wrote:
> 13.09.2019 18:28, Maxim Levitsky wrote:
> > Change do_perform_cow_encrypt and its callee qcow2_co_encrypt
> > to just receive full host and guest offsets and in pariticular
> > remove the offset_in_cluster parameter of do_perform_cow_encrypt,
> > since it is misleading, because that offset can be larger than
> > cluster size currently.
> > 
> > Also document the qcow2_co_encrypt arguments to prevent
> > that bug from happening again
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >   block/qcow2-cluster.c | 30 ++++++++++++---------
> >   block/qcow2-threads.c | 62 ++++++++++++++++++++++++++++++++++---------
> >   block/qcow2.c         |  5 ++--
> >   block/qcow2.h         |  8 +++---
> >   4 files changed, 73 insertions(+), 32 deletions(-)
> > 
> > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > index bfeb0241d7..e87a4637fd 100644
> > --- a/block/qcow2-cluster.c
> > +++ b/block/qcow2-cluster.c
> > @@ -463,21 +463,21 @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
> >   }
> >   
> >   static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
> > -                                                uint64_t src_cluster_offset,
> > -                                                uint64_t cluster_offset,
> > -                                                unsigned offset_in_cluster,
> > +                                                uint64_t guest_offset,
> > +                                                uint64_t host_offset,
> >                                                   uint8_t *buffer,
> >                                                   unsigned bytes)
> >   {
> >       if (bytes && bs->encrypted) {
> >           BDRVQcow2State *s = bs->opaque;
> > -        assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
> > -        assert((bytes & ~BDRV_SECTOR_MASK) == 0);
> > +
> > +        assert(QEMU_IS_ALIGNED(guest_offset, BDRV_SECTOR_SIZE));
> > +        assert(QEMU_IS_ALIGNED(host_offset, BDRV_SECTOR_SIZE));
> > +        assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
> >           assert(s->crypto);
> > -        if (qcow2_co_encrypt(bs,
> > -                start_of_cluster(s, cluster_offset + offset_in_cluster),
> > -                src_cluster_offset + offset_in_cluster,
> > -                buffer, bytes) < 0) {
> > +
> > +        if (qcow2_co_encrypt(bs, host_offset, guest_offset,
> > +            buffer, bytes) < 0) {
> 
> strange alignment of second line of the condition.. [1]
To be honest I am too tired currently, I think I should align to the (
of function argument if possible.
I removed the do_perform_cow_encrypt as you suggested so no issue.

> 
> >               return false;
> >           }
> >       }
> > @@ -891,11 +891,15 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta *m)
> >   
> >       /* Encrypt the data if necessary before writing it */
> >       if (bs->encrypted) {
> > -        if (!do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
> > -                                    start->offset, start_buffer,
> > +        if (!do_perform_cow_encrypt(bs,
> > +                                    m->offset + start->offset,
> > +                                    m->alloc_offset + start->offset,
> > +                                    start_buffer,
> >                                       start->nb_bytes) ||
> > -            !do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
> > -                                    end->offset, end_buffer, end->nb_bytes)) {
> > +            !do_perform_cow_encrypt(bs,
> > +                                    m->offset + end->offset,
> > +                                    m->alloc_offset + end->offset,
> > +                                    end_buffer, end->nb_bytes)) {
> 
> Looking at this now, I think that do_perform_cow_encrypt can be dropped at all,
> as it's now just an extra wrapper, sending same parameters to qcow2_co_encrypt.
> 
> I'll send a follow-up, if this patch goes as is.
I kind of agree but I didn't want to make too many changes. I do think 
now also that it is worth it.


> 
> >               ret = -EIO;
> >               goto fail;
> >           }
> > diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
> > index 3b1e63fe41..9646243a9b 100644
> > --- a/block/qcow2-threads.c
> > +++ b/block/qcow2-threads.c
> > @@ -234,15 +234,15 @@ static int qcow2_encdec_pool_func(void *opaque)
> >   }
> >   
> >   static int coroutine_fn
> > -qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
> > -                  uint64_t offset, void *buf, size_t len, Qcow2EncDecFunc func)
> > +qcow2_co_encdec(BlockDriverState *bs, uint64_t host_offset,
> > +                uint64_t guest_offset, void *buf, size_t len,
> > +                Qcow2EncDecFunc func)
> >   {
> >       BDRVQcow2State *s = bs->opaque;
> > +
> >       Qcow2EncDecData arg = {
> >           .block = s->crypto,
> > -        .offset = s->crypt_physical_offset ?
> > -                      file_cluster_offset + offset_into_cluster(s, offset) :
> > -                      offset,
> > +        .offset = s->crypt_physical_offset ? host_offset : guest_offset,
> >           .buf = buf,
> >           .len = len,
> >           .func = func,
> > @@ -251,18 +251,54 @@ qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
> >       return qcow2_co_process(bs, qcow2_encdec_pool_func, &arg);
> >   }
> >   
> > +
> > +/*
> > + * qcow2_co_encrypt()
> > + *
> > + * Encrypts one or more contiguous aligned sectors
> > + *
> > + * @host_offset - underlying storage offset of the first sector of the
> > + * data to be encrypted
> > +
> 
> asterisk missed
Too tired :-(
> 
> > + * @guest_offset - guest (virtual) offset of the first sector of the
> > + * data to be encrypted
> > + *
> > + * @buf - buffer with the data to encrypt, that after encryption
> > + *        will be written to the underlying storage device at
> > + *        @host_offset
> > + *
> > + * @len - length of the buffer (must be a BDRV_SECTOR_SIZE multiple)
> > + *
> > + * Depending on the encryption method, @host_cluster_offset and/or @guest_offset
> 
> s/host_cluster_offset/host_offset
Opps, that is leftover, I remember that I already fixed that, but I guess
I didn't.
> 
> With at least alignment[1] and s/host_cluster_offset/host_offset/ fixed:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> > + * may be used for generating the initialization vector for
> > + * encryption.
> > + *
> > + * Note that while the whole range must be aligned on sectors, it
> > + * does not have to be aligned on clusters and can also cross cluster
> > + * boundaries
> > + *
> 
> hmm, extra empty line IMHO.

> 
> > + */
> >   int coroutine_fn
> > -qcow2_co_encrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
> > -                 uint64_t offset, void *buf, size_t len)
> > +qcow2_co_encrypt(BlockDriverState *bs, uint64_t host_offset,
> > +                 uint64_t guest_offset, void *buf, size_t len)
> >   {
> > -    return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
> > -                             qcrypto_block_encrypt);
> > +    return qcow2_co_encdec(bs, host_offset, guest_offset, buf, len,
> > +                           qcrypto_block_encrypt);
> >   }
> >   
> > +
> > +/*
> > + * qcow2_co_decrypt()
> > + *
> > + * Decrypts one or more contiguous aligned sectors
> > + * Similar to qcow2_co_encrypt
> > + *
> 
> and this one.
Doesn't really matter, but no problem to fix.

> 
> > + */
> > +
> >   int coroutine_fn
> > -qcow2_co_decrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
> > -                 uint64_t offset, void *buf, size_t len)
> > +qcow2_co_decrypt(BlockDriverState *bs, uint64_t host_offset,
> > +                 uint64_t guest_offset, void *buf, size_t len)
> >   {
> > -    return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
> > -                             qcrypto_block_decrypt);
> > +    return qcow2_co_encdec(bs, host_offset, guest_offset, buf, len,
> > +                           qcrypto_block_decrypt);
> >   }
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 57734f20cf..ac768092bb 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -2069,7 +2069,8 @@ static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs,
> >   
> >                   assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> >                   assert((cur_bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> > -                if (qcow2_co_decrypt(bs, cluster_offset, offset,
> > +                if (qcow2_co_decrypt(bs, cluster_offset + offset_in_cluster,
> > +                                     offset,
> >                                        cluster_data, cur_bytes) < 0) {
> >                       ret = -EIO;
> >                       goto fail;
> > @@ -2288,7 +2289,7 @@ static coroutine_fn int qcow2_co_pwritev_part(
> >               qemu_iovec_to_buf(qiov, qiov_offset + bytes_done,
> >                                 cluster_data, cur_bytes);
> >   
> > -            if (qcow2_co_encrypt(bs, cluster_offset, offset,
> > +            if (qcow2_co_encrypt(bs, cluster_offset + offset_in_cluster, offset,
> >                                    cluster_data, cur_bytes) < 0) {
> >                   ret = -EIO;
> >                   goto out_unlocked;
> > diff --git a/block/qcow2.h b/block/qcow2.h
> > index 998bcdaef1..a488d761ff 100644
> > --- a/block/qcow2.h
> > +++ b/block/qcow2.h
> > @@ -758,10 +758,10 @@ ssize_t coroutine_fn
> >   qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
> >                       const void *src, size_t src_size);
> >   int coroutine_fn
> > -qcow2_co_encrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
> > -                 uint64_t offset, void *buf, size_t len);
> > +qcow2_co_encrypt(BlockDriverState *bs, uint64_t host_offset,
> > +                 uint64_t guest_offset, void *buf, size_t len);
> >   int coroutine_fn
> > -qcow2_co_decrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
> > -                 uint64_t offset, void *buf, size_t len);
> > +qcow2_co_decrypt(BlockDriverState *bs, uint64_t host_offset,
> > +                 uint64_t guest_offset, void *buf, size_t len);
> >   
> >   #endif
> 


Thanks for the review,
	Best regards,
		Maxim Levitsky

Re: [Qemu-devel] [PATCH v5 3/3] qemu-iotests: Add test for bz #1745922

[Maxim Levitsky]

On Fri, 2019-09-13 at 16:27 +0000, Vladimir Sementsov-Ogievskiy wrote:
> 13.09.2019 18:28, Maxim Levitsky wrote:
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >   tests/qemu-iotests/263     | 91 ++++++++++++++++++++++++++++++++++++++
> >   tests/qemu-iotests/263.out | 40 +++++++++++++++++
> >   tests/qemu-iotests/group   |  2 +
> >   3 files changed, 133 insertions(+)
> >   create mode 100755 tests/qemu-iotests/263
> >   create mode 100644 tests/qemu-iotests/263.out
> > 
> > diff --git a/tests/qemu-iotests/263 b/tests/qemu-iotests/263
> > new file mode 100755
> > index 0000000000..d2c030fae9
> > --- /dev/null
> > +++ b/tests/qemu-iotests/263
> 
> [..]
> 
> > --- a/tests/qemu-iotests/group
> > +++ b/tests/qemu-iotests/group
> > @@ -274,5 +274,7 @@
> >   257 rw
> >   258 rw quick
> >   262 rw quick migration
> > +263 rw quick
> >   265 rw auto quick
> >   266 rw quick
> > +>>>>>>> patched
> > 
> 
> last line is a mistake.
Last minute merge mistake :-(

> 
> also, test failed for me:
> 
> 263      fail       [19:21:35] [19:21:35]                    output mismatch (see 263.out.bad)
> --- /work/src/qemu/maxim-luks/tests/qemu-iotests/263.out        2019-09-13 19:11:45.464727427 +0300
> +++ /work/src/qemu/maxim-luks/tests/qemu-iotests/263.out.bad    2019-09-13 19:21:35.535381253 +0300
> @@ -2,6 +2,7 @@
> 
>   testing LUKS qcow2 encryption
> 
> +qemu-img: TEST_DIR/t.IMGFMT: No crypto library supporting PBKDF in this build: Function not implemented
>   Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10
>   == reading the whole image ==
>   read 1048576/1048576 bytes at offset 0
> Failures: 263
> Failed 1 of 1 tests
> 
> 
> and if reconfigure with
>   --enable-gnutls --enable-gcrypt --enable-vhost-crypto --enable-crypto-afalg
> (don't know which one is actually needed)
> it works..
> 
> so, we at least should skip the test if it's unsupported
Don't know. I based this test on 188 and it also doesn't have anything special about this.
Its not in auto group though.
I guess we need to ask Daniel about this.



Best regards,
	Maxim Levitsky

Re: [Qemu-devel] [PATCH v5 3/3] qemu-iotests: Add test for bz #1745922

[Vladimir Sementsov-Ogievskiy]

13.09.2019 19:39, Maxim Levitsky wrote:
> On Fri, 2019-09-13 at 16:27 +0000, Vladimir Sementsov-Ogievskiy wrote:
>> 13.09.2019 18:28, Maxim Levitsky wrote:
>>> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
>>> ---
>>>    tests/qemu-iotests/263     | 91 ++++++++++++++++++++++++++++++++++++++
>>>    tests/qemu-iotests/263.out | 40 +++++++++++++++++
>>>    tests/qemu-iotests/group   |  2 +
>>>    3 files changed, 133 insertions(+)
>>>    create mode 100755 tests/qemu-iotests/263
>>>    create mode 100644 tests/qemu-iotests/263.out
>>>
>>> diff --git a/tests/qemu-iotests/263 b/tests/qemu-iotests/263
>>> new file mode 100755
>>> index 0000000000..d2c030fae9
>>> --- /dev/null
>>> +++ b/tests/qemu-iotests/263
>>
>> [..]
>>
>>> --- a/tests/qemu-iotests/group
>>> +++ b/tests/qemu-iotests/group
>>> @@ -274,5 +274,7 @@
>>>    257 rw
>>>    258 rw quick
>>>    262 rw quick migration
>>> +263 rw quick
>>>    265 rw auto quick
>>>    266 rw quick
>>> +>>>>>>> patched
>>>
>>
>> last line is a mistake.
> Last minute merge mistake :-(
> 
>>
>> also, test failed for me:
>>
>> 263      fail       [19:21:35] [19:21:35]                    output mismatch (see 263.out.bad)
>> --- /work/src/qemu/maxim-luks/tests/qemu-iotests/263.out        2019-09-13 19:11:45.464727427 +0300
>> +++ /work/src/qemu/maxim-luks/tests/qemu-iotests/263.out.bad    2019-09-13 19:21:35.535381253 +0300
>> @@ -2,6 +2,7 @@
>>
>>    testing LUKS qcow2 encryption
>>
>> +qemu-img: TEST_DIR/t.IMGFMT: No crypto library supporting PBKDF in this build: Function not implemented
>>    Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10
>>    == reading the whole image ==
>>    read 1048576/1048576 bytes at offset 0
>> Failures: 263
>> Failed 1 of 1 tests
>>
>>
>> and if reconfigure with
>>    --enable-gnutls --enable-gcrypt --enable-vhost-crypto --enable-crypto-afalg
>> (don't know which one is actually needed)
>> it works..
>>
>> so, we at least should skip the test if it's unsupported
> Don't know. I based this test on 188 and it also doesn't have anything special about this.
> Its not in auto group though.
> I guess we need to ask Daniel about this.
> 
> 

Hmm, the problem was not in "configure" arguments, seems needed thing is enabled by default, but actually
all works for me after installing gnutls-devel

and without it 188 don't work without it (or some its dependencies) too. So it's "not a regression"
and I'dont care:

Tested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
   [don't forget to drop ">>>>>>> patched"]

and I also checked that test fails if position it before patch 01, so it's true bug-catcher.

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v5 3/3] qemu-iotests: Add test for bz #1745922

[Maxim Levitsky]

On Fri, 2019-09-13 at 16:57 +0000, Vladimir Sementsov-Ogievskiy wrote:
> 13.09.2019 19:39, Maxim Levitsky wrote:
> > On Fri, 2019-09-13 at 16:27 +0000, Vladimir Sementsov-Ogievskiy wrote:
> > > 13.09.2019 18:28, Maxim Levitsky wrote:
> > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > > > ---
> > > >    tests/qemu-iotests/263     | 91 ++++++++++++++++++++++++++++++++++++++
> > > >    tests/qemu-iotests/263.out | 40 +++++++++++++++++
> > > >    tests/qemu-iotests/group   |  2 +
> > > >    3 files changed, 133 insertions(+)
> > > >    create mode 100755 tests/qemu-iotests/263
> > > >    create mode 100644 tests/qemu-iotests/263.out
> > > > 
> > > > diff --git a/tests/qemu-iotests/263 b/tests/qemu-iotests/263
> > > > new file mode 100755
> > > > index 0000000000..d2c030fae9
> > > > --- /dev/null
> > > > +++ b/tests/qemu-iotests/263
> > > 
> > > [..]
> > > 
> > > > --- a/tests/qemu-iotests/group
> > > > +++ b/tests/qemu-iotests/group
> > > > @@ -274,5 +274,7 @@
> > > >    257 rw
> > > >    258 rw quick
> > > >    262 rw quick migration
> > > > +263 rw quick
> > > >    265 rw auto quick
> > > >    266 rw quick
> > > > +>>>>>>> patched
> > > > 
> > > 
> > > last line is a mistake.
> > 
> > Last minute merge mistake :-(
> > 
> > > 
> > > also, test failed for me:
> > > 
> > > 263      fail       [19:21:35] [19:21:35]                    output mismatch (see 263.out.bad)
> > > --- /work/src/qemu/maxim-luks/tests/qemu-iotests/263.out        2019-09-13 19:11:45.464727427 +0300
> > > +++ /work/src/qemu/maxim-luks/tests/qemu-iotests/263.out.bad    2019-09-13 19:21:35.535381253 +0300
> > > @@ -2,6 +2,7 @@
> > > 
> > >    testing LUKS qcow2 encryption
> > > 
> > > +qemu-img: TEST_DIR/t.IMGFMT: No crypto library supporting PBKDF in this build: Function not implemented
> > >    Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10
> > >    == reading the whole image ==
> > >    read 1048576/1048576 bytes at offset 0
> > > Failures: 263
> > > Failed 1 of 1 tests
> > > 
> > > 
> > > and if reconfigure with
> > >    --enable-gnutls --enable-gcrypt --enable-vhost-crypto --enable-crypto-afalg
> > > (don't know which one is actually needed)
> > > it works..
> > > 
> > > so, we at least should skip the test if it's unsupported
> > 
> > Don't know. I based this test on 188 and it also doesn't have anything special about this.
> > Its not in auto group though.
> > I guess we need to ask Daniel about this.
> > 
> > 
> 
> Hmm, the problem was not in "configure" arguments, seems needed thing is enabled by default, but actually
> all works for me after installing gnutls-devel
> 
> and without it 188 don't work without it (or some its dependencies) too. So it's "not a regression"
> and I'dont care:
> 
> Tested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>    [don't forget to drop ">>>>>>> patched"]
> 
> and I also checked that test fails if position it before patch 01, so it's true bug-catcher.

Thanks!!

Best regards,
	Maxim Levitsky