[Qemu-devel] [PATCH 0/2] qcow2: improve error handling when luks creation fails

[Qemu-devel] [PATCH 0/2] qcow2: improve error handling when luks creation fails

[Daniel P. Berrangé]

If qemu is built without crypto support luks creation will fail. The 188
iotest however still carried on using the partially created file. This
showed a few flaws in error handling, one during creation and one during
opening of the image.

Daniel P. Berrangé (2):
  qcow2: fail if encryption opts are provided to non-encrypted image
  qcow2: mark image as corrupt if failing during create

 block/qcow2.c | 9 +++++++++
 1 file changed, 9 insertions(+)

-- 
2.20.1

[Qemu-devel] [PATCH 1/2] qcow2: fail if encryption opts are provided to non-encrypted image

[Daniel P. Berrangé]

If the qcow2 image does not have any encryption method specified in its
header, the user should not be providing any encryption options when
opening it. We already detect this if the user had set "encrypt.format"
but this field is optional so must consider any "encrypt.*" option to be
an error.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 block/qcow2.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 65a54c9ac6..ecc577175f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1045,6 +1045,12 @@ static int qcow2_update_options_prepare(BlockDriverState *bs,
             ret = -EINVAL;
             goto fail;
         }
+        if (encryptopts && qdict_size(encryptopts)) {
+            error_setg(errp, "No encryption in image header, but encryption "
+                       "options provided");
+            ret = -EINVAL;
+            goto fail;
+        }
         break;
 
     case QCOW_CRYPT_AES:
-- 
2.20.1

Re: [Qemu-devel] [PATCH 1/2] qcow2: fail if encryption opts are provided to non-encrypted image

[Eric Blake]

On 2/19/19 6:50 AM, Daniel P. Berrangé wrote:
> If the qcow2 image does not have any encryption method specified in its
> header, the user should not be providing any encryption options when
> opening it. We already detect this if the user had set "encrypt.format"
> but this field is optional so must consider any "encrypt.*" option to be
> an error.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  block/qcow2.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 1/2] qcow2: fail if encryption opts are provided to non-encrypted image

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1191 bytes --]

On 19.02.19 13:50, Daniel P. Berrangé wrote:
> If the qcow2 image does not have any encryption method specified in its
> header, the user should not be providing any encryption options when
> opening it. We already detect this if the user had set "encrypt.format"
> but this field is optional so must consider any "encrypt.*" option to be
> an error.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  block/qcow2.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 65a54c9ac6..ecc577175f 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1045,6 +1045,12 @@ static int qcow2_update_options_prepare(BlockDriverState *bs,
>              ret = -EINVAL;
>              goto fail;
>          }
> +        if (encryptopts && qdict_size(encryptopts)) {
> +            error_setg(errp, "No encryption in image header, but encryption "
> +                       "options provided");
> +            ret = -EINVAL;
> +            goto fail;
> +        }

Doesn't this make the block right before this one a bit superfluous?

Max

>          break;
>  
>      case QCOW_CRYPT_AES:
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 1/2] qcow2: fail if encryption opts are provided to non-encrypted image

[Daniel P. Berrangé]

On Fri, Feb 22, 2019 at 08:17:40PM +0100, Max Reitz wrote:
> On 19.02.19 13:50, Daniel P. Berrangé wrote:
> > If the qcow2 image does not have any encryption method specified in its
> > header, the user should not be providing any encryption options when
> > opening it. We already detect this if the user had set "encrypt.format"
> > but this field is optional so must consider any "encrypt.*" option to be
> > an error.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  block/qcow2.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 65a54c9ac6..ecc577175f 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -1045,6 +1045,12 @@ static int qcow2_update_options_prepare(BlockDriverState *bs,
> >              ret = -EINVAL;
> >              goto fail;
> >          }
> > +        if (encryptopts && qdict_size(encryptopts)) {
> > +            error_setg(errp, "No encryption in image header, but encryption "
> > +                       "options provided");
> > +            ret = -EINVAL;
> > +            goto fail;
> > +        }
> 
> Doesn't this make the block right before this one a bit superfluous?

Yes, in the sense that we'll still get an error if we removed the
prior block. The prior block has a more useful error message which
will help diagnosis though, so I thought it worth keeping both.

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 :|

[Qemu-devel] [PATCH 2/2] qcow2: mark image as corrupt if failing during create

[Daniel P. Berrangé]

During creation we write a minimal qcow2 header and then update it with
extra features. If the updating fails for some reason we might still be
left with a valid qcow2 image that will be mistakenly used for I/O. We
cannot delete the image, since we don't know if we created the
underlying storage or not. Thus we mark the header as corrupt to
prevents its later usage.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 block/qcow2.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index ecc577175f..338513e652 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3104,6 +3104,9 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
 
     ret = 0;
 out:
+    if (ret < 0) {
+        qcow2_mark_corrupt(blk_bs(blk));
+    }
     blk_unref(blk);
     bdrv_unref(bs);
     return ret;
-- 
2.20.1

Re: [Qemu-devel] [PATCH 2/2] qcow2: mark image as corrupt if failing during create

[Eric Blake]

