Re: [Qemu-stable] [Qemu-devel] [PATCH v6 2/3] block/qcow2: refactor threaded encryption code

[Maxim Levitsky]

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 <address@hidden>
> > Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > ---
> >   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