[2/2] qcow2: mark image as corrupt if failing during create
diff mbox series

Message ID 20190219125044.5416-3-berrange@redhat.com
State New
Headers show
Series
  • qcow2: improve error handling when luks creation fails
Related show

Commit Message

Daniel P. Berrangé Feb. 19, 2019, 12:50 p.m. UTC
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(+)

Comments

Eric Blake Feb. 19, 2019, 4:11 p.m. UTC | #1
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.
Daniel P. Berrangé Feb. 19, 2019, 4:19 p.m. UTC | #2
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
Max Reitz Feb. 22, 2019, 7:21 p.m. UTC | #3
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;
>
Daniel P. Berrangé Feb. 25, 2019, 10:40 a.m. UTC | #4
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

Patch
diff mbox series

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;