diff mbox series

[v5,7/9] block: truncate: Don't make backing file data visible

Message ID 20200422152129.167074-8-kwolf@redhat.com
State New, archived
Headers show
Series block: Fix resize (extending) of short overlays | expand

Commit Message

Kevin Wolf April 22, 2020, 3:21 p.m. UTC
When extending the size of an image that has a backing file larger than
its old size, make sure that the backing file data doesn't become
visible in the guest, but the added area is properly zeroed out.

Consider the following scenario where the overlay is shorter than its
backing file:

    base.qcow2:     AAAAAAAA
    overlay.qcow2:  BBBB

When resizing (extending) overlay.qcow2, the new blocks should not stay
unallocated and make the additional As from base.qcow2 visible like
before this patch, but zeros should be read.

A similar case happens with the various variants of a commit job when an
intermediate file is short (- for unallocated):

    base.qcow2:     A-A-AAAA
    mid.qcow2:      BB-B
    top.qcow2:      C--C--C-

After commit top.qcow2 to mid.qcow2, the following happens:

    mid.qcow2:      CB-C00C0 (correct result)
    mid.qcow2:      CB-C--C- (before this fix)

Without the fix, blocks that previously read as zeros on top.qcow2
suddenly turn into A.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block/io.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Max Reitz April 23, 2020, 11:14 a.m. UTC | #1
On 22.04.20 17:21, Kevin Wolf wrote:
> When extending the size of an image that has a backing file larger than
> its old size, make sure that the backing file data doesn't become
> visible in the guest, but the added area is properly zeroed out.
> 
> Consider the following scenario where the overlay is shorter than its
> backing file:
> 
>     base.qcow2:     AAAAAAAA
>     overlay.qcow2:  BBBB
> 
> When resizing (extending) overlay.qcow2, the new blocks should not stay
> unallocated and make the additional As from base.qcow2 visible like
> before this patch, but zeros should be read.
> 
> A similar case happens with the various variants of a commit job when an
> intermediate file is short (- for unallocated):
> 
>     base.qcow2:     A-A-AAAA
>     mid.qcow2:      BB-B
>     top.qcow2:      C--C--C-
> 
> After commit top.qcow2 to mid.qcow2, the following happens:
> 
>     mid.qcow2:      CB-C00C0 (correct result)
>     mid.qcow2:      CB-C--C- (before this fix)
> 
> Without the fix, blocks that previously read as zeros on top.qcow2
> suddenly turn into A.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/io.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/block/io.c b/block/io.c
> index 795075954e..8fbb607515 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -3394,6 +3394,20 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
>          goto out;
>      }
>  
> +    /*
> +     * If the image has a backing file that is large enough that it would
> +     * provide data for the new area, we cannot leave it unallocated because
> +     * then the backing file content would become visible. Instead, zero-fill
> +     * the new area.
> +     *
> +     * Note that if the image has a backing file, but was opened without the
> +     * backing file, taking care of keeping things consistent with that backing
> +     * file is the user's responsibility.
> +     */
> +    if (new_bytes && bs->backing) {
> +        flags |= BDRV_REQ_ZERO_WRITE;
> +    }

This breaks growing any non-qcow2 image with any backing file.  Do we
care about that?

The comment says something about “a backing file that is large enough
that it would provide data for the new area”, but that condition doesn’t
appear in the code.  Should it?  (If it did, I think the number of cases
this change broke would be much smaller.)

If it was deliberate to not have that condition here, and if we decide
that we don’t care about non-qcow2 formats here, then I think at least
the error message deserves some improvement over “qemu-img: Block driver
does not support requested flags”.

Max
Kevin Wolf April 23, 2020, 1 p.m. UTC | #2
Am 23.04.2020 um 13:14 hat Max Reitz geschrieben:
> On 22.04.20 17:21, Kevin Wolf wrote:
> > When extending the size of an image that has a backing file larger than
> > its old size, make sure that the backing file data doesn't become
> > visible in the guest, but the added area is properly zeroed out.
> > 
> > Consider the following scenario where the overlay is shorter than its
> > backing file:
> > 
> >     base.qcow2:     AAAAAAAA
> >     overlay.qcow2:  BBBB
> > 
> > When resizing (extending) overlay.qcow2, the new blocks should not stay
> > unallocated and make the additional As from base.qcow2 visible like
> > before this patch, but zeros should be read.
> > 
> > A similar case happens with the various variants of a commit job when an
> > intermediate file is short (- for unallocated):
> > 
> >     base.qcow2:     A-A-AAAA
> >     mid.qcow2:      BB-B
> >     top.qcow2:      C--C--C-
> > 
> > After commit top.qcow2 to mid.qcow2, the following happens:
> > 
> >     mid.qcow2:      CB-C00C0 (correct result)
> >     mid.qcow2:      CB-C--C- (before this fix)
> > 
> > Without the fix, blocks that previously read as zeros on top.qcow2
> > suddenly turn into A.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > Reviewed-by: Alberto Garcia <berto@igalia.com>
> > ---
> >  block/io.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/block/io.c b/block/io.c
> > index 795075954e..8fbb607515 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -3394,6 +3394,20 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
> >          goto out;
> >      }
> >  
> > +    /*
> > +     * If the image has a backing file that is large enough that it would
> > +     * provide data for the new area, we cannot leave it unallocated because
> > +     * then the backing file content would become visible. Instead, zero-fill
> > +     * the new area.
> > +     *
> > +     * Note that if the image has a backing file, but was opened without the
> > +     * backing file, taking care of keeping things consistent with that backing
> > +     * file is the user's responsibility.
> > +     */
> > +    if (new_bytes && bs->backing) {
> > +        flags |= BDRV_REQ_ZERO_WRITE;
> > +    }
> 
> This breaks growing any non-qcow2 image with any backing file.  Do we
> care about that?
> 
> The comment says something about “a backing file that is large enough
> that it would provide data for the new area”, but that condition doesn’t
> appear in the code.  Should it?  (If it did, I think the number of cases
> this change broke would be much smaller.)
> 
> If it was deliberate to not have that condition here, and if we decide
> that we don’t care about non-qcow2 formats here, then I think at least
> the error message deserves some improvement over “qemu-img: Block driver
> does not support requested flags”.

This was not deliberate. v3 had the check and I'm not sure why I removed
it. Probably because the new approach felt so much simpler and I was
glad that I could throw away complicated code that I threw away more
than I should have...

Kevin
diff mbox series

Patch

diff --git a/block/io.c b/block/io.c
index 795075954e..8fbb607515 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3394,6 +3394,20 @@  int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
         goto out;
     }
 
+    /*
+     * If the image has a backing file that is large enough that it would
+     * provide data for the new area, we cannot leave it unallocated because
+     * then the backing file content would become visible. Instead, zero-fill
+     * the new area.
+     *
+     * Note that if the image has a backing file, but was opened without the
+     * backing file, taking care of keeping things consistent with that backing
+     * file is the user's responsibility.
+     */
+    if (new_bytes && bs->backing) {
+        flags |= BDRV_REQ_ZERO_WRITE;
+    }
+
     if (drv->bdrv_co_truncate) {
         if (flags & ~bs->supported_truncate_flags) {
             error_setg(errp, "Block driver does not support requested flags");