Re: [Qemu-devel] [PATCH v3 1/3] block/qcow2: refactoring of threaded encryption code

[Maxim Levitsky]

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 <address@hidden>
> > ---
> >  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