On Fri, 2019-09-13 at 12:37 +0200, Max Reitz wrote:
> On 13.09.19 00:37, Maxim Levitsky wrote:
> > This commit tries to clarify few function arguments,
> > and add comments describing the encrypt/decrypt interface
> >
> > Signed-off-by: Maxim Levitsky <mlevi...@redhat.com>
> > ---
> > block/qcow2-cluster.c | 8 +++---
> > block/qcow2-threads.c | 63 ++++++++++++++++++++++++++++++++++---------
> > 2 files changed, 54 insertions(+), 17 deletions(-)
> >
> > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > index f09cc992af..b95e64c237 100644
> > --- a/block/qcow2-cluster.c
> > +++ b/block/qcow2-cluster.c
> > @@ -463,8 +463,8 @@ static int coroutine_fn
> > do_perform_cow_read(BlockDriverState *bs,
> > }
> >
> > static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
> > - uint64_t
> > src_cluster_offset,
> > - uint64_t cluster_offset,
> > + uint64_t
> > guest_cluster_offset,
> > + uint64_t
> > host_cluster_offset,
> > unsigned offset_in_cluster,
> > uint8_t *buffer,
> > unsigned bytes)
> > @@ -474,8 +474,8 @@ static bool coroutine_fn
> > do_perform_cow_encrypt(BlockDriverState *bs,
> > assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
> > assert((bytes & ~BDRV_SECTOR_MASK) == 0);
> > assert(s->crypto);
> > - if (qcow2_co_encrypt(bs, cluster_offset,
> > - src_cluster_offset + offset_in_cluster,
> > + if (qcow2_co_encrypt(bs, host_cluster_offset,
> > + guest_cluster_offset + offset_in_cluster,
> > buffer, bytes) < 0) {
> > return false;
> > }
> > diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
> > index 3b1e63fe41..6da1838e95 100644
> > --- a/block/qcow2-threads.c
> > +++ b/block/qcow2-threads.c
> > @@ -234,15 +234,19 @@ static int qcow2_encdec_pool_func(void *opaque)
> > }
> >
> > static int coroutine_fn
> > -qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
> > - uint64_t offset, void *buf, size_t len, Qcow2EncDecFunc
> > func)
> > +qcow2_co_encdec(BlockDriverState *bs, uint64_t host_cluster_offset,
> > + uint64_t guest_offset, void *buf, size_t len,
> > + Qcow2EncDecFunc func)
> > {
> > BDRVQcow2State *s = bs->opaque;
> > +
> > + uint64_t offset = s->crypt_physical_offset ?
> > + host_cluster_offset + offset_into_cluster(s, guest_offset) :
> > + guest_offset;
> > +
> > Qcow2EncDecData arg = {
> > .block = s->crypto,
> > - .offset = s->crypt_physical_offset ?
> > - file_cluster_offset + offset_into_cluster(s, offset)
> > :
> > - offset,
> > + .offset = offset,
> > .buf = buf,
> > .len = len,
> > .func = func,
> > @@ -251,18 +255,51 @@ qcow2_co_encdec(BlockDriverState *bs, uint64_t
> > file_cluster_offset,
> > return qcow2_co_process(bs, qcow2_encdec_pool_func, &arg);
> > }
> >
> > +
> > +/*
> > + * qcow2_co_encrypt()
> > + *
> > + * Encrypts one or more contiguous aligned sectors
> > + *
> > + * @host_cluster_offset - underlying storage offset of the first cluster
> > + * in which the encrypted data will be written
> > + * Used as an initialization vector for encryption
>
> s/as an/for generating the/
>
> (AFAIU)
Yes, this is better.
>
> > + *
> > + * @guest_offset - guest (virtual) offset of the first sector of the
> > + * data to be encrypted
> > + * Used as an initialization vector for older, qcow2 native encryption
>
> I wouldn’t be so specific here. I think it’d be better to just have a
> common sentence like “Depending on the encryption method, either of
> these offsets may be used for generating the initialization vector for
> encryption.”
Nothing against this either.
>
> Well, technically, host_cluster_offset will not be used itself. Before
> we can use it, of course we have to add the in-cluster offset to it
> (which qcow2_co_encdec() does).
>
> This makes me wonder whether it wouldn’t make more sense to pass a
> host_offset instead of a host_cluster_offset (i.e. make the callers add
> the in-cluster offset to the host offset already)?
This is what I wanted to do in first place, and it would be the cleanest
solution, but that would need to update the 3rd caller of this function,
the qcow2_co_pwritev_part, which does pass the cluster offset and guest full
offset.
You know what, I'll just do it. A bit more changes, but much cleaner code that
eliminates the possibility of this bug of happening again.
>
> > + *
> > + * @buf - buffer with the data to encrypt, that after encryption
> > + * will be written to the underlying storage device at
> > + * @host_cluster_offset
>
> I think just “buffer with the data to encrypt” is sufficient. (The rest
> is just the same as above.)
I wrote it to clarify this since Vladimir told me that its not clear enough.
Note though that I made a mistake here since the data will be written not at
host_cluster_offset but in host_cluster_offset + offset_into_cluster(s,
guest_offset
> + * @len - length of the buffer (in sector size multiplies)
>
> “In sector size multiples” to me means that it is a sector count (in
> that “one sector size multiple” is equal to 512 byes).
>
> Maybe “must be a BDRV_SECTOR_SIZE multiple” instead?
All right.
>
> > + *
> > + * Note that the group of the sectors, don't have to be aligned
> > + * on cluster boundary and can also cross a cluster boundary.
>
> Maybe “Note that while the whole range must be aligned on sectors, it
> does not have to be aligned on clusters and can also cross cluster
> boundaries”?
>
> (“The group of sectors” sounds a bit weird to me. I don’t quite know,
> why. I think that for some reason it makes me think of a non-continuous
> set of sectors. Also, the caller doesn’t pass sector indices, but byte
> offsets, that just happen to have to be aligned to sectors. (I suppose
> because that’s the simplest way to make it a multiple of the encryption
> block size.))
All right, this sounds better
>
> > + *
> > + */
> > int coroutine_fn
> > -qcow2_co_encrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
> > - uint64_t offset, void *buf, size_t len)
> > +qcow2_co_encrypt(BlockDriverState *bs, uint64_t host_cluster_offset,
> > + uint64_t guest_offset, void *buf, size_t len)
> > {
> > - return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
> > - qcrypto_block_encrypt);
> > + return qcow2_co_encdec(bs, host_cluster_offset, guest_offset, buf, len,
> > + qcrypto_block_encrypt);
> > }
> >
> > +
> > +/*
> > + * qcow2_co_decrypt()
> > + *
> > + * Decrypts one or more contiguous aligned sectors
> > + * Similar to qcow2_co_encrypt
>
> Hm. This would make me wonder in what way it is only similar to
> qcow2_co_encrypt(). Sure, it decrypts instead of encrypting, but maybe
> there’s more...?
I don't think so, since we have symmetrical encryption here.
>
> So maybe “Its interface is the same as qcow2_co_encrypt()'s”?
That would be lawyer territory, since interface is not technically the same,
since it decrypts rather that encrypts the data in the buffer...
I think that this wording should be good enough.
>
> Max
>
> > + *
> > + */
> > +
> > int coroutine_fn
> > -qcow2_co_decrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
> > - uint64_t offset, void *buf, size_t len)
> > +qcow2_co_decrypt(BlockDriverState *bs, uint64_t host_cluster_offset,
> > + uint64_t guest_offset, void *buf, size_t len)
> > {
> > - return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
> > - qcrypto_block_decrypt);
> > + return qcow2_co_encdec(bs, host_cluster_offset, guest_offset, buf, len,
> > + qcrypto_block_decrypt);
> > }
> >
>
>
Best regards,
Maxim Levitsky
