Message ID | 20190319220330.3380-1-danielhb413@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [1/1] block_crypto_co_create_generic: clean up created file on failure | expand |
On Tue, Mar 19, 2019 at 07:03:30PM -0300, Daniel Henrique Barboza wrote: > 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 fixes it by adding a flag to indicate whether bdrv_create_file() > created the file, and then unlinking it in the 'fail' label if > necessary. > > * 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> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > --- > block/crypto.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/block/crypto.c b/block/crypto.c > index fd8c7cfac6..5b43424845 100644 > --- a/block/crypto.c > +++ b/block/crypto.c > @@ -533,6 +533,7 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, > BlockDriverState *bs = NULL; > QDict *cryptoopts; > int64_t size; > + bool filecreated = false; > int ret; > > /* Parse options */ > @@ -554,6 +555,7 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, > if (ret < 0) { > goto fail; > } > + filecreated = true; > > bs = bdrv_open(filename, NULL, NULL, > BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp); > @@ -569,7 +571,14 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, > } > > ret = 0; > + goto cleanup; > + > fail: > + if (filecreated) { > + unlink(filename); You can't do this I'm afraid. "filename" cannot be assumed to be a local file - it can be an arbitrary block device backend string. Contrary to what you might expect, given the call to bdrv_create_file, we can't actually assume this method created the file either. The above bdrv_create_file method may have merely formatted a pre-existing volume. To fix this requires a "bdrv_delete" callback in the BlockDriver code, and the actual deletion would need to be done in the "qemu-img create" code. Regards, Daniel
On 3/20/19 6:52 AM, Daniel P. Berrangé wrote: > On Tue, Mar 19, 2019 at 07:03:30PM -0300, Daniel Henrique Barboza wrote: >> 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 fixes it by adding a flag to indicate whether bdrv_create_file() >> created the file, and then unlinking it in the 'fail' label if >> necessary. >> >> * 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> >> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >> --- >> block/crypto.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/block/crypto.c b/block/crypto.c >> index fd8c7cfac6..5b43424845 100644 >> --- a/block/crypto.c >> +++ b/block/crypto.c >> @@ -533,6 +533,7 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, >> BlockDriverState *bs = NULL; >> QDict *cryptoopts; >> int64_t size; >> + bool filecreated = false; >> int ret; >> >> /* Parse options */ >> @@ -554,6 +555,7 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, >> if (ret < 0) { >> goto fail; >> } >> + filecreated = true; >> >> bs = bdrv_open(filename, NULL, NULL, >> BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp); >> @@ -569,7 +571,14 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, >> } >> >> ret = 0; >> + goto cleanup; >> + >> fail: >> + if (filecreated) { >> + unlink(filename); > You can't do this I'm afraid. > > "filename" cannot be assumed to be a local file - it can be an arbitrary > block device backend string. > > Contrary to what you might expect, given the call to bdrv_create_file, > we can't actually assume this method created the file either. The above > bdrv_create_file method may have merely formatted a pre-existing volume. > > To fix this requires a "bdrv_delete" callback in the BlockDriver code, > and the actual deletion would need to be done in the "qemu-img create" > code. Thanks for the pointers. I'll see if I can make this happen in v2. DHB > > Regards, > Daniel
diff --git a/block/crypto.c b/block/crypto.c index fd8c7cfac6..5b43424845 100644 --- a/block/crypto.c +++ b/block/crypto.c @@ -533,6 +533,7 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, BlockDriverState *bs = NULL; QDict *cryptoopts; int64_t size; + bool filecreated = false; int ret; /* Parse options */ @@ -554,6 +555,7 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, if (ret < 0) { goto fail; } + filecreated = true; bs = bdrv_open(filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp); @@ -569,7 +571,14 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, } ret = 0; + goto cleanup; + fail: + if (filecreated) { + unlink(filename); + } + +cleanup: bdrv_unref(bs); qapi_free_QCryptoBlockCreateOptions(create_opts); qobject_unref(cryptoopts);
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 fixes it by adding a flag to indicate whether bdrv_create_file() created the file, and then unlinking it in the 'fail' label if necessary. * 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> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- block/crypto.c | 9 +++++++++ 1 file changed, 9 insertions(+)