Re: [Qemu-devel] [PATCH v2 4/4] block: convert qcrypto_block_encrypt|decrypt to take bytes offset

2017-08-31 Thread Daniel P. Berrange 
On Thu, Aug 31, 2017 at 10:17:43AM -0500, Eric Blake wrote:
> On 08/31/2017 06:05 AM, Daniel P. Berrange wrote:
> > Instead of sector offset, take the bytes offset when encrypting
> > or decrypting data.
> > 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  block/crypto.c |  8 
> >  block/qcow.c   |  7 +--
> >  block/qcow2-cluster.c  |  8 +++-
> >  block/qcow2.c  |  4 ++--
> >  crypto/block-luks.c| 12 
> >  crypto/block-qcow.c| 12 
> >  crypto/block.c | 14 --
> >  crypto/blockpriv.h |  4 ++--
> >  include/crypto/block.h | 14 --
> >  9 files changed, 48 insertions(+), 35 deletions(-)
> > 
> > diff --git a/block/crypto.c b/block/crypto.c
> > index 40daa77188..6b8d88efbc 100644
> > --- a/block/crypto.c
> > +++ b/block/crypto.c
> > @@ -426,8 +426,8 @@ block_crypto_co_preadv(BlockDriverState *bs, uint64_t 
> > offset, uint64_t bytes,
> >  goto cleanup;
> >  }
> >  
> > -if (qcrypto_block_decrypt(crypto->block, sector_num, cipher_data,
> > -  cur_bytes, NULL) < 0) {
> > +if (qcrypto_block_decrypt(crypto->block, offset + bytes_done,
> > +  cipher_data, cur_bytes, NULL) < 0) {
> 
> How close are we to getting rid of even needing 'sector_num' as a variable?

I thought there was more usage, but I just realized we can in fact
remove it in this patch.

> 
> > +++ b/block/qcow.c
> > @@ -456,7 +456,9 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
> >  if (i < n_start || i >= n_end) {
> >  Error *err = NULL;
> >  memset(s->cluster_data, 0x00, 512);
> > -if (qcrypto_block_encrypt(s->crypto, 
> > start_sect + i,
> > +if (qcrypto_block_encrypt(s->crypto,
> > +  start_sect + i *
> > +  BDRV_SECTOR_SIZE,
> 
> Umm, not the same.  You want (start_sect + i) * BDRV_SECTOR_SIZE.

Heh, oppps.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 4/4] block: convert qcrypto_block_encrypt|decrypt to take bytes offset

2017-08-31 Thread Eric Blake 
On 08/31/2017 06:05 AM, Daniel P. Berrange wrote:
> Instead of sector offset, take the bytes offset when encrypting
> or decrypting data.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  block/crypto.c |  8 
>  block/qcow.c   |  7 +--
>  block/qcow2-cluster.c  |  8 +++-
>  block/qcow2.c  |  4 ++--
>  crypto/block-luks.c| 12 
>  crypto/block-qcow.c| 12 
>  crypto/block.c | 14 --
>  crypto/blockpriv.h |  4 ++--
>  include/crypto/block.h | 14 --
>  9 files changed, 48 insertions(+), 35 deletions(-)
> 
> diff --git a/block/crypto.c b/block/crypto.c
> index 40daa77188..6b8d88efbc 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -426,8 +426,8 @@ block_crypto_co_preadv(BlockDriverState *bs, uint64_t 
> offset, uint64_t bytes,
>  goto cleanup;
>  }
>  
> -if (qcrypto_block_decrypt(crypto->block, sector_num, cipher_data,
> -  cur_bytes, NULL) < 0) {
> +if (qcrypto_block_decrypt(crypto->block, offset + bytes_done,
> +  cipher_data, cur_bytes, NULL) < 0) {

How close are we to getting rid of even needing 'sector_num' as a variable?

> +++ b/block/qcow.c
> @@ -456,7 +456,9 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>  if (i < n_start || i >= n_end) {
>  Error *err = NULL;
>  memset(s->cluster_data, 0x00, 512);
> -if (qcrypto_block_encrypt(s->crypto, start_sect 
> + i,
> +if (qcrypto_block_encrypt(s->crypto,
> +  start_sect + i *
> +  BDRV_SECTOR_SIZE,

Umm, not the same.  You want (start_sect + i) * BDRV_SECTOR_SIZE.

> +++ b/block/qcow2-cluster.c
> @@ -396,15 +396,13 @@ static bool coroutine_fn 
> do_perform_cow_encrypt(BlockDriverState *bs,
>  {
>  if (bytes && bs->encrypted) {
>  BDRVQcow2State *s = bs->opaque;
> -int64_t sector = (s->crypt_physical_offset ?
> +int64_t offset = (s->crypt_physical_offset ?
>(cluster_offset + offset_in_cluster) :
> -  (src_cluster_offset + offset_in_cluster))
> - >> BDRV_SECTOR_BITS;
> +  (src_cluster_offset + offset_in_cluster));
>  assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
>  assert((bytes & ~BDRV_SECTOR_MASK) == 0);

Pre-existing, but we could use osdep.h macros here, as in QEMU_IS_ALIGNED().

> +++ b/crypto/block-luks.c
> @@ -1403,29 +1403,33 @@ static void qcrypto_block_luks_cleanup(QCryptoBlock 
> *block)
>  
>  static int
>  qcrypto_block_luks_decrypt(QCryptoBlock *block,
> -   uint64_t startsector,
> +   uint64_t offset,
> uint8_t *buf,
> size_t len,
> Error **errp)
>  {
> +assert(!(offset % QCRYPTO_BLOCK_LUKS_SECTOR_SIZE));
> +assert(!(len % QCRYPTO_BLOCK_LUKS_SECTOR_SIZE));

Again, QEMU_IS_ALIGNED() might be easier to read - but this time, it's
in code you're adding.

Looking forward to v3.

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



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH v2 4/4] block: convert qcrypto_block_encrypt|decrypt to take bytes offset

2017-08-31 Thread Daniel P. Berrange 
Instead of sector offset, take the bytes offset when encrypting
or decrypting data.

Signed-off-by: Daniel P. Berrange 
---
 block/crypto.c |  8 
 block/qcow.c   |  7 +--
 block/qcow2-cluster.c  |  8 +++-
 block/qcow2.c  |  4 ++--
 crypto/block-luks.c| 12 
 crypto/block-qcow.c| 12 
 crypto/block.c | 14 --
 crypto/blockpriv.h |  4 ++--
 include/crypto/block.h | 14 --
 9 files changed, 48 insertions(+), 35 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index 40daa77188..6b8d88efbc 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -426,8 +426,8 @@ block_crypto_co_preadv(BlockDriverState *bs, uint64_t 
offset, uint64_t bytes,
 goto cleanup;
 }
 
-if (qcrypto_block_decrypt(crypto->block, sector_num, cipher_data,
-  cur_bytes, NULL) < 0) {
+if (qcrypto_block_decrypt(crypto->block, offset + bytes_done,
+  cipher_data, cur_bytes, NULL) < 0) {
 ret = -EIO;
 goto cleanup;
 }
@@ -484,8 +484,8 @@ block_crypto_co_pwritev(BlockDriverState *bs, uint64_t 
offset, uint64_t bytes,
 
 qemu_iovec_to_buf(qiov, bytes_done, cipher_data, cur_bytes);
 
-if (qcrypto_block_encrypt(crypto->block, sector_num, cipher_data,
-  cur_bytes, NULL) < 0) {
+if (qcrypto_block_encrypt(crypto->block, offset + bytes_done,
+  cipher_data, cur_bytes, NULL) < 0) {
 ret = -EIO;
 goto cleanup;
 }
diff --git a/block/qcow.c b/block/qcow.c
index c08cdc4a7b..c2a446e824 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -456,7 +456,9 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
 if (i < n_start || i >= n_end) {
 Error *err = NULL;
 memset(s->cluster_data, 0x00, 512);
-if (qcrypto_block_encrypt(s->crypto, start_sect + 
i,
+if (qcrypto_block_encrypt(s->crypto,
+  start_sect + i *
+  BDRV_SECTOR_SIZE,
   s->cluster_data,
   BDRV_SECTOR_SIZE,
   ) < 0) {
@@ -636,7 +638,8 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, 
int64_t sector_num,
 }
 if (bs->encrypted) {
 assert(s->crypto);
-if (qcrypto_block_decrypt(s->crypto, sector_num, buf,
+if (qcrypto_block_decrypt(s->crypto,
+  sector_num * BDRV_SECTOR_SIZE, buf,
   n * BDRV_SECTOR_SIZE, ) < 0) {
 goto fail;
 }
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index f06c08f64c..85ffc33c2c 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -396,15 +396,13 @@ static bool coroutine_fn 
do_perform_cow_encrypt(BlockDriverState *bs,
 {
 if (bytes && bs->encrypted) {
 BDRVQcow2State *s = bs->opaque;
-int64_t sector = (s->crypt_physical_offset ?
+int64_t offset = (s->crypt_physical_offset ?
   (cluster_offset + offset_in_cluster) :
-  (src_cluster_offset + offset_in_cluster))
- >> BDRV_SECTOR_BITS;
+  (src_cluster_offset + offset_in_cluster));
 assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
 assert((bytes & ~BDRV_SECTOR_MASK) == 0);
 assert(s->crypto);
-if (qcrypto_block_encrypt(s->crypto, sector, buffer,
-  bytes, NULL) < 0) {
+if (qcrypto_block_encrypt(s->crypto, offset, buffer, bytes, NULL) < 0) 
{
 return false;
 }
 }
diff --git a/block/qcow2.c b/block/qcow2.c
index 40ba26c111..1c9f6c8f7d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1824,7 +1824,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState 
*bs, uint64_t offset,
 if (qcrypto_block_decrypt(s->crypto,
   (s->crypt_physical_offset ?
cluster_offset + offset_in_cluster :
-   offset) >> BDRV_SECTOR_BITS,
+   offset),
   cluster_data,
   cur_bytes,
   ) < 0) {
@@ -1961,7 +1961,7 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState 
*bs, uint64_t offset,
 if (qcrypto_block_encrypt(s->crypto,