Re: [Qemu-devel] [PATCH 2/3] block/qcow2: fix the corruption when rebasing luks encrypted files

[Maxim Levitsky]

On Fri, 2019-09-06 at 14:17 -0500, Eric Blake wrote:
> On 9/6/19 12:32 PM, Maxim Levitsky wrote:
> > This fixes subltle corruption introduced by luks threaded encryption
> 
> subtle

I usually put the commit messages to a spellchecker, but this time
I forgot to do this. I will try not to in the future.

> 
> > in commit 8ac0f15f335
> > 
> > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1745922
> > 
> > The corruption happens when we do
> >    * write to two or more unallocated clusters at once
> >    * write doesn't fully cover nether first nor last cluster
> 
> s/nether/neither/
> 
> or even:
> 
> write doesn't fully cover either the first or the last cluster
I think I didn't wrote the double negative correctly here.
I meant a write that doesn't cover first sector fully and doesn't cover second 
sector.
I'll just write it like that I guess.

> 
> > 
> > In this case, when allocating the new clusters we COW both area
> 
> areas
> 
> > prior to the write and after the write, and we encrypt them.
> > 
> > The above mentioned commit accidently made it so, we encrypt the
> 
> accidentally
> 
> s/made it so, we encrypt/changed the encryption of/
> 
> > second COW are using the physical cluster offset of the first area.
> 
> s/are using/to use/
I actually meant to write 'area' here. I just haven't proofed the commit
message at all I confess. Next time I do better.

> 
> > 
> > Fix this by:
> >  * remove the offset_in_cluster parameter of do_perform_cow_encrypt
> >    since it is misleading. That offset can be larger that cluster size.
> >    instead just add the start and end COW are offsets to both host and 
> > guest offsets
> >    that do_perform_cow_encrypt receives.
> > 
> > *  in do_perform_cow_encrypt, remove the cluster offset from the host_offset
> >    And thus pass correctly to the qcow2_co_encrypt, the host cluster offset 
> > and full guest offset
> > 
> > 
> > Signed-off-by: Maxim Levitsky <address@hidden>
> > ---
> >  block/qcow2-cluster.c | 26 +++++++++++++++-----------
> >  1 file changed, 15 insertions(+), 11 deletions(-)
> > 
> > +++ b/block/qcow2-cluster.c
> > @@ -463,20 +463,20 @@ static int coroutine_fn 
> > do_perform_cow_read(BlockDriverState *bs,
> >  }
> >  
> >  static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
> > -                                                uint64_t 
> > guest_cluster_offset,
> > -                                                uint64_t 
> > host_cluster_offset,
> > -                                                unsigned offset_in_cluster,
> > +                                                uint64_t guest_offset,
> > +                                                uint64_t host_offset,
> >                                                  uint8_t *buffer,
> >                                                  unsigned bytes)
> >  {
> >      if (bytes && bs->encrypted) {
> >          BDRVQcow2State *s = bs->opaque;
> > -        assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
> > +        assert((guest_offset & ~BDRV_SECTOR_MASK) == 0);
> > +        assert((host_offset & ~BDRV_SECTOR_MASK) == 0);
> >          assert((bytes & ~BDRV_SECTOR_MASK) == 0);
> 
> Pre-existing, but we could use QEMU_IS_ALIGNED(x, BDRV_SECTOR_SIZE) for
> slightly more legibility than open-coding the bit operation.
> 
> Neat trick about power-of-2 alignment checks:
> 
> assert(QEMU_IS_ALIGNED(offset_in_cluster | guest_offset |
>                        host_offset | bytes, BDRV_SECTOR_SIZE));

In my book, a shorter code is almost always better, so why not.
> 
> gives the same result in one assertion.  (I've used it elsewhere in the
> code base, but I'm not opposed to one assert per variable if you think
> batching is too dense.)
> 
> I'll let Dan review the actual code change, but offhand it makes sense
> to me.
> 

Best regards,
        Thanks for the review,
                Maxim Levitsky