Re: [PATCH v7 04/10] qcow2: Support BDRV_REQ_ZERO_WRITE for truncate

[Eric Blake]

On 4/28/20 1:45 PM, Kevin Wolf wrote:
> Am 28.04.2020 um 18:28 hat Eric Blake geschrieben:
> > On 4/24/20 7:54 AM, Kevin Wolf wrote:
> > > If BDRV_REQ_ZERO_WRITE is set and we're extending the image, calling
> > > qcow2_cluster_zeroize() with flags=0 does the right thing: It doesn't
> > > undo any previous preallocation, but just adds the zero flag to all
> > > relevant L2 entries. If an external data file is in use, a write_zeroes
> > > request to the data file is made instead.
> > >
> > > Signed-off-by: Kevin Wolf <address@hidden>
> > > ---
> > >    block/qcow2-cluster.c |  2 +-
> > >    block/qcow2.c         | 34 ++++++++++++++++++++++++++++++++++
> > >    2 files changed, 35 insertions(+), 1 deletion(-)
> >
> > > +++ b/block/qcow2.c
> > > @@ -1726,6 +1726,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
> > > *bs, QDict *options,
> > >        bs->supported_zero_flags = header.version >= 3 ?
> > >                                   BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK : 0;
> > > +    bs->supported_truncate_flags = BDRV_REQ_ZERO_WRITE;
> >
> > Is this really what we want for encrypted files, or would it be better as:
> >
> >      if (bs->encrypted) {
> >          bs->supported_truncate_flags = 0;
> >      } else {
> >          bs->supported_truncate_flags = BDRV_REQ_ZERO_WRITE;
> >      }
> >
> > At the qcow2 level, we can guarantee a read of 0 even for an encrypted
> > image, but is that really what we want?  Is setting the qcow2 zero flag on
> > the cluster done at the decrypted level (at which point we may be leaking
> > information about guest contents via anyone that can read the qcow2
> > metadata) or at the encrypted level (at which point it's useless
> > information, because knowing the underlying file reads as zero still
> > decrypts into garbage)?
>
> The zero flag means that the guest reads zeros, even with encrypted
> files. I'm not sure if it's worse than exposing the information which
> clusters are allocated and which are unallocated, which we have always
> been doing and which is hard to avoid without encrypting all the
> metadata, too. But it does reveal some information.
>
> If we think that exposing zero flags is worse than exposing the
> allocation status, I would still not use your solution above. In that
> case, the full fix would be returning -ENOTSUP from
> .bdrv_co_pwrite_zeroes() to cover all other callers, too.

Indeed, it also makes me wonder if we should support 
truncate(BDRV_REQ_ZERO_WRITE|BDRV_REQ_NO_FALLBACK), to differentiate 
whether a truncation request is aiming more to be fast (NO_FALLBACK set, 
fail immediately with -ENOTSUP on encryption) or complete (NO_FALLBACK 
clear, go ahead and write guest-visible zeroes, which populates the 
format layer).  In other words, maybe we want a knob that the user can 
set on encrypted volumes on whether to allow zero flags in the qcow2 image.

> If we think that allocation status and zero flags are of comparable
> importance, then we need to fix either both or nothing. Hiding all of
> this information probably means encrypting at least the L2 tables and
> potentially all of the metadata apart from the header. This would
> obviously require an incompatible feature flag (and some effort to
> implement it).

Indeed, my question is broad enough that it does not hold up _this_ 
series, so much as providing food for thought on what else we may need 
to add for encrypted qcow2 images as a future series, to make it easier 
to adjust the slider between the extremes of performance vs. minimal 
data leaks when using encryption.

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