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

[Eric Blake]

On 9/6/19 12:31 PM, 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 | 53 ++++++++++++++++++++++++++++++++++---------
>  2 files changed, 46 insertions(+), 15 deletions(-)
> 

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

Pre-existing bug in alignment...

> +qcow2_co_encdec(BlockDriverState *bs, uint64_t host_cluster_offset,
> +                  uint64_t guest_offset, void *buf, size_t len,
> +                  Qcow2EncDecFunc func)

...so this would be a great time to fix it.

>  {
>      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,

I'm ambivalent on whether the new 'offset' variable gains us anything.
But it doesn't hurt.


>          .buf = buf,
>          .len = len,
>          .func = func,
> @@ -251,18 +255,45 @@ qcow2_co_encdec(BlockDriverState *bs, uint64_t 
> file_cluster_offset,
>      return qcow2_co_process(bs, qcow2_encdec_pool_func, &arg);
>  }
>  
> +
> +/*
> + * qcow2_co_encrypt()
> + *
> + * Encrypts a sector size aligned contiguous area
> + *
> + * @host_cluster_offset - on disk offset of the cluster in which
> + *                        the buffer resides
> + *
> + * @guest_offset - guest (virtual) offset of the buffer
> + * @buf - buffer with the data to encrypt
> + * @len - length of the buffer
> + *
> + * Note that the area is not cluster aligned and might cross a cluster
> + * boundary

Umm, how is it possible for a sector to cross a cluster boundary?  All
clusters are sector-aligned, and encryption only works on aligned
sectors.  Oh, I see - if @len is a multiple larger than sector size,
then we have multiple sectors, and then indeed we may cross clusters.
But then the docs about being 'a sector size aligned contiguous area' is
not quite right.

> + *
> + *
> + */
>  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,
> +    return qcow2_co_encdec(bs, host_cluster_offset, guest_offset, buf, len,
>                               qcrypto_block_encrypt);


Another alignment worth fixing up while in the area.

>  }
>  
> +
> +/*
> + * qcow2_co_decrypt()
> + *
> + * Decrypts a sector size aligned contiguous area
> + * Same function as qcow2_co_encrypt
> + *
> + */
> +
>  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,
> +    return qcow2_co_encdec(bs, host_cluster_offset, guest_offset, buf, len,
>                               qcrypto_block_decrypt);

and again.

>  }
> 

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

signature.asc
Description: OpenPGP digital signature