[Qemu-devel] [PATCH v6 0/4] delete created files when block_crypto_co_create_opts_luks fails

[Qemu-devel] [PATCH v6 0/4] delete created files when block_crypto_co_create_opts_luks fails

[Daniel Henrique Barboza]

Changes from previous version 5 [1] suggested by John Snow:
- patch 2: return ENOMEDIUM with !drv, return negative error
codes in bdrv_delete_file
- patch 3: clarify the meaning of returning ENOENT in the
comment

[1] https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg01173.html


Daniel Henrique Barboza (4):
  block: introducing 'bdrv_co_delete_file' interface
  block.c: adding bdrv_delete_file
  crypto.c: cleanup created file when block_crypto_co_create_opts_luks
    fails
  qemu-iotests: adding LUKS cleanup for non-UTF8 secret error

 block.c                    | 78 ++++++++++++++++++++++++++++++++++++++
 block/crypto.c             | 22 +++++++++++
 block/file-posix.c         | 28 ++++++++++++++
 include/block/block.h      |  1 +
 include/block/block_int.h  |  6 +++
 tests/qemu-iotests/259     | 67 ++++++++++++++++++++++++++++++++
 tests/qemu-iotests/259.out | 11 ++++++
 tests/qemu-iotests/group   |  1 +
 8 files changed, 214 insertions(+)
 create mode 100755 tests/qemu-iotests/259
 create mode 100644 tests/qemu-iotests/259.out

-- 
2.21.0

[Qemu-devel] [PATCH v6 1/4] block: introducing 'bdrv_co_delete_file' interface

[Daniel Henrique Barboza]

Adding to Block Drivers the capability of being able to clean up
its created files can be useful in certain situations. For the
LUKS driver, for instance, a failure in one of its authentication
steps can leave files in the host that weren't there before.

This patch adds the 'bdrv_co_delete_file' interface to block
drivers and add it to the 'file' driver in file-posix.c.The
implementation is given by 'raw_co_delete_file'.

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 block/file-posix.c        | 28 ++++++++++++++++++++++++++++
 include/block/block_int.h |  6 ++++++
 2 files changed, 34 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index fbeb0068db..52756de522 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2390,6 +2390,33 @@ static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts,
     return raw_co_create(&options, errp);
 }
 
+/**
+ * Co-routine function that erases a regular file.
+ */
+static int coroutine_fn raw_co_delete_file(BlockDriverState *bs,
+                                           Error **errp)
+{
+    struct stat st;
+    int ret;
+
+    if (!(stat(bs->filename, &st) == 0) || !S_ISREG(st.st_mode)) {
+        ret = -ENOENT;
+        error_setg_errno(errp, -ret, "%s is not a regular file",
+                         bs->filename);
+        goto done;
+    }
+
+    ret = unlink(bs->filename);
+    if (ret < 0) {
+        ret = -errno;
+        error_setg_errno(errp, -ret, "Error when deleting file %s",
+                         bs->filename);
+    }
+
+done:
+    return ret;
+}
+
 /*
  * Find allocation range in @bs around offset @start.
  * May change underlying file descriptor's file offset.
@@ -2942,6 +2969,7 @@ BlockDriver bdrv_file = {
     .bdrv_co_block_status = raw_co_block_status,
     .bdrv_co_invalidate_cache = raw_co_invalidate_cache,
     .bdrv_co_pwrite_zeroes = raw_co_pwrite_zeroes,
+    .bdrv_co_delete_file = raw_co_delete_file,
 
     .bdrv_co_preadv         = raw_co_preadv,
     .bdrv_co_pwritev        = raw_co_pwritev,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index ceec8c2f56..1442c75e3c 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -308,6 +308,12 @@ struct BlockDriver {
      */
     int coroutine_fn (*bdrv_co_flush)(BlockDriverState *bs);
 
