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

[Vladimir Sementsov-Ogievskiy]

06.09.2019 22:57, 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 | 10 +++----
>   block/qcow2-threads.c | 61 ++++++++++++++++++++++++++++++++++---------
>   2 files changed, 53 insertions(+), 18 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index f09cc992af..1989b423da 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;
>           }
> @@ -496,7 +496,7 @@ static int coroutine_fn 
> do_perform_cow_write(BlockDriverState *bs,
>       }
>   
>       ret = qcow2_pre_write_overlap_check(bs, 0,
> -            cluster_offset + offset_in_cluster, qiov->size, true);
> +              cluster_offset + offset_in_cluster, qiov->size, true);


Hmm, unrelated hunk.

>       if (ret < 0) {
>           return ret;
>       }
> diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
> index 3b1e63fe41..c3cda0c6a5 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,49 @@ 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 - on disk offset of the first cluster in which
> + * the encrypted data will be written


It's not quite right, it's not on disk, but on .file child of qcow2 node, which
may be any other format or protocol node.. So, I called it file_cluster_offset.
But I'm OK with new naming anyway. And it may be better for encryption related
logic..

 > + * Used as an initialization vector for encryption

Hmm, is it default now?

> + *
> + * @guest_offset - guest (virtual) offset of the first sector of the
> + * data to be encrypted

Hmm, stop. It's wrong. Data to be encrypted is in buffer, so, it's not first 
sector of
the data to be encrypted, but first sector in which guest writes data (to be 
encrypted
in meantime).

> + * Used as an initialization vector for older, qcow2 native encryption
> + *
> + * @buf - buffer with the data to encrypt
> + * @len - length of the buffer (in sector size multiplies)
> + *
> + * Note that the group of the sectors, don't have to be aligned
> + * on cluster boundary and can also cross a cluster boundary.

And I doubt in it now. I'm afraid that if we call qcow2_co_encrypt for a group
of the sectors crossing a cluster boundary, we will finish up with similar bug: 
we'll
use first cluster offset as a vector for all the sectors. We still never do 
it.. So,
I think it worth assertion and corresponding comment.

Or is it correct?

> + *
> + *
> + */
>   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
> + * Same function as qcow2_co_encrypt

Hmm, not exactly same :)

> + *
> + */
> +
>   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,
Vladimir