[Qemu-devel] [PATCH v2 0/6] luks: Implement .bdrv_co_create

[Qemu-devel] [PATCH v2 0/6] luks: Implement .bdrv_co_create

[Kevin Wolf]

This series implements the .bdrv_co_create callback for luks, adds an
image creation test for it and contains some bonus fixes for bugs that
the test triggered.

v2:
- Use QCryptoBlockCreateOptionsLUKS as the base type for
  BlockdevCreateOptionsLUKS [Dan]
- Use INT64_MAX instead of UINT64_MAX as the maximum image size [Eric]
- Use iter_time=10 in tests where the creation succeeds [Dan]


Kevin Wolf (6):
  luks: Separate image file creation from formatting
  luks: Create block_crypto_co_create_generic()
  luks: Support .bdrv_co_create
  luks: Turn invalid assertion into check
  luks: Catch integer overflow for huge sizes
  qemu-iotests: Test luks QMP image creation

 qapi/block-core.json         |  17 +++-
 block/crypto.c               | 150 ++++++++++++++++++++++---------
 tests/qemu-iotests/209       | 210 +++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/209.out   | 136 ++++++++++++++++++++++++++++
 tests/qemu-iotests/common.rc |   2 +-
 tests/qemu-iotests/group     |   1 +
 6 files changed, 472 insertions(+), 44 deletions(-)
 create mode 100755 tests/qemu-iotests/209
 create mode 100644 tests/qemu-iotests/209.out

-- 
2.13.6

[Qemu-devel] [PATCH v2 1/6] luks: Separate image file creation from formatting

[Kevin Wolf]

The crypto driver used to create the image file in a callback from the
crypto subsystem. If we want to implement .bdrv_co_create, this needs to
go away because that callback will get a reference to an already
existing block node.