+    /*
+     * Delete a local created file.
+     */
+    int coroutine_fn (*bdrv_co_delete_file)(BlockDriverState *bs,
+                                            Error **errp);
+
     /*
      * Flushes all data that was already written to the OS all the way down to
      * the disk (for example file-posix.c calls fsync()).
-- 
2.21.0

[Qemu-devel] [PATCH v6 2/4] block.c: adding bdrv_delete_file

[Daniel Henrique Barboza]

Using the new 'bdrv_co_delete_file' interface, bdrv_delete_file
can be used in a way similar of the existing bdrv_create_file to
to clean up a created file.

The logic is also similar to what is already done in bdrv_create_file:
a qemu_coroutine is created if needed, a specialized function
bdrv_delete_co_entry is used to call the bdrv_co_delete_file
co-routine of the driver, if the driver implements it.

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 block.c               | 78 +++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |  1 +
 2 files changed, 79 insertions(+)

diff --git a/block.c b/block.c
index 874a29a983..75212d873a 100644
--- a/block.c
+++ b/block.c
@@ -548,6 +548,84 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
     return ret;
 }
 
+typedef struct DeleteCo {
+    BlockDriver *drv;
+    BlockDriverState *bs;
+    int ret;
+    Error *err;
+} DeleteCo;
+
+static void coroutine_fn bdrv_delete_co_entry(void *opaque)
+{
+    Error *local_err = NULL;
+    DeleteCo *dco = opaque;
+
+    assert(dco->bs);
+
+    dco->ret = dco->drv->bdrv_co_delete_file(dco->bs, &local_err);
+    error_propagate(&dco->err, local_err);
+}
+
+int bdrv_delete_file(const char *filename, Error **errp)
+{
+    BlockDriver *drv = bdrv_find_protocol(filename, true, NULL);
+    BlockDriverState *bs = bdrv_open(filename, NULL, NULL,
+                                     BDRV_O_RDWR | BDRV_O_PROTOCOL,
+                                     NULL);
+    DeleteCo dco = {
+        .drv = drv,
+        .bs = bs,
+        .ret = NOT_DONE,
+        .err = NULL,
+    };
+    Coroutine *co;
+    int ret;
+
+    if (!drv) {
+        error_setg(errp, "File '%s' has unknown format", filename);
+        ret = -ENOMEDIUM;
+        goto out;
+    }
+
+    if (!drv->bdrv_co_delete_file) {
+        error_setg(errp, "Driver '%s' does not support image delete",
+                   drv->format_name);
+        ret = -ENOTSUP;
+        goto out;
+    }
+
+    if (!bs) {
+        error_setg(errp, "Could not open image '%s' for erasing",
+                   filename);
+        ret = -1;
+        goto out;
+    }
+
+    if (qemu_in_coroutine()) {
+        /* Fast-path if already in coroutine context */
+        bdrv_delete_co_entry(&dco);
+    } else {
+        co = qemu_coroutine_create(bdrv_delete_co_entry, &dco);
+        qemu_coroutine_enter(co);
+        while (dco.ret == NOT_DONE) {
+            aio_poll(qemu_get_aio_context(), true);
+        }
+    }
+
+    ret = dco.ret;
+    if (ret < 0) {
+        if (dco.err) {
+            error_propagate(errp, dco.err);
+        } else {
+            error_setg_errno(errp, -ret, "Could not delete image");
+        }
+    }
+
+out:
+    bdrv_unref(bs);
+    return ret;
+}
+
 /**
  * Try to get @bs's logical and physical block size.
  * On success, store them in @bsz struct and return 0.
diff --git a/include/block/block.h b/include/block/block.h
index 124ad40809..dbe737202b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -374,6 +374,7 @@ bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState *base,
 int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
                               Error **errp);
 void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base);
+int bdrv_delete_file(const char *filename, Error **errp);
 
 
 typedef struct BdrvCheckResult {
-- 
2.21.0

[Qemu-devel] [PATCH v6 3/4] crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails

[Daniel Henrique Barboza]

When using a non-UTF8 secret to create a volume using qemu-img, the
following error happens:

$ qemu-img create -f luks --object secret,id=vol_1_encrypt0,file=vol_resize_pool.vol_1.secret.qzVQrI -o key-secret=vol_1_encrypt0 /var/tmp/pool_target/vol_1 10240K

Formatting '/var/tmp/pool_target/vol_1', fmt=luks size=10485760 key-secret=vol_1_encrypt0
qemu-img: /var/tmp/pool_target/vol_1: Data from secret vol_1_encrypt0 is not valid UTF-8

However, the created file '/var/tmp/pool_target/vol_1' is left behind in the
file system after the failure. This behavior can be observed when creating
the volume using Libvirt, via 'virsh vol-create', and then getting "volume
target path already exist" errors when trying to re-create the volume.

The volume file is created inside block_crypto_co_create_opts_luks(), in
block/crypto.c. If the bdrv_create_file() call is successful but any
succeeding step fails*, the existing 'fail' label does not take into
account the created file, leaving it behind.

This patch changes block_crypto_co_create_opts_luks() to delete
'filename' in case of failure. A failure in this point means that
the volume is now truncated/corrupted, so even if 'filename' was an
existing volume before calling qemu-img, it is now unusable. Deleting
the file it is not much worse than leaving it in the filesystem in
this scenario, and we don't have to deal with checking the file
pre-existence in the code.

* in our case, block_crypto_co_create_generic calls qcrypto_block_create,
which calls qcrypto_block_luks_create, and this function fails when
calling qcrypto_secret_lookup_as_utf8.

Reported-by: Srikanth Aithal <bssrikanth@in.ibm.com>
Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 block/crypto.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/block/crypto.c b/block/crypto.c
index 7eb698774e..fe2f0c42b0 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -30,6 +30,7 @@
 #include "qapi/error.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
+#include "qemu/cutils.h"
 #include "crypto.h"
 
 typedef struct BlockCrypto BlockCrypto;
@@ -599,6 +600,27 @@ fail:
     bdrv_unref(bs);
     qapi_free_QCryptoBlockCreateOptions(create_opts);
     qobject_unref(cryptoopts);
+
+    /*
+     * If an error occurred, delete the file. Even if the file existed
+     * beforehand, it has been truncated and corrupted in the process.
+     */
+    if (ret) {
+        Error *local_err;
+        int r_del = bdrv_delete_file(filename, &local_err);
+        /*
+         * ENOTSUP will happen if the block driver doesn't support
+         * 'bdrv_co_delete_file'. ENOENT will be fired by
+         * 'raw_co_delete_file' if the file doesn't exist. Both are
+         * predictable (we're not verifying if the driver supports
+         * file deletion or if the file was created), thus we
+         * shouldn't report this back to the user.
+         */
+        if ((r_del < 0) && (r_del != -ENOTSUP) && (r_del != -ENOENT)) {
+            error_reportf_err(local_err, "%s: ", bs->filename);
+        }
+    }
+
     return ret;
 }
 
