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

[Eric Blake <eblake@redhat.com>]

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 <kwolf@redhat.com>
>>> ---
>>>    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