Move the image file creation to block_crypto_create_generic().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/crypto.c | 37 +++++++++++++++++--------------------
 1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index e6095e7807..77871640cc 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -71,8 +71,6 @@ static ssize_t block_crypto_read_func(QCryptoBlock *block,
 
 
 struct BlockCryptoCreateData {
-    const char *filename;
-    QemuOpts *opts;
     BlockBackend *blk;
     uint64_t size;
 };
@@ -103,27 +101,13 @@ static ssize_t block_crypto_init_func(QCryptoBlock *block,
                                       Error **errp)
 {
     struct BlockCryptoCreateData *data = opaque;
-    int ret;
 
     /* User provided size should reflect amount of space made
      * available to the guest, so we must take account of that
      * which will be used by the crypto header
      */
-    data->size += headerlen;
-
-    qemu_opt_set_number(data->opts, BLOCK_OPT_SIZE, data->size, &error_abort);
-    ret = bdrv_create_file(data->filename, data->opts, errp);
-    if (ret < 0) {
-        return -1;
-    }
-
-    data->blk = blk_new_open(data->filename, NULL, NULL,
-                             BDRV_O_RDWR | BDRV_O_PROTOCOL, errp);
-    if (!data->blk) {
-        return -1;
-    }
-
-    return 0;
+    return blk_truncate(data->blk, data->size + headerlen, PREALLOC_MODE_OFF,
+                        errp);
 }
 
 
@@ -333,11 +317,10 @@ static int block_crypto_create_generic(QCryptoBlockFormat format,
     struct BlockCryptoCreateData data = {
         .size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
                          BDRV_SECTOR_SIZE),
-        .opts = opts,
-        .filename = filename,
     };
     QDict *cryptoopts;
 
+    /* Parse options */
     cryptoopts = qemu_opts_to_qdict(opts, NULL);
 
     create_opts = block_crypto_create_opts_init(format, cryptoopts, errp);
@@ -345,6 +328,20 @@ static int block_crypto_create_generic(QCryptoBlockFormat format,
         return -1;
     }
 
+    /* Create protocol layer */
+    ret = bdrv_create_file(filename, opts, errp);
+    if (ret < 0) {
+        return ret;
+    }
+
+    data.blk = blk_new_open(filename, NULL, NULL,
+                            BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
+                            errp);
+    if (!data.blk) {
+        return -EINVAL;
+    }
+
+    /* Create format layer */
     crypto = qcrypto_block_create(create_opts, NULL,
                                   block_crypto_init_func,
                                   block_crypto_write_func,
-- 
2.13.6

[Qemu-devel] [PATCH v2 2/6] luks: Create block_crypto_co_create_generic()

[Kevin Wolf]

Everything that refers to the protocol layer or QemuOpts is moved out of
block_crypto_create_generic(), so that the remaining function is
suitable to be called by a .bdrv_co_create implementation.

LUKS is the only driver that actually implements the old interface, and
we don't intend to use it in any new drivers, so put the moved out code
directly into a LUKS function rather than creating a generic
intermediate one.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/crypto.c | 95 +++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 61 insertions(+), 34 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index 77871640cc..b0a4cb3388 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -306,43 +306,29 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
 }
 
 
-static int block_crypto_create_generic(QCryptoBlockFormat format,
-                                       const char *filename,
-                                       QemuOpts *opts,
-                                       Error **errp)
+static int block_crypto_co_create_generic(BlockDriverState *bs,
+                                          int64_t size,
+                                          QCryptoBlockCreateOptions *opts,
+                                          Error **errp)
 {
-    int ret = -EINVAL;
-    QCryptoBlockCreateOptions *create_opts = NULL;
+    int ret;
+    BlockBackend *blk;
     QCryptoBlock *crypto = NULL;
-    struct BlockCryptoCreateData data = {
-        .size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
-                         BDRV_SECTOR_SIZE),
-    };
-    QDict *cryptoopts;
-
-    /* Parse options */
-    cryptoopts = qemu_opts_to_qdict(opts, NULL);
+    struct BlockCryptoCreateData data;
 
-    create_opts = block_crypto_create_opts_init(format, cryptoopts, errp);
-    if (!create_opts) {
-        return -1;
-    }
+    blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
 
-    /* Create protocol layer */
-    ret = bdrv_create_file(filename, opts, errp);
+    ret = blk_insert_bs(blk, bs, errp);
     if (ret < 0) {
-        return ret;
+        goto cleanup;
     }
 
-    data.blk = blk_new_open(filename, NULL, NULL,
-                            BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
-                            errp);
-    if (!data.blk) {
-        return -EINVAL;
-    }
+    data = (struct BlockCryptoCreateData) {
+        .blk = blk,
+        .size = size,
+    };
 
-    /* Create format layer */
-    crypto = qcrypto_block_create(create_opts, NULL,
+    crypto = qcrypto_block_create(opts, NULL,
                                   block_crypto_init_func,
                                   block_crypto_write_func,
                                   &data,
@@ -355,10 +341,8 @@ static int block_crypto_create_generic(QCryptoBlockFormat format,
 
     ret = 0;
  cleanup:
-    QDECREF(cryptoopts);
     qcrypto_block_free(crypto);
-    blk_unref(data.blk);
-    qapi_free_QCryptoBlockCreateOptions(create_opts);
+    blk_unref(blk);
     return ret;
 }
 
@@ -563,8 +547,51 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename,
                                                          QemuOpts *opts,
                                                          Error **errp)
 {
-    return block_crypto_create_generic(Q_CRYPTO_BLOCK_FORMAT_LUKS,
-                                       filename, opts, errp);
+    QCryptoBlockCreateOptions *create_opts = NULL;
+    BlockDriverState *bs = NULL;
+    QDict *cryptoopts;
+    int64_t size;
+    int ret;
+
+    /* Parse options */
+    size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
+
+    cryptoopts = qemu_opts_to_qdict_filtered(opts, NULL,
+                                             &block_crypto_create_opts_luks,
+                                             true);
+
+    create_opts = block_crypto_create_opts_init(Q_CRYPTO_BLOCK_FORMAT_LUKS,
+                                                cryptoopts, errp);
+    if (!create_opts) {
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    /* Create protocol layer */
+    ret = bdrv_create_file(filename, opts, errp);
+    if (ret < 0) {
+        return ret;
+    }
+
+    bs = bdrv_open(filename, NULL, NULL,
+                   BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
+    if (!bs) {
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    /* Create format layer */
+    ret = block_crypto_co_create_generic(bs, size, create_opts, errp);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    ret = 0;
+fail:
+    bdrv_unref(bs);
+    qapi_free_QCryptoBlockCreateOptions(create_opts);
+    QDECREF(cryptoopts);
+    return ret;
 }
 
 static int block_crypto_get_info_luks(BlockDriverState *bs,
-- 
2.13.6

[Qemu-devel] [PATCH v2 3/6] luks: Support .bdrv_co_create

[Kevin Wolf]

This adds the .bdrv_co_create driver callback to luks, which enables
image creation over QMP.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/block-core.json | 17 ++++++++++++++++-
 block/crypto.c       | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 524d51567a..751adf89f4 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3452,6 +3452,21 @@
             '*preallocation':   'PreallocMode' } }
 
 ##
+# @BlockdevCreateOptionsLUKS:
+#
+# Driver specific image creation options for LUKS.
+#
+# @file             Node to create the image format on
+# @size             Size of the virtual disk in bytes
+#
+# Since: 2.12
+##
+{ 'struct': 'BlockdevCreateOptionsLUKS',
+  'base': 'QCryptoBlockCreateOptionsLUKS',
+  'data': { 'file':             'BlockdevRef',
+            'size':             'size' } }
+
+##
 # @BlockdevCreateOptionsNfs:
 #
 # Driver specific image creation options for NFS.
@@ -3643,7 +3658,7 @@
       'http':           'BlockdevCreateNotSupported',
       'https':          'BlockdevCreateNotSupported',
       'iscsi':          'BlockdevCreateNotSupported',
-      'luks':           'BlockdevCreateNotSupported',
+      'luks':           'BlockdevCreateOptionsLUKS',
       'nbd':            'BlockdevCreateNotSupported',
       'nfs':            'BlockdevCreateOptionsNfs',
       'null-aio':       'BlockdevCreateNotSupported',
diff --git a/block/crypto.c b/block/crypto.c
index b0a4cb3388..a1139b6f09 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -543,6 +543,39 @@ static int block_crypto_open_luks(BlockDriverState *bs,
                                      bs, options, flags, errp);
 }
 
+static int coroutine_fn
+block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error **errp)
+{
+    BlockdevCreateOptionsLUKS *luks_opts;
+    BlockDriverState *bs = NULL;
+    QCryptoBlockCreateOptions create_opts;
+    int ret;
+
+    assert(create_options->driver == BLOCKDEV_DRIVER_LUKS);
+    luks_opts = &create_options->u.luks;
+
+    bs = bdrv_open_blockdev_ref(luks_opts->file, errp);
+    if (bs == NULL) {
+        return -EIO;
+    }
+
+    create_opts = (QCryptoBlockCreateOptions) {
+        .format = Q_CRYPTO_BLOCK_FORMAT_LUKS,
+        .u.luks = *qapi_BlockdevCreateOptionsLUKS_base(luks_opts),
+    };
+
+    ret = block_crypto_co_create_generic(bs, luks_opts->size, &create_opts,
+                                         errp);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    ret = 0;
+fail:
+    bdrv_unref(bs);
+    return ret;
+}
+
 static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename,
                                                          QemuOpts *opts,
                                                          Error **errp)
@@ -647,6 +680,7 @@ BlockDriver bdrv_crypto_luks = {
     .bdrv_open          = block_crypto_open_luks,
     .bdrv_close         = block_crypto_close,
     .bdrv_child_perm    = bdrv_format_default_perms,
+    .bdrv_co_create     = block_crypto_co_create_luks,
     .bdrv_co_create_opts = block_crypto_co_create_opts_luks,
     .bdrv_truncate      = block_crypto_truncate,
     .create_opts        = &block_crypto_create_opts_luks,
-- 
2.13.6

[Qemu-devel] [PATCH v2 4/6] luks: Turn invalid assertion into check

[Kevin Wolf]

The .bdrv_getlength implementation of the crypto block driver asserted
that the payload offset isn't after EOF. This is an invalid assertion to
make as the image file could be corrupted. Instead, check it and return
-EIO if the file is too small for the payload offset.

Zero length images are fine, so trigger -EIO only on offset > len, not
on offset >= len as the assertion did before.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/crypto.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/crypto.c b/block/crypto.c
index a1139b6f09..16c371ec9c 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -518,7 +518,10 @@ static int64_t block_crypto_getlength(BlockDriverState *bs)
 
     uint64_t offset = qcrypto_block_get_payload_offset(crypto->block);
     assert(offset < INT64_MAX);
-    assert(offset < len);
+
+    if (offset > len) {
+       return -EIO;
+    }
 
     len -= offset;
 
-- 
2.13.6

[Qemu-devel] [PATCH v2 5/6] luks: Catch integer overflow for huge sizes

[Kevin Wolf]

When you request an image size close to UINT64_MAX, the addition of the
crypto header may cause an integer overflow. Catch it instead of
silently truncating the image size.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 block/crypto.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/block/crypto.c b/block/crypto.c
index 16c371ec9c..9dac08c6c5 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -102,6 +102,11 @@ static ssize_t block_crypto_init_func(QCryptoBlock *block,
 {
     struct BlockCryptoCreateData *data = opaque;
 
+    if (data->size > INT64_MAX || headerlen > INT64_MAX - data->size) {
+        error_setg(errp, "The requested file size is too large");
+        return -EFBIG;
+    }
+
     /* User provided size should reflect amount of space made
      * available to the guest, so we must take account of that
      * which will be used by the crypto header
-- 
2.13.6

[Qemu-devel] [PATCH v2 6/6] qemu-iotests: Test luks QMP image creation

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/209       | 210 +++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/209.out   | 136 ++++++++++++++++++++++++++++
 tests/qemu-iotests/common.rc |   2 +-
 tests/qemu-iotests/group     |   1 +
 4 files changed, 348 insertions(+), 1 deletion(-)
 create mode 100755 tests/qemu-iotests/209
 create mode 100644 tests/qemu-iotests/209.out

diff --git a/tests/qemu-iotests/209 b/tests/qemu-iotests/209
new file mode 100755
index 0000000000..96a5213e77
--- /dev/null
+++ b/tests/qemu-iotests/209
@@ -0,0 +1,210 @@
+#!/bin/bash
+#
+# Test luks and file image creation
+#
+# Copyright (C) 2018 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=kwolf@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1	# failure is the default!
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt luks
+_supported_proto file
+_supported_os Linux
+
+function do_run_qemu()
+{
+    echo Testing: "$@"
+    $QEMU -nographic -qmp stdio -serial none "$@"
+    echo
+}
+
+function run_qemu()
+{
+    do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp \
+                          | _filter_qemu | _filter_imgfmt \
+                          | _filter_actual_image_size
+}
+
+echo
+echo "=== Successful image creation (defaults) ==="
+echo
+
+size=$((128 * 1024 * 1024))
+
+run_qemu -object secret,id=keysec0,data="foo" <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "file",
+      "filename": "$TEST_IMG_FILE",
+      "size": 0
+  }
+}
+{ "execute": "blockdev-add",
+  "arguments": {
+      "driver": "file",
+      "node-name": "imgfile",
+      "filename": "$TEST_IMG_FILE"
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "imgfile",
+      "key-secret": "keysec0",
+      "size": $size,
+      "iter-time": 10
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info --format-specific | _filter_img_info --format-specific
+
+echo
+echo "=== Successful image creation (with non-default options) ==="
+echo
+
+# Choose a different size to show that we got a new image
+size=$((64 * 1024 * 1024))
+
+run_qemu -object secret,id=keysec0,data="foo" <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "file",
+      "filename": "$TEST_IMG_FILE",
+      "size": 0
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": {
+          "driver": "file",
+          "filename": "$TEST_IMG_FILE"
+      },
+      "size": $size,
+      "key-secret": "keysec0",
+      "cipher-alg": "twofish-128",
+      "cipher-mode": "ctr",
+      "ivgen-alg": "plain64",
+      "ivgen-hash-alg": "md5",
+      "hash-alg": "sha1",
+      "iter-time": 10
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info --format-specific | _filter_img_info --format-specific
+
+echo
+echo "=== Invalid BlockdevRef ==="
+echo
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "this doesn't exist",
+      "size": $size
+  }
+}
+{ "execute": "quit" }
+EOF
+
+echo
+echo "=== Zero size ==="
+echo
+
+run_qemu -blockdev driver=file,filename="$TEST_IMG_FILE",node-name=node0 \
+         -object secret,id=keysec0,data="foo" <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "key-secret": "keysec0",
+      "size": 0,
+      "iter-time": 10
+  }
+}
+{ "execute": "quit" }
+EOF
+
+_img_info | _filter_img_info
+
+
+echo
+echo "=== Invalid sizes ==="
+echo
+
+# TODO Negative image sizes aren't handled correctly, but this is a problem
+# with QAPI's implementation of the 'size' type and affects other commands as
+# well. Once this is fixed, we may want to add a test case here.
+
+# 1. 2^64 - 512
+# 2. 2^63 = 8 EB (qemu-img enforces image sizes less than this)
+# 3. 2^63 - 512 (generally valid, but with the crypto header the file will
+#                exceed 63 bits)
+
+run_qemu -blockdev driver=file,filename="$TEST_IMG_FILE",node-name=node0 \
+         -object secret,id=keysec0,data="foo" <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "key-secret": "keysec0",
+      "size": 18446744073709551104
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "key-secret": "keysec0",
+      "size": 9223372036854775808
+  }
+}
+{ "execute": "x-blockdev-create",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "file": "node0",
+      "key-secret": "keysec0",
+      "size": 9223372036854775296
+  }
+}
+{ "execute": "quit" }
+EOF
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/209.out b/tests/qemu-iotests/209.out
new file mode 100644
index 0000000000..f3dd2074a3
--- /dev/null
+++ b/tests/qemu-iotests/209.out
@@ -0,0 +1,136 @@
+QA output created by 209
+
+=== Successful image creation (defaults) ===
+
+Testing: -object secret,id=keysec0,data=foo
+QMP_VERSION
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
+
+image: json:{"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "key-secret": "keysec0"}
+file format: IMGFMT
+virtual size: 128M (134217728 bytes)
+Format specific information:
+    ivgen alg: plain64
+    hash alg: sha256
+    cipher alg: aes-256
+    uuid: 00000000-0000-0000-0000-000000000000
+    cipher mode: xts
+    slots:
+        [0]:
+            active: true
+            iters: 1024
+            key offset: 4096
+            stripes: 4000
+        [1]:
+            active: false
+            key offset: 262144
+        [2]:
+            active: false
+            key offset: 520192
+        [3]:
+            active: false
+            key offset: 778240
+        [4]:
+            active: false
+            key offset: 1036288
+        [5]:
+            active: false
+            key offset: 1294336
+        [6]:
+            active: false
+            key offset: 1552384
+        [7]:
+            active: false
+            key offset: 1810432
+    payload offset: 2068480
+    master key iters: 1024
+
+=== Successful image creation (with non-default options) ===
+
+Testing: -object secret,id=keysec0,data=foo
+QMP_VERSION
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
+
+image: json:{"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "key-secret": "keysec0"}
+file format: IMGFMT
+virtual size: 64M (67108864 bytes)
+Format specific information:
+    ivgen alg: plain64
+    hash alg: sha1
+    cipher alg: twofish-128
+    uuid: 00000000-0000-0000-0000-000000000000
+    cipher mode: ctr
+    slots:
+        [0]:
+            active: true
+            iters: 1024
+            key offset: 4096
+            stripes: 4000
+        [1]:
+            active: false
+            key offset: 69632
+        [2]:
+            active: false
+            key offset: 135168
+        [3]:
+            active: false
+            key offset: 200704
+        [4]:
+            active: false
+            key offset: 266240
+        [5]:
+            active: false
+            key offset: 331776
+        [6]:
+            active: false
+            key offset: 397312
+        [7]:
+            active: false
+            key offset: 462848
+    payload offset: 528384
+    master key iters: 1024
+
+=== Invalid BlockdevRef ===
+
+Testing:
+QMP_VERSION
+{"return": {}}
+{"error": {"class": "GenericError", "desc": "Cannot find device=this doesn't exist nor node_name=this doesn't exist"}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
+
+
+=== Zero size ===
+
+Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0 -object secret,id=keysec0,data=foo
+QMP_VERSION
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
+
+image: json:{"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "key-secret": "keysec0"}
+file format: IMGFMT
+virtual size: 0 (0 bytes)
+
+=== Invalid sizes ===
+
+Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0 -object secret,id=keysec0,data=foo
+QMP_VERSION
+{"return": {}}
+{"error": {"class": "GenericError", "desc": "The requested file size is too large"}}
+{"error": {"class": "GenericError", "desc": "The requested file size is too large"}}
+{"error": {"class": "GenericError", "desc": "The requested file size is too large"}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
+
+*** done
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 6fa0495e10..9a65a11026 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -332,7 +332,7 @@ _img_info()
 
     discard=0
     regex_json_spec_start='^ *"format-specific": \{'
-    $QEMU_IMG info "$@" "$TEST_IMG" 2>&1 | \
+    $QEMU_IMG info $QEMU_IMG_EXTRA_ARGS "$@" "$TEST_IMG" 2>&1 | \
         sed -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
             -e "s#$TEST_DIR#TEST_DIR#g" \
             -e "s#$IMGFMT#IMGFMT#g" \
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index c401791fcd..b8d0fd6177 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -204,3 +204,4 @@
 205 rw auto quick
 206 rw auto
 207 rw auto
+209 rw auto
-- 
2.13.6

Re: [Qemu-devel] [PATCH v2 3/6] luks: Support .bdrv_co_create

[Daniel P. Berrangé]

On Mon, Mar 12, 2018 at 04:02:15PM +0100, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to luks, which enables
> image creation over QMP.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi/block-core.json | 17 ++++++++++++++++-
>  block/crypto.c       | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 4/6] luks: Turn invalid assertion into check

[Daniel P. Berrangé]

On Mon, Mar 12, 2018 at 04:02:16PM +0100, Kevin Wolf wrote:
> The .bdrv_getlength implementation of the crypto block driver asserted
> that the payload offset isn't after EOF. This is an invalid assertion to
> make as the image file could be corrupted. Instead, check it and return
> -EIO if the file is too small for the payload offset.
> 
> Zero length images are fine, so trigger -EIO only on offset > len, not
> on offset >= len as the assertion did before.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/crypto.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)


Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 6/6] qemu-iotests: Test luks QMP image creation

[Daniel P. Berrangé]

On Mon, Mar 12, 2018 at 04:02:18PM +0100, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/209       | 210 +++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/209.out   | 136 ++++++++++++++++++++++++++++
>  tests/qemu-iotests/common.rc |   2 +-
>  tests/qemu-iotests/group     |   1 +
>  4 files changed, 348 insertions(+), 1 deletion(-)
>  create mode 100755 tests/qemu-iotests/209
>  create mode 100644 tests/qemu-iotests/209.out

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 0/6] luks: Implement .bdrv_co_create

[Kevin Wolf]

Am 12.03.2018 um 16:02 hat Kevin Wolf geschrieben:
> This series implements the .bdrv_co_create callback for luks, adds an
> image creation test for it and contains some bonus fixes for bugs that
> the test triggered.
> 
> v2:
> - Use QCryptoBlockCreateOptionsLUKS as the base type for
>   BlockdevCreateOptionsLUKS [Dan]
> - Use INT64_MAX instead of UINT64_MAX as the maximum image size [Eric]
> - Use iter_time=10 in tests where the creation succeeds [Dan]

Thanks for the review, applied to the block branch now.

Kevin

Re: [Qemu-devel] [PATCH v2 0/6] luks: Implement .bdrv_co_create

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180312150218.1314-1-kwolf@redhat.com
Subject: [Qemu-devel] [PATCH v2 0/6] luks: Implement .bdrv_co_create

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
21707c7252 qemu-iotests: Test luks QMP image creation
c3cce1168f luks: Catch integer overflow for huge sizes
bee065e0ba luks: Turn invalid assertion into check
b9d31479f8 luks: Support .bdrv_co_create
2dc9f95b59 luks: Create block_crypto_co_create_generic()
26821e37e9 luks: Separate image file creation from formatting

=== OUTPUT BEGIN ===
Checking PATCH 1/6: luks: Separate image file creation from formatting...
Checking PATCH 2/6: luks: Create block_crypto_co_create_generic()...
Checking PATCH 3/6: luks: Support .bdrv_co_create...
Checking PATCH 4/6: luks: Turn invalid assertion into check...
ERROR: suspect code indent for conditional statements (4, 7)
#31: FILE: block/crypto.c:522:
+    if (offset > len) {
+       return -EIO;

total: 1 errors, 0 warnings, 11 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 5/6: luks: Catch integer overflow for huge sizes...
Checking PATCH 6/6: qemu-iotests: Test luks QMP image creation...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH v2 2/6] luks: Create block_crypto_co_create_generic()

[Daniel P. Berrangé]

On Mon, Mar 12, 2018 at 04:02:14PM +0100, Kevin Wolf wrote:
> Everything that refers to the protocol layer or QemuOpts is moved out of
> block_crypto_create_generic(), so that the remaining function is
> suitable to be called by a .bdrv_co_create implementation.
> 
> LUKS is the only driver that actually implements the old interface, and
> we don't intend to use it in any new drivers, so put the moved out code
> directly into a LUKS function rather than creating a generic
> intermediate one.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/crypto.c | 95 +++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 61 insertions(+), 34 deletions(-)


Reviving a year old commit...

The LUKS driver doesn't implement preallocation during create.

Before this commit this would be reported

 $ qemu-img create -f luks --object secret,id=sec0,data=base -o key-secret=sec0 base.luks 1G -o preallocation=full
 Formatting 'base.luks', fmt=luks size=1073741824 key-secret=sec0 preallocation=full
 qemu-img: base.luks: Parameter 'preallocation' is unexpected


After this commit, there is no error reported - it just silently
ignores the preallocation=full option.

I'm a bit lost in block layer understanding where is the right
place to fix the error reporting in this case.

> 
> diff --git a/block/crypto.c b/block/crypto.c
> index 77871640cc..b0a4cb3388 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -306,43 +306,29 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
>  }
>  
>  
> -static int block_crypto_create_generic(QCryptoBlockFormat format,
> -                                       const char *filename,
> -                                       QemuOpts *opts,
> -                                       Error **errp)
> +static int block_crypto_co_create_generic(BlockDriverState *bs,
> +                                          int64_t size,
> +                                          QCryptoBlockCreateOptions *opts,
> +                                          Error **errp)
>  {
> -    int ret = -EINVAL;
> -    QCryptoBlockCreateOptions *create_opts = NULL;
> +    int ret;
> +    BlockBackend *blk;
>      QCryptoBlock *crypto = NULL;
> -    struct BlockCryptoCreateData data = {
> -        .size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> -                         BDRV_SECTOR_SIZE),
> -    };
> -    QDict *cryptoopts;
> -
> -    /* Parse options */
> -    cryptoopts = qemu_opts_to_qdict(opts, NULL);
> +    struct BlockCryptoCreateData data;
>  
> -    create_opts = block_crypto_create_opts_init(format, cryptoopts, errp);
> -    if (!create_opts) {
> -        return -1;
> -    }
> +    blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
>  
> -    /* Create protocol layer */
> -    ret = bdrv_create_file(filename, opts, errp);
> +    ret = blk_insert_bs(blk, bs, errp);
>      if (ret < 0) {
> -        return ret;
> +        goto cleanup;
>      }
>  
> -    data.blk = blk_new_open(filename, NULL, NULL,
> -                            BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
> -                            errp);
> -    if (!data.blk) {
> -        return -EINVAL;
> -    }
> +    data = (struct BlockCryptoCreateData) {
> +        .blk = blk,
> +        .size = size,
> +    };
>  
> -    /* Create format layer */
> -    crypto = qcrypto_block_create(create_opts, NULL,
> +    crypto = qcrypto_block_create(opts, NULL,
>                                    block_crypto_init_func,
>                                    block_crypto_write_func,
>                                    &data,
> @@ -355,10 +341,8 @@ static int block_crypto_create_generic(QCryptoBlockFormat format,
>  
>      ret = 0;
>   cleanup:
> -    QDECREF(cryptoopts);
>      qcrypto_block_free(crypto);
> -    blk_unref(data.blk);
> -    qapi_free_QCryptoBlockCreateOptions(create_opts);
> +    blk_unref(blk);
>      return ret;
>  }
>  
> @@ -563,8 +547,51 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename,
>                                                           QemuOpts *opts,
>                                                           Error **errp)
>  {
> -    return block_crypto_create_generic(Q_CRYPTO_BLOCK_FORMAT_LUKS,
> -                                       filename, opts, errp);
> +    QCryptoBlockCreateOptions *create_opts = NULL;
> +    BlockDriverState *bs = NULL;
> +    QDict *cryptoopts;
> +    int64_t size;
> +    int ret;
> +
> +    /* Parse options */
> +    size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
> +
> +    cryptoopts = qemu_opts_to_qdict_filtered(opts, NULL,
> +                                             &block_crypto_create_opts_luks,
> +                                             true);
> +
> +    create_opts = block_crypto_create_opts_init(Q_CRYPTO_BLOCK_FORMAT_LUKS,
> +                                                cryptoopts, errp);
> +    if (!create_opts) {
> +        ret = -EINVAL;
> +        goto fail;
> +    }
> +
> +    /* Create protocol layer */
> +    ret = bdrv_create_file(filename, opts, errp);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    bs = bdrv_open(filename, NULL, NULL,
> +                   BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
> +    if (!bs) {
> +        ret = -EINVAL;
> +        goto fail;
> +    }
> +
> +    /* Create format layer */
> +    ret = block_crypto_co_create_generic(bs, size, create_opts, errp);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +
> +    ret = 0;
> +fail:
> +    bdrv_unref(bs);
> +    qapi_free_QCryptoBlockCreateOptions(create_opts);
> +    QDECREF(cryptoopts);
> +    return ret;
>  }
>  
>  static int block_crypto_get_info_luks(BlockDriverState *bs,
> -- 
> 2.13.6
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 2/6] luks: Create block_crypto_co_create_generic()

[Kevin Wolf]

Am 14.05.2019 um 13:13 hat Daniel P. Berrangé geschrieben:
> On Mon, Mar 12, 2018 at 04:02:14PM +0100, Kevin Wolf wrote:
> > Everything that refers to the protocol layer or QemuOpts is moved out of
> > block_crypto_create_generic(), so that the remaining function is
> > suitable to be called by a .bdrv_co_create implementation.
> > 
> > LUKS is the only driver that actually implements the old interface, and
> > we don't intend to use it in any new drivers, so put the moved out code
> > directly into a LUKS function rather than creating a generic
> > intermediate one.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > ---
> >  block/crypto.c | 95 +++++++++++++++++++++++++++++++++++++---------------------
> >  1 file changed, 61 insertions(+), 34 deletions(-)
> 
> 
> Reviving a year old commit...
> 
> The LUKS driver doesn't implement preallocation during create.
> 
> Before this commit this would be reported
> 
>  $ qemu-img create -f luks --object secret,id=sec0,data=base -o key-secret=sec0 base.luks 1G -o preallocation=full
>  Formatting 'base.luks', fmt=luks size=1073741824 key-secret=sec0 preallocation=full
>  qemu-img: base.luks: Parameter 'preallocation' is unexpected
> 
> After this commit, there is no error reported - it just silently
> ignores the preallocation=full option.
> 
> I'm a bit lost in block layer understanding where is the right
> place to fix the error reporting in this case.

Hmm, this looked strange at first, but I see now where the difference
is.

In the old state, crypto used qemu_opts_to_qdict() and then fed all
options to its own visitor. I'm not sure whether I can clearly call this
a bug, but it's different from other format drivers, which parse all
options they know and pass the rest to the protocol driver.

The new version was converted to use qemu_opts_to_qdict_filtered() like
in all other drivers, so we got the same behaviour in crypto: Unknown
options are passed to the protocol.

As it happens, file-posix does support 'preallocation', so the operation
will return success now. Of course, passing the preallocation to the
protocol level is questionable for non-raw image formats, and completely
useless when you create the protocol level with size 0 and you'll call
blk_truncate() later.

I think we would get back to the old state if you just changed the
qemu_opts_to_qdict_filtered() call back to qemu_opts_to_qdict(). But it
make the driver inconsistent with other drivers again.

Maybe the best way to solve this would be to just implement
preallocation. It should be as easy as adding a 'preallocation' create
option (in QAPI and block_crypto_create_opts_luks), putting the
PreallocMode into BlockCryptoCreateData and then passing it to the
blk_truncate() call in block_crypto_init_func().

Kevin