On 2/19/19 6:50 AM, Daniel P. Berrangé wrote:
> During creation we write a minimal qcow2 header and then update it with
> extra features. If the updating fails for some reason we might still be
> left with a valid qcow2 image that will be mistakenly used for I/O. We
> cannot delete the image, since we don't know if we created the
> underlying storage or not. Thus we mark the header as corrupt to
> prevents its later usage.

Should we unconditionally mark the image as corrupt at the time we write
the minimal qcow2 header, and then update the image to non-corrupt on
the final update?

> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  block/qcow2.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index ecc577175f..338513e652 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3104,6 +3104,9 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
>  
>      ret = 0;
>  out:
> +    if (ret < 0) {
> +        qcow2_mark_corrupt(blk_bs(blk));
> +    }

If ret < 0 because of an EIO error, this may also fail to write the
change to the header. Hence my question as to whether this is too late.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 2/2] qcow2: mark image as corrupt if failing during create

[Daniel P. Berrangé]

On Tue, Feb 19, 2019 at 10:11:58AM -0600, Eric Blake wrote:
> On 2/19/19 6:50 AM, Daniel P. Berrangé wrote:
> > During creation we write a minimal qcow2 header and then update it with
> > extra features. If the updating fails for some reason we might still be
> > left with a valid qcow2 image that will be mistakenly used for I/O. We
> > cannot delete the image, since we don't know if we created the
> > underlying storage or not. Thus we mark the header as corrupt to
> > prevents its later usage.
> 
> Should we unconditionally mark the image as corrupt at the time we write
> the minimal qcow2 header, and then update the image to non-corrupt on
> the final update?

That's a nice idea, but we call blk_new_open() half way through to
qcow2_co_create method to open the minimal image. If we mark it
corrupt upfront we'll never be able to open this minimal image.

Adding a flag to allow blk_new_open to ignore the "corrupt" marker
feels unplesant to me.

> 
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  block/qcow2.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index ecc577175f..338513e652 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -3104,6 +3104,9 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
> >  
> >      ret = 0;
> >  out:
> > +    if (ret < 0) {
> > +        qcow2_mark_corrupt(blk_bs(blk));
> > +    }
> 
> If ret < 0 because of an EIO error, this may also fail to write the
> change to the header. Hence my question as to whether this is too late.

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 2/2] qcow2: mark image as corrupt if failing during create

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2269 bytes --]

On 19.02.19 13:50, Daniel P. Berrangé wrote:
> During creation we write a minimal qcow2 header and then update it with
> extra features. If the updating fails for some reason we might still be
> left with a valid qcow2 image that will be mistakenly used for I/O. We
> cannot delete the image, since we don't know if we created the
> underlying storage or not. Thus we mark the header as corrupt to
> prevents its later usage.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  block/qcow2.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index ecc577175f..338513e652 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3104,6 +3104,9 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
>  
>      ret = 0;
>  out:
> +    if (ret < 0) {
> +        qcow2_mark_corrupt(blk_bs(blk));

First, blk_bs(blk) may be the qcow2 BDS, or it may the protocol BDS here
(it is the latter before the first blk_new_open() call).  Calling
qcow2_mark_corrupt() unconditionally may mean an invalid access to
bs->opaque (which is not going to be of type BDRVQcow2State if the BDS
is not a qcow2 BDS).

Second, blk may be NULL (at various points, e.g. after blk_new_open()
failed).  Then this would yield a segfault in in blk_bs().

Third, blk_bs(blk) may be NULL (if blk_insert_bs() failed).  Then this
would yield a segfault in qcow2_mark_corrupt().

On a minor note, it is rather probably that blk_new_open() fails.  In
that case, there is currently no way to mark the image corrupt.  Would
it be useful and possible to have a function to mark a qcow2 image
corrupt without relying on qcow2 features, i.e. by writing directly to
the protocol layer (which is always @bs)?  This would be unsafe to use
as long as the protocol layer is opened by the qcow2 driver in some
other node, but we could invoke this function safely after @blk has been
freed.


Or maybe Eric's suggestion really is for the best, i.e. mark the image
corrupt from the start and then clean that after we're all done.  You
don't need a new flag for that, we already have BDRV_O_CHECK.

Max

> +    }
>      blk_unref(blk);
>      bdrv_unref(bs);
>      return ret;
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 2/2] qcow2: mark image as corrupt if failing during create

[Daniel P. Berrangé]

On Fri, Feb 22, 2019 at 08:21:26PM +0100, Max Reitz wrote:
> On 19.02.19 13:50, Daniel P. Berrangé wrote:
> > During creation we write a minimal qcow2 header and then update it with
> > extra features. If the updating fails for some reason we might still be
> > left with a valid qcow2 image that will be mistakenly used for I/O. We
> > cannot delete the image, since we don't know if we created the
> > underlying storage or not. Thus we mark the header as corrupt to
> > prevents its later usage.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  block/qcow2.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index ecc577175f..338513e652 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -3104,6 +3104,9 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
> >  
> >      ret = 0;
> >  out:
> > +    if (ret < 0) {
> > +        qcow2_mark_corrupt(blk_bs(blk));

...snip...

> Or maybe Eric's suggestion really is for the best, i.e. mark the image
> corrupt from the start and then clean that after we're all done.  You
> don't need a new flag for that, we already have BDRV_O_CHECK.

Ah, I didn't realize that is what BDRV_O_CHECK could do. I'll try this
approach as it is nicer.


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 :|