qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Max Reitz <mreitz@redhat.com>
To: Maxim Levitsky <mlevitsk@redhat.com>, qemu-devel@nongnu.org
Cc: "Kevin Wolf" <kwolf@redhat.com>,
	"Vladimir Sementsov-Ogievskiy" <vsementsov@virtuozzo.com>,
	"Daniel P . Berrangé" <berrange@redhat.com>,
	qemu-block@nongnu.org, qemu-stable <qemu-stable@nongnu.org>
Subject: Re: [Qemu-devel] [PATCH v3 1/3] block/qcow2: refactoring of threaded encryption code
Date: Fri, 13 Sep 2019 12:37:52 +0200	[thread overview]
Message-ID: <abd660bc-5e3c-ec6d-e595-6830e1b6f0d1@redhat.com> (raw)
In-Reply-To: <20190912223754.875-2-mlevitsk@redhat.com>


[-- Attachment #1.1: Type: text/plain, Size: 7146 bytes --]

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 <mlevitsk@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)

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  reply	other threads:[~2019-09-13 10:38 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-12 22:37 [Qemu-devel] [PATCH v3 0/3] Fix qcow2+luks corruption introduced by commit 8ac0f15f335 Maxim Levitsky
2019-09-12 22:37 ` [Qemu-devel] [PATCH v3 1/3] block/qcow2: refactoring of threaded encryption code Maxim Levitsky
2019-09-13 10:37   ` Max Reitz [this message]
2019-09-13 13:21     ` Maxim Levitsky
2019-09-12 22:37 ` [Qemu-devel] [PATCH v3 2/3] block/qcow2: fix the corruption when rebasing luks encrypted files Maxim Levitsky
2019-09-13 10:52   ` Max Reitz
2019-09-12 22:37 ` [Qemu-devel] [PATCH v3 3/3] qemu-iotests: Add test for bz #1745922 Maxim Levitsky
2019-09-13 11:01   ` Max Reitz
2019-09-13 11:53     ` Maxim Levitsky

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=abd660bc-5e3c-ec6d-e595-6830e1b6f0d1@redhat.com \
    --to=mreitz@redhat.com \
    --cc=berrange@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=mlevitsk@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-stable@nongnu.org \
    --cc=vsementsov@virtuozzo.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).