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

[Max Reitz]

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)

> + *
> + * @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.”

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)?

> + *
> + * @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.)

> + *
> + * @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?

> + *
> + * 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.))

> + *
> + */
>  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...?

So maybe “Its interface is the same as qcow2_co_encrypt()'s”?

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);
>  }
>

signature.asc
Description: OpenPGP digital signature