-- 
2.21.0

[Qemu-devel] [PATCH v6 4/4] qemu-iotests: adding LUKS cleanup for non-UTF8 secret error

[Daniel Henrique Barboza]

This patch adds a new test file, 257, to exercise the case where
qemu-img fails to complete for the LUKS format when a non-UTF8
secret is used.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 tests/qemu-iotests/259     | 67 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/259.out | 11 +++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 79 insertions(+)
 create mode 100755 tests/qemu-iotests/259
 create mode 100644 tests/qemu-iotests/259.out

diff --git a/tests/qemu-iotests/259 b/tests/qemu-iotests/259
new file mode 100755
index 0000000000..cb362598b4
--- /dev/null
+++ b/tests/qemu-iotests/259
@@ -0,0 +1,67 @@
+#!/usr/bin/env bash
+#
+# Test qemu-img file cleanup for LUKS when using a non-UTF8 secret
+#
+# Copyright (C) 2019, IBM Corporation.
+#
+# 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/>.
+#
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1	# failure is the default!
+TEST_IMAGE_FILE='vol.img'
+
+_cleanup()
+{
+  _cleanup_test_img
+  rm non_utf8_secret
+  rm -f $TEST_IMAGE_FILE
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt luks
+_supported_proto generic
+_unsupported_proto vxhs
+
+echo "== Create non-UTF8 secret =="
+echo -n -e '\x3a\x3c\x3b\xff' > non_utf8_secret
+SECRET="secret,id=sec0,file=non_utf8_secret"
+
+echo "== Throws an error because of invalid UTF-8 secret =="
+$QEMU_IMG create -f $IMGFMT --object $SECRET -o "key-secret=sec0" $TEST_IMAGE_FILE 4M
+
+echo "== Image file should not exist after the error =="
+if test -f "$TEST_IMAGE_FILE"; then
+    exit 1
+fi
+
+echo "== Create a stub image file and run qemu-img again =="
+touch $TEST_IMAGE_FILE
+$QEMU_IMG create -f $IMGFMT --object $SECRET -o "key-secret=sec0" $TEST_IMAGE_FILE 4M
+
+echo "== Pre-existing image file should also be deleted after the error =="
+if test -f "$TEST_IMAGE_FILE"; then
+    exit 1
+fi
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/259.out b/tests/qemu-iotests/259.out
new file mode 100644
index 0000000000..6b0188111c
--- /dev/null
+++ b/tests/qemu-iotests/259.out
@@ -0,0 +1,11 @@
+QA output created by 259
+== Create non-UTF8 secret ==
+== Throws an error because of invalid UTF-8 secret ==
+qemu-img: vol.img: Data from secret sec0 is not valid UTF-8
+Formatting 'vol.img', fmt=luks size=4194304 key-secret=sec0
+== Image file should not exist after the error ==
+== Create a stub image file and run qemu-img again ==
+qemu-img: vol.img: Data from secret sec0 is not valid UTF-8
+Formatting 'vol.img', fmt=luks size=4194304 key-secret=sec0
+== Pre-existing image file should also be deleted after the error ==
+ *** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index d95d556414..67c3faf80f 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -273,4 +273,5 @@
 256 rw quick
 257 rw
 258 rw quick
+259 rw auto quick
 262 rw quick migration
-- 
2.21.0

Re: [Qemu-devel] [PATCH v6 0/4] delete created files when block_crypto_co_create_opts_luks fails

[Daniel Henrique Barboza]

Please ignore this series. I'll respin a v7 to actually do what
Kevin suggested in the v4 review.

Thanks,


DHB

On 9/2/19 5:58 PM, Daniel Henrique Barboza wrote:
> Changes from previous version 5 [1] suggested by John Snow:
> - patch 2: return ENOMEDIUM with !drv, return negative error
> codes in bdrv_delete_file
> - patch 3: clarify the meaning of returning ENOENT in the
> comment
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg01173.html
>
>
> Daniel Henrique Barboza (4):
>    block: introducing 'bdrv_co_delete_file' interface
>    block.c: adding bdrv_delete_file
>    crypto.c: cleanup created file when block_crypto_co_create_opts_luks
>      fails
>    qemu-iotests: adding LUKS cleanup for non-UTF8 secret error
>
>   block.c                    | 78 ++++++++++++++++++++++++++++++++++++++
>   block/crypto.c             | 22 +++++++++++
>   block/file-posix.c         | 28 ++++++++++++++
>   include/block/block.h      |  1 +
>   include/block/block_int.h  |  6 +++
>   tests/qemu-iotests/259     | 67 ++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/259.out | 11 ++++++
>   tests/qemu-iotests/group   |  1 +
>   8 files changed, 214 insertions(+)
>   create mode 100755 tests/qemu-iotests/259
>   create mode 100644 tests/qemu-iotests/259.out
>