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 <mlevi...@redhat.com> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@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