* [PATCH v8 1/4] block: introducing 'bdrv_co_delete_file' interface
2019-11-11 17:01 [PATCH v8 0/4] delete created files when block_crypto_co_create_opts_luks fails Daniel Henrique Barboza
@ 2019-11-11 17:01 ` Daniel Henrique Barboza
2019-11-11 17:01 ` [PATCH v8 2/4] block.c: adding bdrv_co_delete_file Daniel Henrique Barboza
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Daniel Henrique Barboza @ 2019-11-11 17:01 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, Daniel P . Berrangé, Daniel Henrique Barboza, jsnow
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 | 23 +++++++++++++++++++++++
include/block/block_int.h | 4 ++++
2 files changed, 27 insertions(+)
diff --git a/block/file-posix.c b/block/file-posix.c
index 1f0f61a02b..692a36a799 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2386,6 +2386,28 @@ static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts,
return raw_co_create(&options, errp);
}
+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)) {
+ error_setg_errno(errp, ENOENT, "%s is not a regular file",
+ bs->filename);
+ return -ENOENT;
+ }
+
+ ret = unlink(bs->filename);
+ if (ret < 0) {
+ ret = -errno;
+ error_setg_errno(errp, -ret, "Error when deleting file %s",
+ bs->filename);
+ }
+
+ return ret;
+}
+
/*
* Find allocation range in @bs around offset @start.
* May change underlying file descriptor's file offset.
@@ -3017,6 +3039,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 dd033d0b37..d938d3e8d2 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -314,6 +314,10 @@ struct BlockDriver {
*/
int coroutine_fn (*bdrv_co_flush)(BlockDriverState *bs);
+ /* Delete a 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v8 2/4] block.c: adding bdrv_co_delete_file
2019-11-11 17:01 [PATCH v8 0/4] delete created files when block_crypto_co_create_opts_luks fails Daniel Henrique Barboza
2019-11-11 17:01 ` [PATCH v8 1/4] block: introducing 'bdrv_co_delete_file' interface Daniel Henrique Barboza
@ 2019-11-11 17:01 ` Daniel Henrique Barboza
2019-11-11 17:01 ` [PATCH v8 3/4] crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails Daniel Henrique Barboza
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Daniel Henrique Barboza @ 2019-11-11 17:01 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, Daniel P . Berrangé, Daniel Henrique Barboza, jsnow
Using the new 'bdrv_co_delete_file' interface, a pure co_routine function
'bdrv_co_delete_file' inside block.c can can be used in a way similar of
the existing bdrv_create_file to to clean up a created file.
We're creating a pure co_routine because the only caller of
'bdrv_co_delete_file' will be already in co_routine context, thus there
is no need to add all the machinery to check for qemu_in_coroutine() and
create a separated co_routine to do the job.
Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
block.c | 26 ++++++++++++++++++++++++++
include/block/block.h | 1 +
2 files changed, 27 insertions(+)
diff --git a/block.c b/block.c
index 4cffc2bc35..c325104b8c 100644
--- a/block.c
+++ b/block.c
@@ -548,6 +548,32 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
return ret;
}
+int coroutine_fn bdrv_co_delete_file(BlockDriverState *bs, Error **errp)
+{
+ Error *local_err = NULL;
+ int ret;
+
+ assert(bs != NULL);
+
+ if (!bs->drv) {
+ error_setg(errp, "Block node '%s' is not opened", bs->filename);
+ return -ENOMEDIUM;
+ }
+
+ if (!bs->drv->bdrv_co_delete_file) {
+ error_setg(errp, "Driver '%s' does not support image deletion",
+ bs->drv->format_name);
+ return -ENOTSUP;
+ }
+
+ ret = bs->drv->bdrv_co_delete_file(bs, &local_err);
+ if (ret < 0) {
+ error_propagate(errp, local_err);
+ }
+
+ 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 1df9848e74..ec0d82f6b0 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -372,6 +372,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 coroutine_fn bdrv_co_delete_file(BlockDriverState *bs, Error **errp);
typedef struct BdrvCheckResult {
--
2.21.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v8 3/4] crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails
2019-11-11 17:01 [PATCH v8 0/4] delete created files when block_crypto_co_create_opts_luks fails Daniel Henrique Barboza
2019-11-11 17:01 ` [PATCH v8 1/4] block: introducing 'bdrv_co_delete_file' interface Daniel Henrique Barboza
2019-11-11 17:01 ` [PATCH v8 2/4] block.c: adding bdrv_co_delete_file Daniel Henrique Barboza
@ 2019-11-11 17:01 ` Daniel Henrique Barboza
2019-11-11 17:01 ` [PATCH v8 4/4] qemu-iotests: adding LUKS cleanup for non-UTF8 secret error Daniel Henrique Barboza
2019-12-18 15:31 ` [PATCH v8 0/4] delete created files when block_crypto_co_create_opts_luks fails Daniel Henrique Barboza
4 siblings, 0 replies; 6+ messages in thread
From: Daniel Henrique Barboza @ 2019-11-11 17:01 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, Srikanth Aithal, Daniel Henrique Barboza, jsnow
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 | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/block/crypto.c b/block/crypto.c
index 24823835c1..00e8ec537d 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;
@@ -596,6 +597,23 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename,
ret = 0;
fail:
+ /*
+ * If an error occurred, delete 'filename'. Even if the file existed
+ * beforehand, it has been truncated and corrupted in the process.
+ */
+ if (ret && bs) {
+ Error *local_delete_err = NULL;
+ int r_del = bdrv_co_delete_file(bs, &local_delete_err);
+ /*
+ * ENOTSUP will happen if the block driver doesn't support
+ * the 'bdrv_co_delete_file' interface. This is a predictable
+ * scenario and shouldn't be reported back to the user.
+ */
+ if ((r_del < 0) && (r_del != -ENOTSUP)) {
+ error_report_err(local_delete_err);
+ }
+ }
+
bdrv_unref(bs);
qapi_free_QCryptoBlockCreateOptions(create_opts);
qobject_unref(cryptoopts);
--
2.21.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v8 4/4] qemu-iotests: adding LUKS cleanup for non-UTF8 secret error
2019-11-11 17:01 [PATCH v8 0/4] delete created files when block_crypto_co_create_opts_luks fails Daniel Henrique Barboza
` (2 preceding siblings ...)
2019-11-11 17:01 ` [PATCH v8 3/4] crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails Daniel Henrique Barboza
@ 2019-11-11 17:01 ` Daniel Henrique Barboza
2019-12-18 15:31 ` [PATCH v8 0/4] delete created files when block_crypto_co_create_opts_luks fails Daniel Henrique Barboza
4 siblings, 0 replies; 6+ messages in thread
From: Daniel Henrique Barboza @ 2019-11-11 17:01 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, Daniel Henrique Barboza, jsnow
This patch adds a new test file 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/273 | 67 ++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/273.out | 11 +++++++
tests/qemu-iotests/group | 1 +
3 files changed, 79 insertions(+)
create mode 100755 tests/qemu-iotests/273
create mode 100644 tests/qemu-iotests/273.out
diff --git a/tests/qemu-iotests/273 b/tests/qemu-iotests/273
new file mode 100755
index 0000000000..cb362598b4
--- /dev/null
+++ b/tests/qemu-iotests/273
@@ -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/273.out b/tests/qemu-iotests/273.out
new file mode 100644
index 0000000000..8c6653cd82
--- /dev/null
+++ b/tests/qemu-iotests/273.out
@@ -0,0 +1,11 @@
+QA output created by 273
+== 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 065040398d..fc5a680739 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -284,3 +284,4 @@
268 rw auto quick
270 rw backing quick
272 rw
+273 rw img quick
\ No newline at end of file
--
2.21.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v8 0/4] delete created files when block_crypto_co_create_opts_luks fails
2019-11-11 17:01 [PATCH v8 0/4] delete created files when block_crypto_co_create_opts_luks fails Daniel Henrique Barboza
` (3 preceding siblings ...)
2019-11-11 17:01 ` [PATCH v8 4/4] qemu-iotests: adding LUKS cleanup for non-UTF8 secret error Daniel Henrique Barboza
@ 2019-12-18 15:31 ` Daniel Henrique Barboza
4 siblings, 0 replies; 6+ messages in thread
From: Daniel Henrique Barboza @ 2019-12-18 15:31 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, jsnow
Ping
On 11/11/19 2:01 PM, Daniel Henrique Barboza wrote:
> changes from previous version 7 [1], all suggested by Kevin Wolf:
>
> - patch 1:
> * removed function comment of raw_co_delete_file;
> * removed 'done' label from raw_co_delete_file;
> * removed 'local' remark from bdrv_co_delete_file comment. The comment
> is now single-lined;
> * added missing space in the commit msg;
> - patch 2:
> * ditched bdrv_delete_co_entry and bdrv_delete_file, now it's a single
> coroutine_fn bdrv_co_delete_file;
> * BlockDriverState != NULL dropped - the caller will need to ensure it
> is not null;
> * changed the error message of '!bs->drv' condition;
> * s/delete/deletion in the error message of !bs->drv->bdrv_co_delete_file;
> * 'out' label removed - function will return immediately on error;
> - patch 3:
> * check for (ret && bs);
> * drop the ENOENT verification;
> * do not prepend the filename in the error message;
> * removed an extra blank line.
>
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg00282.html
>
> Daniel Henrique Barboza (4):
> block: introducing 'bdrv_co_delete_file' interface
> block.c: adding bdrv_co_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 | 26 +++++++++++++++
> block/crypto.c | 18 ++++++++++
> block/file-posix.c | 23 +++++++++++++
> include/block/block.h | 1 +
> include/block/block_int.h | 4 +++
> tests/qemu-iotests/273 | 67 ++++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/273.out | 11 +++++++
> tests/qemu-iotests/group | 1 +
> 8 files changed, 151 insertions(+)
> create mode 100755 tests/qemu-iotests/273
> create mode 100644 tests/qemu-iotests/273.out
>
^ permalink raw reply [flat|nested] 6+ messages in thread