Re: [PATCH v6 07/10] block: truncate: Don't make backing file data visible

[Max Reitz]

On 23.04.20 17:01, 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 <address@hidden>
> Reviewed-by: Alberto Garcia <address@hidden>

Seems like Berto gave you a rather broad R-b in v4. :)

> ---
>  block/io.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/block/io.c b/block/io.c
> index 795075954e..f618db3499 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -3394,6 +3394,30 @@ 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) {
> +        int64_t backing_len;
> +
> +        backing_len = bdrv_getlength(backing_bs(bs));
> +        if (backing_len < 0) {
> +            ret = backing_len;

Shouldn’t we set errp?

Max

> +            goto out;
> +        }
> +
> +        if (backing_len > old_size) {
> +            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");
>

signature.asc
Description: OpenPGP digital signature