Change the qcow2_co_encrypt to just receive full host and
guest offsets and in pariticular remove the
offset_in_cluster parameter of do_perform_cow_encrypt,
since it is misleading, because that offset can be larger than
cluster size currently.
Remove the do_perform_cow_encrypt by merging it with
qcow2_co_encrypt
Also document the qcow2_co_encrypt arguments to prevent
that bug from happening again
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2-cluster.c | 35 +++++--------------
block/qcow2-threads.c | 81 ++++++++++++++++++++++++++++++++++++-------
block/qcow2.c | 5 +--
block/qcow2.h | 8 ++---
4 files changed, 83 insertions(+), 46 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index bfeb0241d7..f42b8a404c 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -462,28 +462,6 @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
return 0;
}
-static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
- uint64_t src_cluster_offset,
- uint64_t cluster_offset,
- unsigned offset_in_cluster,
- uint8_t *buffer,
- unsigned bytes)
-{
- if (bytes && bs->encrypted) {
- BDRVQcow2State *s = bs->opaque;
- assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
- assert((bytes & ~BDRV_SECTOR_MASK) == 0);
- assert(s->crypto);
- if (qcow2_co_encrypt(bs,
- start_of_cluster(s, cluster_offset + offset_in_cluster),
- src_cluster_offset + offset_in_cluster,
- buffer, bytes) < 0) {
- return false;
- }
- }
- return true;
-}
-
static int coroutine_fn do_perform_cow_write(BlockDriverState *bs,
uint64_t cluster_offset,
unsigned offset_in_cluster,
@@ -891,11 +869,14 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta *m)
/* Encrypt the data if necessary before writing it */
if (bs->encrypted) {
- if (!do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
- start->offset, start_buffer,
- start->nb_bytes) ||
- !do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
- end->offset, end_buffer, end->nb_bytes)) {
+ if (!qcow2_co_encrypt(bs,
+ m->offset + start->offset,
+ m->alloc_offset + start->offset,
+ start_buffer, start->nb_bytes) ||
+ !qcow2_co_encrypt(bs,
+ m->offset + end->offset,
+ m->alloc_offset + end->offset,
+ end_buffer, end->nb_bytes)) {
ret = -EIO;
goto fail;
}
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 3b1e63fe41..b31d45fb2b 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -234,15 +234,15 @@ 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_offset,
+ uint64_t guest_offset, void *buf, size_t len,
+ Qcow2EncDecFunc func)
{
BDRVQcow2State *s = bs->opaque;
+
Qcow2EncDecData arg = {
.block = s->crypto,
- .offset = s->crypt_physical_offset ?
- file_cluster_offset + offset_into_cluster(s, offset) :
- offset,
+ .offset = s->crypt_physical_offset ? host_offset : guest_offset,
.buf = buf,
.len = len,
.func = func,
@@ -251,18 +251,73 @@ 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_offset - underlying storage offset of the first sector of the
+ * data to be encrypted
+ *
+ * @guest_offset - guest (virtual) offset of the first sector of the
+ * data to be encrypted
+ *
+ * @buf - buffer with the data to encrypt, that after encryption
+ * will be written to the underlying storage device at
+ * @host_offset
+ *
+ * @len - length of the buffer (must be a BDRV_SECTOR_SIZE multiple)
+ *
+ * Depending on the encryption method, @host_offset and/or @guest_offset
+ * may be used for generating the initialization vector for
+ * encryption.
+ *
+ * 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
+ */
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_offset,
+ uint64_t guest_offset, void *buf, size_t len)
{
- return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
- qcrypto_block_encrypt);
+
+ BDRVQcow2State *s = bs->opaque;
+ assert(QEMU_IS_ALIGNED(guest_offset, BDRV_SECTOR_SIZE));
+ assert(QEMU_IS_ALIGNED(host_offset, BDRV_SECTOR_SIZE));
+ assert(QEMU_IS_ALIGNED(len, BDRV_SECTOR_SIZE));
+ assert(s->crypto);
+
+ if (!len) {
+ return 0;
+ }
+
+ return qcow2_co_encdec(bs, host_offset, guest_offset, buf, len,
+ qcrypto_block_encrypt);
}
+
+/*
+ * qcow2_co_decrypt()
+ *
+ * Decrypts one or more contiguous aligned sectors
+ * Similar to 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_offset,
+ uint64_t guest_offset, void *buf, size_t len)
{
- return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
- qcrypto_block_decrypt);
+ BDRVQcow2State *s = bs->opaque;
+ assert(QEMU_IS_ALIGNED(guest_offset, BDRV_SECTOR_SIZE));
+ assert(QEMU_IS_ALIGNED(host_offset, BDRV_SECTOR_SIZE));
+ assert(QEMU_IS_ALIGNED(len, BDRV_SECTOR_SIZE));
+ assert(s->crypto);
+
+ if (!len) {
+ return 0;
+ }
+
+ return qcow2_co_encdec(bs, host_offset, guest_offset, buf, len,
+ qcrypto_block_decrypt);
}
diff --git a/block/qcow2.c b/block/qcow2.c
index 57734f20cf..ac768092bb 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2069,7 +2069,8 @@ static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs,
assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
assert((cur_bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
- if (qcow2_co_decrypt(bs, cluster_offset, offset,
+ if (qcow2_co_decrypt(bs, cluster_offset + offset_in_cluster,
+ offset,
cluster_data, cur_bytes) < 0) {
ret = -EIO;
goto fail;
@@ -2288,7 +2289,7 @@ static coroutine_fn int qcow2_co_pwritev_part(
qemu_iovec_to_buf(qiov, qiov_offset + bytes_done,
cluster_data, cur_bytes);
- if (qcow2_co_encrypt(bs, cluster_offset, offset,
+ if (qcow2_co_encrypt(bs, cluster_offset + offset_in_cluster, offset,
cluster_data, cur_bytes) < 0) {
ret = -EIO;
goto out_unlocked;
diff --git a/block/qcow2.h b/block/qcow2.h
index 998bcdaef1..a488d761ff 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -758,10 +758,10 @@ ssize_t coroutine_fn
qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
const void *src, size_t src_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_offset,
+ uint64_t guest_offset, void *buf, size_t len);
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_offset,
+ uint64_t guest_offset, void *buf, size_t len);
#endif
--
2.17.2
13.09.2019 20:27, Maxim Levitsky wrote:
> Change the qcow2_co_encrypt to just receive full host and
> guest offsets and in pariticular remove the
> offset_in_cluster parameter of do_perform_cow_encrypt,
> since it is misleading, because that offset can be larger than
> cluster size currently.
>
> Remove the do_perform_cow_encrypt by merging it with
> qcow2_co_encrypt
>
> Also document the qcow2_co_encrypt arguments to prevent
> that bug from happening again
>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/qcow2-cluster.c | 35 +++++--------------
> block/qcow2-threads.c | 81 ++++++++++++++++++++++++++++++++++++-------
> block/qcow2.c | 5 +--
> block/qcow2.h | 8 ++---
> 4 files changed, 83 insertions(+), 46 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index bfeb0241d7..f42b8a404c 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -462,28 +462,6 @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
> return 0;
> }
>
> -static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
> - uint64_t src_cluster_offset,
> - uint64_t cluster_offset,
> - unsigned offset_in_cluster,
> - uint8_t *buffer,
> - unsigned bytes)
> -{
> - if (bytes && bs->encrypted) {
> - BDRVQcow2State *s = bs->opaque;
> - assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
> - assert((bytes & ~BDRV_SECTOR_MASK) == 0);
> - assert(s->crypto);
> - if (qcow2_co_encrypt(bs,
> - start_of_cluster(s, cluster_offset + offset_in_cluster),
> - src_cluster_offset + offset_in_cluster,
> - buffer, bytes) < 0) {
> - return false;
> - }
> - }
> - return true;
> -}
> -
> static int coroutine_fn do_perform_cow_write(BlockDriverState *bs,
> uint64_t cluster_offset,
> unsigned offset_in_cluster,
> @@ -891,11 +869,14 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta *m)
>
> /* Encrypt the data if necessary before writing it */
> if (bs->encrypted) {
> - if (!do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
> - start->offset, start_buffer,
> - start->nb_bytes) ||
> - !do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
> - end->offset, end_buffer, end->nb_bytes)) {
> + if (!qcow2_co_encrypt(bs,
> + m->offset + start->offset,
> + m->alloc_offset + start->offset,
> + start_buffer, start->nb_bytes) ||
You got caught)
second argument qcow2_co_encrypt is host_offset and third is guest_offset.
I noticed this difference when reviewing previous version, and it's actually
additional reason to drop the function. Unfortunately, I didn't said.
Also, we can use return value of qcow2_co_encrypt instead of dropping it,
something like classical
ret = qcow2_co_encrypt(..)
if (ret < 0) {
goto fail;
}
ret = qcow2_co_encrypt(..)
if (ret < 0) {
goto fail;
}
Also, IMHO, too much changes too keep reviewed-by mark. Still, maybe it's my
"with it least fixed" is misleading, may be interpreted like removing the
function and corresponding refactoring is included..
> + !qcow2_co_encrypt(bs,
> + m->offset + end->offset,
> + m->alloc_offset + end->offset,
> + end_buffer, end->nb_bytes)) {
> ret = -EIO;
> goto fail;
> }
> diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
> index 3b1e63fe41..b31d45fb2b 100644
> --- a/block/qcow2-threads.c
> +++ b/block/qcow2-threads.c
> @@ -234,15 +234,15 @@ 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_offset,
> + uint64_t guest_offset, void *buf, size_t len,
> + Qcow2EncDecFunc func)
> {
> BDRVQcow2State *s = bs->opaque;
> +
> Qcow2EncDecData arg = {
> .block = s->crypto,
> - .offset = s->crypt_physical_offset ?
> - file_cluster_offset + offset_into_cluster(s, offset) :
> - offset,
> + .offset = s->crypt_physical_offset ? host_offset : guest_offset,
> .buf = buf,
> .len = len,
> .func = func,
> @@ -251,18 +251,73 @@ 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_offset - underlying storage offset of the first sector of the
> + * data to be encrypted
> + *
> + * @guest_offset - guest (virtual) offset of the first sector of the
> + * data to be encrypted
> + *
> + * @buf - buffer with the data to encrypt, that after encryption
> + * will be written to the underlying storage device at
> + * @host_offset
> + *
> + * @len - length of the buffer (must be a BDRV_SECTOR_SIZE multiple)
> + *
> + * Depending on the encryption method, @host_offset and/or @guest_offset
> + * may be used for generating the initialization vector for
> + * encryption.
> + *
> + * 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
> + */
> 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_offset,
> + uint64_t guest_offset, void *buf, size_t len)
> {
> - return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
> - qcrypto_block_encrypt);
> +
> + BDRVQcow2State *s = bs->opaque;
> + assert(QEMU_IS_ALIGNED(guest_offset, BDRV_SECTOR_SIZE));
> + assert(QEMU_IS_ALIGNED(host_offset, BDRV_SECTOR_SIZE));
> + assert(QEMU_IS_ALIGNED(len, BDRV_SECTOR_SIZE));
> + assert(s->crypto);
Hmm, better to move these asertions to _encdec I think, and don't duplicate
them in two functions.
> +
> + if (!len) {
> + return 0;
> + }
Aha, that's why you don't check this in perform_cow. That's OK,
but still, better keep it in _encdec and don't duplicate.
> +
> + return qcow2_co_encdec(bs, host_offset, guest_offset, buf, len,
> + qcrypto_block_encrypt);
> }
>
> +
> +/*
> + * qcow2_co_decrypt()
> + *
> + * Decrypts one or more contiguous aligned sectors
> + * Similar to 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_offset,
> + uint64_t guest_offset, void *buf, size_t len)
> {
> - return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
> - qcrypto_block_decrypt);
> + BDRVQcow2State *s = bs->opaque;
> + assert(QEMU_IS_ALIGNED(guest_offset, BDRV_SECTOR_SIZE));
> + assert(QEMU_IS_ALIGNED(host_offset, BDRV_SECTOR_SIZE));
> + assert(QEMU_IS_ALIGNED(len, BDRV_SECTOR_SIZE));
> + assert(s->crypto);
> +
> + if (!len) {
> + return 0;
> + }
> +
> + return qcow2_co_encdec(bs, host_offset, guest_offset, buf, len,
> + qcrypto_block_decrypt);
> }
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 57734f20cf..ac768092bb 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2069,7 +2069,8 @@ static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs,
>
> assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> assert((cur_bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> - if (qcow2_co_decrypt(bs, cluster_offset, offset,
> + if (qcow2_co_decrypt(bs, cluster_offset + offset_in_cluster,
> + offset,
> cluster_data, cur_bytes) < 0) {
> ret = -EIO;
> goto fail;
> @@ -2288,7 +2289,7 @@ static coroutine_fn int qcow2_co_pwritev_part(
> qemu_iovec_to_buf(qiov, qiov_offset + bytes_done,
> cluster_data, cur_bytes);
>
> - if (qcow2_co_encrypt(bs, cluster_offset, offset,
> + if (qcow2_co_encrypt(bs, cluster_offset + offset_in_cluster, offset,
> cluster_data, cur_bytes) < 0) {
> ret = -EIO;
> goto out_unlocked;
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 998bcdaef1..a488d761ff 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -758,10 +758,10 @@ ssize_t coroutine_fn
> qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
> const void *src, size_t src_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_offset,
> + uint64_t guest_offset, void *buf, size_t len);
> 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_offset,
> + uint64_t guest_offset, void *buf, size_t len);
>
> #endif
>
--
Best regards,
Vladimir
On Fri, 2019-09-13 at 17:51 +0000, Vladimir Sementsov-Ogievskiy wrote:
> 13.09.2019 20:27, Maxim Levitsky wrote:
> > Change the qcow2_co_encrypt to just receive full host and
> > guest offsets and in pariticular remove the
> > offset_in_cluster parameter of do_perform_cow_encrypt,
> > since it is misleading, because that offset can be larger than
> > cluster size currently.
> >
> > Remove the do_perform_cow_encrypt by merging it with
> > qcow2_co_encrypt
> >
> > Also document the qcow2_co_encrypt arguments to prevent
> > that bug from happening again
> >
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> > block/qcow2-cluster.c | 35 +++++--------------
> > block/qcow2-threads.c | 81 ++++++++++++++++++++++++++++++++++++-------
> > block/qcow2.c | 5 +--
> > block/qcow2.h | 8 ++---
> > 4 files changed, 83 insertions(+), 46 deletions(-)
> >
> > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > index bfeb0241d7..f42b8a404c 100644
> > --- a/block/qcow2-cluster.c
> > +++ b/block/qcow2-cluster.c
> > @@ -462,28 +462,6 @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
> > return 0;
> > }
> >
> > -static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
> > - uint64_t src_cluster_offset,
> > - uint64_t cluster_offset,
> > - unsigned offset_in_cluster,
> > - uint8_t *buffer,
> > - unsigned bytes)
> > -{
> > - if (bytes && bs->encrypted) {
> > - BDRVQcow2State *s = bs->opaque;
> > - assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
> > - assert((bytes & ~BDRV_SECTOR_MASK) == 0);
> > - assert(s->crypto);
> > - if (qcow2_co_encrypt(bs,
> > - start_of_cluster(s, cluster_offset + offset_in_cluster),
> > - src_cluster_offset + offset_in_cluster,
> > - buffer, bytes) < 0) {
> > - return false;
> > - }
> > - }
> > - return true;
> > -}
> > -
> > static int coroutine_fn do_perform_cow_write(BlockDriverState *bs,
> > uint64_t cluster_offset,
> > unsigned offset_in_cluster,
> > @@ -891,11 +869,14 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta *m)
> >
> > /* Encrypt the data if necessary before writing it */
> > if (bs->encrypted) {
> > - if (!do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
> > - start->offset, start_buffer,
> > - start->nb_bytes) ||
> > - !do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
> > - end->offset, end_buffer, end->nb_bytes)) {
> > + if (!qcow2_co_encrypt(bs,
> > + m->offset + start->offset,
> > + m->alloc_offset + start->offset,
> > + start_buffer, start->nb_bytes) ||
>
> You got caught)
Ha!, I am just too tired.
> second argument qcow2_co_encrypt is host_offset and third is guest_offset.
>
> I noticed this difference when reviewing previous version, and it's actually
> additional reason to drop the function. Unfortunately, I didn't said.
>
> Also, we can use return value of qcow2_co_encrypt instead of dropping it,
>
> something like classical
>
> ret = qcow2_co_encrypt(..)
> if (ret < 0) {
> goto fail;
> }
>
> ret = qcow2_co_encrypt(..)
> if (ret < 0) {
> goto fail;
> }
>
> Also, IMHO, too much changes too keep reviewed-by mark. Still, maybe it's my
> "with it least fixed" is misleading, may be interpreted like removing the
> function and corresponding refactoring is included..
Yea. So V7 there will be...
Re-factoring is hard (but worth it) :-(
>
>
> > + !qcow2_co_encrypt(bs,
> > + m->offset + end->offset,
> > + m->alloc_offset + end->offset,
> > + end_buffer, end->nb_bytes)) {
> > ret = -EIO;
> > goto fail;
> > }
> > diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
> > index 3b1e63fe41..b31d45fb2b 100644
> > --- a/block/qcow2-threads.c
> > +++ b/block/qcow2-threads.c
> > @@ -234,15 +234,15 @@ 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_offset,
> > + uint64_t guest_offset, void *buf, size_t len,
> > + Qcow2EncDecFunc func)
> > {
> > BDRVQcow2State *s = bs->opaque;
> > +
> > Qcow2EncDecData arg = {
> > .block = s->crypto,
> > - .offset = s->crypt_physical_offset ?
> > - file_cluster_offset + offset_into_cluster(s, offset) :
> > - offset,
> > + .offset = s->crypt_physical_offset ? host_offset : guest_offset,
> > .buf = buf,
> > .len = len,
> > .func = func,
> > @@ -251,18 +251,73 @@ 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_offset - underlying storage offset of the first sector of the
> > + * data to be encrypted
> > + *
> > + * @guest_offset - guest (virtual) offset of the first sector of the
> > + * data to be encrypted
> > + *
> > + * @buf - buffer with the data to encrypt, that after encryption
> > + * will be written to the underlying storage device at
> > + * @host_offset
> > + *
> > + * @len - length of the buffer (must be a BDRV_SECTOR_SIZE multiple)
> > + *
> > + * Depending on the encryption method, @host_offset and/or @guest_offset
> > + * may be used for generating the initialization vector for
> > + * encryption.
> > + *
> > + * 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
> > + */
> > 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_offset,
> > + uint64_t guest_offset, void *buf, size_t len)
> > {
> > - return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
> > - qcrypto_block_encrypt);
> > +
> > + BDRVQcow2State *s = bs->opaque;
> > + assert(QEMU_IS_ALIGNED(guest_offset, BDRV_SECTOR_SIZE));
> > + assert(QEMU_IS_ALIGNED(host_offset, BDRV_SECTOR_SIZE));
> > + assert(QEMU_IS_ALIGNED(len, BDRV_SECTOR_SIZE));
> > + assert(s->crypto);
>
> Hmm, better to move these asertions to _encdec I think, and don't duplicate
> them in two functions.
OK.
>
> > +
> > + if (!len) {
> > + return 0;
> > + }
>
> Aha, that's why you don't check this in perform_cow. That's OK,
> but still, better keep it in _encdec and don't duplicate.
OK.
>
> > +
> > + return qcow2_co_encdec(bs, host_offset, guest_offset, buf, len,
> > + qcrypto_block_encrypt);
> > }
> >
> > +
> > +/*
> > + * qcow2_co_decrypt()
> > + *
> > + * Decrypts one or more contiguous aligned sectors
> > + * Similar to 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_offset,
> > + uint64_t guest_offset, void *buf, size_t len)
> > {
> > - return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
> > - qcrypto_block_decrypt);
> > + BDRVQcow2State *s = bs->opaque;
> > + assert(QEMU_IS_ALIGNED(guest_offset, BDRV_SECTOR_SIZE));
> > + assert(QEMU_IS_ALIGNED(host_offset, BDRV_SECTOR_SIZE));
> > + assert(QEMU_IS_ALIGNED(len, BDRV_SECTOR_SIZE));
> > + assert(s->crypto);
> > +
> > + if (!len) {
> > + return 0;
> > + }
> > +
> > + return qcow2_co_encdec(bs, host_offset, guest_offset, buf, len,
> > + qcrypto_block_decrypt);
> > }
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 57734f20cf..ac768092bb 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -2069,7 +2069,8 @@ static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs,
> >
> > assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> > assert((cur_bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> > - if (qcow2_co_decrypt(bs, cluster_offset, offset,
> > + if (qcow2_co_decrypt(bs, cluster_offset + offset_in_cluster,
> > + offset,
> > cluster_data, cur_bytes) < 0) {
> > ret = -EIO;
> > goto fail;
> > @@ -2288,7 +2289,7 @@ static coroutine_fn int qcow2_co_pwritev_part(
> > qemu_iovec_to_buf(qiov, qiov_offset + bytes_done,
> > cluster_data, cur_bytes);
> >
> > - if (qcow2_co_encrypt(bs, cluster_offset, offset,
> > + if (qcow2_co_encrypt(bs, cluster_offset + offset_in_cluster, offset,
> > cluster_data, cur_bytes) < 0) {
> > ret = -EIO;
> > goto out_unlocked;
> > diff --git a/block/qcow2.h b/block/qcow2.h
> > index 998bcdaef1..a488d761ff 100644
> > --- a/block/qcow2.h
> > +++ b/block/qcow2.h
> > @@ -758,10 +758,10 @@ ssize_t coroutine_fn
> > qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
> > const void *src, size_t src_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_offset,
> > + uint64_t guest_offset, void *buf, size_t len);
> > 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_offset,
> > + uint64_t guest_offset, void *buf, size_t len);
> >
> > #endif
> >
Best regards,
Maxim Levitsky
and a bit about empty lines
13.09.2019 20:27, Maxim Levitsky wrote:
> Change the qcow2_co_encrypt to just receive full host and
> guest offsets and in pariticular remove the
> offset_in_cluster parameter of do_perform_cow_encrypt,
> since it is misleading, because that offset can be larger than
> cluster size currently.
>
> Remove the do_perform_cow_encrypt by merging it with
> qcow2_co_encrypt
>
> Also document the qcow2_co_encrypt arguments to prevent
> that bug from happening again
>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
[..]
> diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
> index 3b1e63fe41..b31d45fb2b 100644
> --- a/block/qcow2-threads.c
> +++ b/block/qcow2-threads.c
> @@ -234,15 +234,15 @@ 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_offset,
> + uint64_t guest_offset, void *buf, size_t len,
> + Qcow2EncDecFunc func)
> {
> BDRVQcow2State *s = bs->opaque;
> +
Don't see real benefit in separating one variable initialization from another one here,
but I don't mind, it's OK of course.
> Qcow2EncDecData arg = {
> .block = s->crypto,
> - .offset = s->crypt_physical_offset ?
> - file_cluster_offset + offset_into_cluster(s, offset) :
> - offset,
> + .offset = s->crypt_physical_offset ? host_offset : guest_offset,
> .buf = buf,
> .len = len,
> .func = func,
> @@ -251,18 +251,73 @@ qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
> return qcow2_co_process(bs, qcow2_encdec_pool_func, &arg);
> }
>
> +
Hmm, in this file doubled empty lines
are used to separate "Compression" and "Cryptography"
sections. And all other splits are one-empty-line.
> +/*
> + * qcow2_co_encrypt()
> + *
> + * Encrypts one or more contiguous aligned sectors
> + *
> + * @host_offset - underlying storage offset of the first sector of the
> + * data to be encrypted
> + *
> + * @guest_offset - guest (virtual) offset of the first sector of the
> + * data to be encrypted
> + *
> + * @buf - buffer with the data to encrypt, that after encryption
> + * will be written to the underlying storage device at
> + * @host_offset
> + *
> + * @len - length of the buffer (must be a BDRV_SECTOR_SIZE multiple)
> + *
> + * Depending on the encryption method, @host_offset and/or @guest_offset
> + * may be used for generating the initialization vector for
> + * encryption.
> + *
> + * 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
> + */
> 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_offset,
> + uint64_t guest_offset, void *buf, size_t len)
> {
> - return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
> - qcrypto_block_encrypt);
> +
This empty line is not needed for sure
> + BDRVQcow2State *s = bs->opaque;
But it's common practice to split variables definition from code by empty line here
> + assert(QEMU_IS_ALIGNED(guest_offset, BDRV_SECTOR_SIZE));
> + assert(QEMU_IS_ALIGNED(host_offset, BDRV_SECTOR_SIZE));
> + assert(QEMU_IS_ALIGNED(len, BDRV_SECTOR_SIZE));
> + assert(s->crypto);
> +
> + if (!len) {
> + return 0;
> + }
> +
> + return qcow2_co_encdec(bs, host_offset, guest_offset, buf, len,
> + qcrypto_block_encrypt);
> }
>
> +
IMHO extra one (see above)
> +/*
> + * qcow2_co_decrypt()
> + *
> + * Decrypts one or more contiguous aligned sectors
> + * Similar to qcow2_co_encrypt
> + */
> +
Here you replaced one extra empty line by another
> 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_offset,
> + uint64_t guest_offset, void *buf, size_t len)
> {
> - return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
> - qcrypto_block_decrypt);
--
Best regards,
Vladimir
© 2016 - 2026 Red Hat, Inc.