[v4,1/3] block/qcow2: refactoring of threaded encryption code
diff mbox series

Message ID 20190913125909.15348-2-mlevitsk@redhat.com
State New
Headers show
Series
  • Fix qcow2+luks corruption introduced by commit 8ac0f15f335
Related show

Commit Message

Maxim Levitsky Sept. 13, 2019, 12:59 p.m. UTC
This commit tries to clarify few function arguments,
and add comments describing the encrypt/decrypt interface

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 block/qcow2-cluster.c |  9 ++++---
 block/qcow2-threads.c | 62 ++++++++++++++++++++++++++++++++++---------
 block/qcow2.c         |  5 ++--
 block/qcow2.h         |  8 +++---
 4 files changed, 61 insertions(+), 23 deletions(-)

Comments

Vladimir Sementsov-Ogievskiy Sept. 13, 2019, 2:01 p.m. UTC | #1
13.09.2019 15:59, Maxim Levitsky wrote:
> This commit tries to clarify few function arguments,
> and add comments describing the encrypt/decrypt interface
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>   block/qcow2-cluster.c |  9 ++++---
>   block/qcow2-threads.c | 62 ++++++++++++++++++++++++++++++++++---------
>   block/qcow2.c         |  5 ++--
>   block/qcow2.h         |  8 +++---
>   4 files changed, 61 insertions(+), 23 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index f09cc992af..46b0854d7e 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -463,8 +463,8 @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
>   }
>   
>   static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
> -                                                uint64_t src_cluster_offset,
> -                                                uint64_t cluster_offset,
> +                                                uint64_t guest_cluster_offset,
> +                                                uint64_t host_cluster_offset,
>                                                   unsigned offset_in_cluster,
>                                                   uint8_t *buffer,
>                                                   unsigned bytes)
> @@ -474,8 +474,9 @@ static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
>           assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
>           assert((bytes & ~BDRV_SECTOR_MASK) == 0);
>           assert(s->crypto);
> -        if (qcow2_co_encrypt(bs, cluster_offset,
> -                             src_cluster_offset + offset_in_cluster,
> +        if (qcow2_co_encrypt(bs,
> +                             host_cluster_offset + offset_in_cluster,
> +                             guest_cluster_offset + offset_in_cluster,

oops, seems you accidentally fixed the bug, which you are going to fix in the next
patch, as now correct offsets are given to qcow2_co_encrypt :)

and next patch no is a simple no-logic-change refactoring, so at least commit message
should be changed.

>                                buffer, bytes) < 0) {
>               return false;
>           }
> diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
> index 3b1e63fe41..9646243a9b 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,

Nice change. Obviously original design was worse.

>           .buf = buf,
>           .len = len,
>           .func = func,
> @@ -251,18 +251,54 @@ 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_cluster_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);
> +    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);
> +    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 0882ff6e92..7ec50157e0 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2065,7 +2065,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;
> @@ -2284,7 +2285,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
>
Maxim Levitsky Sept. 13, 2019, 2:07 p.m. UTC | #2
On Fri, 2019-09-13 at 14:01 +0000, Vladimir Sementsov-Ogievskiy wrote:
> 13.09.2019 15:59, Maxim Levitsky wrote:
> > This commit tries to clarify few function arguments,
> > and add comments describing the encrypt/decrypt interface
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >   block/qcow2-cluster.c |  9 ++++---
> >   block/qcow2-threads.c | 62 ++++++++++++++++++++++++++++++++++---------
> >   block/qcow2.c         |  5 ++--
> >   block/qcow2.h         |  8 +++---
> >   4 files changed, 61 insertions(+), 23 deletions(-)
> > 
> > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > index f09cc992af..46b0854d7e 100644
> > --- a/block/qcow2-cluster.c
> > +++ b/block/qcow2-cluster.c
> > @@ -463,8 +463,8 @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
> >   }
> >   
> >   static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
> > -                                                uint64_t src_cluster_offset,
> > -                                                uint64_t cluster_offset,
> > +                                                uint64_t guest_cluster_offset,
> > +                                                uint64_t host_cluster_offset,
> >                                                   unsigned offset_in_cluster,
> >                                                   uint8_t *buffer,
> >                                                   unsigned bytes)
> > @@ -474,8 +474,9 @@ static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
> >           assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
> >           assert((bytes & ~BDRV_SECTOR_MASK) == 0);
> >           assert(s->crypto);
> > -        if (qcow2_co_encrypt(bs, cluster_offset,
> > -                             src_cluster_offset + offset_in_cluster,
> > +        if (qcow2_co_encrypt(bs,
> > +                             host_cluster_offset + offset_in_cluster,
> > +                             guest_cluster_offset + offset_in_cluster,
> 
> oops, seems you accidentally fixed the bug, which you are going to fix in the next
> patch, as now correct offsets are given to qcow2_co_encrypt :)
> 
> and next patch no is a simple no-logic-change refactoring, so at least commit message
> should be changed.

Yep :-( I am trying my best in addition to fixing the bug, also clarify the area to
avoid this from happening again.

What do you think that I fold these two patches together after all?
Best regards,
	Maxim Levitsky
Vladimir Sementsov-Ogievskiy Sept. 13, 2019, 2:21 p.m. UTC | #3
13.09.2019 17:07, Maxim Levitsky wrote:
> On Fri, 2019-09-13 at 14:01 +0000, Vladimir Sementsov-Ogievskiy wrote:
>> 13.09.2019 15:59, Maxim Levitsky wrote:
>>> This commit tries to clarify few function arguments,
>>> and add comments describing the encrypt/decrypt interface
>>>
>>> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
>>> ---
>>>    block/qcow2-cluster.c |  9 ++++---
>>>    block/qcow2-threads.c | 62 ++++++++++++++++++++++++++++++++++---------
>>>    block/qcow2.c         |  5 ++--
>>>    block/qcow2.h         |  8 +++---
>>>    4 files changed, 61 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>>> index f09cc992af..46b0854d7e 100644
>>> --- a/block/qcow2-cluster.c
>>> +++ b/block/qcow2-cluster.c
>>> @@ -463,8 +463,8 @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
>>>    }
>>>    
>>>    static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
>>> -                                                uint64_t src_cluster_offset,
>>> -                                                uint64_t cluster_offset,
>>> +                                                uint64_t guest_cluster_offset,
>>> +                                                uint64_t host_cluster_offset,
>>>                                                    unsigned offset_in_cluster,
>>>                                                    uint8_t *buffer,
>>>                                                    unsigned bytes)
>>> @@ -474,8 +474,9 @@ static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
>>>            assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
>>>            assert((bytes & ~BDRV_SECTOR_MASK) == 0);
>>>            assert(s->crypto);
>>> -        if (qcow2_co_encrypt(bs, cluster_offset,
>>> -                             src_cluster_offset + offset_in_cluster,
>>> +        if (qcow2_co_encrypt(bs,
>>> +                             host_cluster_offset + offset_in_cluster,
>>> +                             guest_cluster_offset + offset_in_cluster,
>>
>> oops, seems you accidentally fixed the bug, which you are going to fix in the next
>> patch, as now correct offsets are given to qcow2_co_encrypt :)
>>
>> and next patch no is a simple no-logic-change refactoring, so at least commit message
>> should be changed.
> 
> Yep :-( I am trying my best in addition to fixing the bug, also clarify the area to
> avoid this from happening again.

And this is really great! I'm sorry that I've broken these things, we actually don't
use encryption (don't ask me why I've implemented threaded encryption :), so, I hope
you are not only damaged by my patches but also have some benefit.

> 
> What do you think that I fold these two patches together after all?

OK for me (but I'm not a maintainer).
Kevin Wolf Sept. 13, 2019, 2:24 p.m. UTC | #4
Am 13.09.2019 um 16:07 hat Maxim Levitsky geschrieben:
> On Fri, 2019-09-13 at 14:01 +0000, Vladimir Sementsov-Ogievskiy wrote:
> > 13.09.2019 15:59, Maxim Levitsky wrote:
> > > This commit tries to clarify few function arguments,
> > > and add comments describing the encrypt/decrypt interface
> > > 
> > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > > ---
> > >   block/qcow2-cluster.c |  9 ++++---
> > >   block/qcow2-threads.c | 62 ++++++++++++++++++++++++++++++++++---------
> > >   block/qcow2.c         |  5 ++--
> > >   block/qcow2.h         |  8 +++---
> > >   4 files changed, 61 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > > index f09cc992af..46b0854d7e 100644
> > > --- a/block/qcow2-cluster.c
> > > +++ b/block/qcow2-cluster.c
> > > @@ -463,8 +463,8 @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
> > >   }
> > >   
> > >   static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
> > > -                                                uint64_t src_cluster_offset,
> > > -                                                uint64_t cluster_offset,
> > > +                                                uint64_t guest_cluster_offset,
> > > +                                                uint64_t host_cluster_offset,
> > >                                                   unsigned offset_in_cluster,
> > >                                                   uint8_t *buffer,
> > >                                                   unsigned bytes)
> > > @@ -474,8 +474,9 @@ static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
> > >           assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
> > >           assert((bytes & ~BDRV_SECTOR_MASK) == 0);
> > >           assert(s->crypto);
> > > -        if (qcow2_co_encrypt(bs, cluster_offset,
> > > -                             src_cluster_offset + offset_in_cluster,
> > > +        if (qcow2_co_encrypt(bs,
> > > +                             host_cluster_offset + offset_in_cluster,
> > > +                             guest_cluster_offset + offset_in_cluster,
> > 
> > oops, seems you accidentally fixed the bug, which you are going to fix in the next
> > patch, as now correct offsets are given to qcow2_co_encrypt :)
> > 
> > and next patch no is a simple no-logic-change refactoring, so at least commit message
> > should be changed.
> 
> Yep :-( I am trying my best in addition to fixing the bug, also clarify the area to
> avoid this from happening again.
> 
> What do you think that I fold these two patches together after all?

No, just make sure that your refactoring patch is really just
refactoring without semantic change, i.e. make sure to preserve the bug
in this patch.

Maybe you should actually have two refactoring patches (this one without
the addition of offset_in_cluster, and patch 2) and an additional
one-liner for the actual fix.

Kevin
Maxim Levitsky Sept. 13, 2019, 2:37 p.m. UTC | #5
On Fri, 2019-09-13 at 16:24 +0200, Kevin Wolf wrote:
> Am 13.09.2019 um 16:07 hat Maxim Levitsky geschrieben:
> > On Fri, 2019-09-13 at 14:01 +0000, Vladimir Sementsov-Ogievskiy wrote:
> > > 13.09.2019 15:59, Maxim Levitsky wrote:
> > > > This commit tries to clarify few function arguments,
> > > > and add comments describing the encrypt/decrypt interface
> > > > 
> > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > > > ---
> > > >   block/qcow2-cluster.c |  9 ++++---
> > > >   block/qcow2-threads.c | 62 ++++++++++++++++++++++++++++++++++---------
> > > >   block/qcow2.c         |  5 ++--
> > > >   block/qcow2.h         |  8 +++---
> > > >   4 files changed, 61 insertions(+), 23 deletions(-)
> > > > 
> > > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > > > index f09cc992af..46b0854d7e 100644
> > > > --- a/block/qcow2-cluster.c
> > > > +++ b/block/qcow2-cluster.c
> > > > @@ -463,8 +463,8 @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
> > > >   }
> > > >   
> > > >   static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
> > > > -                                                uint64_t src_cluster_offset,
> > > > -                                                uint64_t cluster_offset,
> > > > +                                                uint64_t guest_cluster_offset,
> > > > +                                                uint64_t host_cluster_offset,
> > > >                                                   unsigned offset_in_cluster,
> > > >                                                   uint8_t *buffer,
> > > >                                                   unsigned bytes)
> > > > @@ -474,8 +474,9 @@ static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
> > > >           assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
> > > >           assert((bytes & ~BDRV_SECTOR_MASK) == 0);
> > > >           assert(s->crypto);
> > > > -        if (qcow2_co_encrypt(bs, cluster_offset,
> > > > -                             src_cluster_offset + offset_in_cluster,
> > > > +        if (qcow2_co_encrypt(bs,
> > > > +                             host_cluster_offset + offset_in_cluster,
> > > > +                             guest_cluster_offset + offset_in_cluster,
> > > 
> > > oops, seems you accidentally fixed the bug, which you are going to fix in the next
> > > patch, as now correct offsets are given to qcow2_co_encrypt :)
> > > 
> > > and next patch no is a simple no-logic-change refactoring, so at least commit message
> > > should be changed.
> > 
> > Yep :-( I am trying my best in addition to fixing the bug, also clarify the area to
> > avoid this from happening again.
> > 
> > What do you think that I fold these two patches together after all?
> 
> No, just make sure that your refactoring patch is really just
> refactoring without semantic change, i.e. make sure to preserve the bug
> in this patch.
> 
> Maybe you should actually have two refactoring patches (this one without
> the addition of offset_in_cluster, and patch 2) and an additional
> one-liner for the actual fix.
> 
> Kevin

Let me do it simplier I'll just split it to one liner patch that fixes it
and second patch that does all the refactoring.

Best regards,
	Maxim Levitsky
Vladimir Sementsov-Ogievskiy Sept. 13, 2019, 2:44 p.m. UTC | #6
13.09.2019 17:37, Maxim Levitsky wrote:
> On Fri, 2019-09-13 at 16:24 +0200, Kevin Wolf wrote:
>> Am 13.09.2019 um 16:07 hat Maxim Levitsky geschrieben:
>>> On Fri, 2019-09-13 at 14:01 +0000, Vladimir Sementsov-Ogievskiy wrote:
>>>> 13.09.2019 15:59, Maxim Levitsky wrote:
>>>>> This commit tries to clarify few function arguments,
>>>>> and add comments describing the encrypt/decrypt interface
>>>>>
>>>>> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
>>>>> ---
>>>>>    block/qcow2-cluster.c |  9 ++++---
>>>>>    block/qcow2-threads.c | 62 ++++++++++++++++++++++++++++++++++---------
>>>>>    block/qcow2.c         |  5 ++--
>>>>>    block/qcow2.h         |  8 +++---
>>>>>    4 files changed, 61 insertions(+), 23 deletions(-)
>>>>>
>>>>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>>>>> index f09cc992af..46b0854d7e 100644
>>>>> --- a/block/qcow2-cluster.c
>>>>> +++ b/block/qcow2-cluster.c
>>>>> @@ -463,8 +463,8 @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
>>>>>    }
>>>>>    
>>>>>    static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
>>>>> -                                                uint64_t src_cluster_offset,
>>>>> -                                                uint64_t cluster_offset,
>>>>> +                                                uint64_t guest_cluster_offset,
>>>>> +                                                uint64_t host_cluster_offset,
>>>>>                                                    unsigned offset_in_cluster,
>>>>>                                                    uint8_t *buffer,
>>>>>                                                    unsigned bytes)
>>>>> @@ -474,8 +474,9 @@ static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
>>>>>            assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
>>>>>            assert((bytes & ~BDRV_SECTOR_MASK) == 0);
>>>>>            assert(s->crypto);
>>>>> -        if (qcow2_co_encrypt(bs, cluster_offset,
>>>>> -                             src_cluster_offset + offset_in_cluster,
>>>>> +        if (qcow2_co_encrypt(bs,
>>>>> +                             host_cluster_offset + offset_in_cluster,
>>>>> +                             guest_cluster_offset + offset_in_cluster,
>>>>
>>>> oops, seems you accidentally fixed the bug, which you are going to fix in the next
>>>> patch, as now correct offsets are given to qcow2_co_encrypt :)
>>>>
>>>> and next patch no is a simple no-logic-change refactoring, so at least commit message
>>>> should be changed.
>>>
>>> Yep :-( I am trying my best in addition to fixing the bug, also clarify the area to
>>> avoid this from happening again.
>>>
>>> What do you think that I fold these two patches together after all?
>>
>> No, just make sure that your refactoring patch is really just
>> refactoring without semantic change, i.e. make sure to preserve the bug
>> in this patch.
>>
>> Maybe you should actually have two refactoring patches (this one without
>> the addition of offset_in_cluster, and patch 2) and an additional
>> one-liner for the actual fix.
>>
>> Kevin
> 
> Let me do it simplier I'll just split it to one liner patch that fixes it
> and second patch that does all the refactoring.
> 

[me typing actually the same suggestion in parallel, but you were the first]

I think it's the best: firstly fix the bug in a simple patch and then
refactor to make code better.

I expect something like simply
s/cluster_offset/start_of_cluster(cluster_offset + offset_in_cluster) in
qcow2_co_encrypt call from do_perform_cow_encrypt,
yes?
Kevin Wolf Sept. 13, 2019, 2:46 p.m. UTC | #7
Am 13.09.2019 um 16:37 hat Maxim Levitsky geschrieben:
> On Fri, 2019-09-13 at 16:24 +0200, Kevin Wolf wrote:
> > Am 13.09.2019 um 16:07 hat Maxim Levitsky geschrieben:
> > > On Fri, 2019-09-13 at 14:01 +0000, Vladimir Sementsov-Ogievskiy wrote:
> > > > 13.09.2019 15:59, Maxim Levitsky wrote:
> > > > > This commit tries to clarify few function arguments,
> > > > > and add comments describing the encrypt/decrypt interface
> > > > > 
> > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > > > > ---
> > > > >   block/qcow2-cluster.c |  9 ++++---
> > > > >   block/qcow2-threads.c | 62 ++++++++++++++++++++++++++++++++++---------
> > > > >   block/qcow2.c         |  5 ++--
> > > > >   block/qcow2.h         |  8 +++---
> > > > >   4 files changed, 61 insertions(+), 23 deletions(-)
> > > > > 
> > > > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > > > > index f09cc992af..46b0854d7e 100644
> > > > > --- a/block/qcow2-cluster.c
> > > > > +++ b/block/qcow2-cluster.c
> > > > > @@ -463,8 +463,8 @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
> > > > >   }
> > > > >   
> > > > >   static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
> > > > > -                                                uint64_t src_cluster_offset,
> > > > > -                                                uint64_t cluster_offset,
> > > > > +                                                uint64_t guest_cluster_offset,
> > > > > +                                                uint64_t host_cluster_offset,
> > > > >                                                   unsigned offset_in_cluster,
> > > > >                                                   uint8_t *buffer,
> > > > >                                                   unsigned bytes)
> > > > > @@ -474,8 +474,9 @@ static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
> > > > >           assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
> > > > >           assert((bytes & ~BDRV_SECTOR_MASK) == 0);
> > > > >           assert(s->crypto);
> > > > > -        if (qcow2_co_encrypt(bs, cluster_offset,
> > > > > -                             src_cluster_offset + offset_in_cluster,
> > > > > +        if (qcow2_co_encrypt(bs,
> > > > > +                             host_cluster_offset + offset_in_cluster,
> > > > > +                             guest_cluster_offset + offset_in_cluster,
> > > > 
> > > > oops, seems you accidentally fixed the bug, which you are going to fix in the next
> > > > patch, as now correct offsets are given to qcow2_co_encrypt :)
> > > > 
> > > > and next patch no is a simple no-logic-change refactoring, so at least commit message
> > > > should be changed.
> > > 
> > > Yep :-( I am trying my best in addition to fixing the bug, also clarify the area to
> > > avoid this from happening again.
> > > 
> > > What do you think that I fold these two patches together after all?
> > 
> > No, just make sure that your refactoring patch is really just
> > refactoring without semantic change, i.e. make sure to preserve the bug
> > in this patch.
> > 
> > Maybe you should actually have two refactoring patches (this one without
> > the addition of offset_in_cluster, and patch 2) and an additional
> > one-liner for the actual fix.
> > 
> > Kevin
> 
> Let me do it simplier I'll just split it to one liner patch that fixes it
> and second patch that does all the refactoring.

Fine with me, as long as the fix is kept separate from the refactoring.

Kevin
Kevin Wolf Sept. 13, 2019, 2:51 p.m. UTC | #8
Am 13.09.2019 um 16:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 13.09.2019 17:37, Maxim Levitsky wrote:
> > On Fri, 2019-09-13 at 16:24 +0200, Kevin Wolf wrote:
> >> Am 13.09.2019 um 16:07 hat Maxim Levitsky geschrieben:
> >>> On Fri, 2019-09-13 at 14:01 +0000, Vladimir Sementsov-Ogievskiy wrote:
> >>>> 13.09.2019 15:59, Maxim Levitsky wrote:
> >>>>> This commit tries to clarify few function arguments,
> >>>>> and add comments describing the encrypt/decrypt interface
> >>>>>
> >>>>> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> >>>>> ---
> >>>>>    block/qcow2-cluster.c |  9 ++++---
> >>>>>    block/qcow2-threads.c | 62 ++++++++++++++++++++++++++++++++++---------
> >>>>>    block/qcow2.c         |  5 ++--
> >>>>>    block/qcow2.h         |  8 +++---
> >>>>>    4 files changed, 61 insertions(+), 23 deletions(-)
> >>>>>
> >>>>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> >>>>> index f09cc992af..46b0854d7e 100644
> >>>>> --- a/block/qcow2-cluster.c
> >>>>> +++ b/block/qcow2-cluster.c
> >>>>> @@ -463,8 +463,8 @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
> >>>>>    }
> >>>>>    
> >>>>>    static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
> >>>>> -                                                uint64_t src_cluster_offset,
> >>>>> -                                                uint64_t cluster_offset,
> >>>>> +                                                uint64_t guest_cluster_offset,
> >>>>> +                                                uint64_t host_cluster_offset,
> >>>>>                                                    unsigned offset_in_cluster,
> >>>>>                                                    uint8_t *buffer,
> >>>>>                                                    unsigned bytes)
> >>>>> @@ -474,8 +474,9 @@ static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
> >>>>>            assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
> >>>>>            assert((bytes & ~BDRV_SECTOR_MASK) == 0);
> >>>>>            assert(s->crypto);
> >>>>> -        if (qcow2_co_encrypt(bs, cluster_offset,
> >>>>> -                             src_cluster_offset + offset_in_cluster,
> >>>>> +        if (qcow2_co_encrypt(bs,
> >>>>> +                             host_cluster_offset + offset_in_cluster,
> >>>>> +                             guest_cluster_offset + offset_in_cluster,
> >>>>
> >>>> oops, seems you accidentally fixed the bug, which you are going to fix in the next
> >>>> patch, as now correct offsets are given to qcow2_co_encrypt :)
> >>>>
> >>>> and next patch no is a simple no-logic-change refactoring, so at least commit message
> >>>> should be changed.
> >>>
> >>> Yep :-( I am trying my best in addition to fixing the bug, also clarify the area to
> >>> avoid this from happening again.
> >>>
> >>> What do you think that I fold these two patches together after all?
> >>
> >> No, just make sure that your refactoring patch is really just
> >> refactoring without semantic change, i.e. make sure to preserve the bug
> >> in this patch.
> >>
> >> Maybe you should actually have two refactoring patches (this one without
> >> the addition of offset_in_cluster, and patch 2) and an additional
> >> one-liner for the actual fix.
> >>
> >> Kevin
> > 
> > Let me do it simplier I'll just split it to one liner patch that fixes it
> > and second patch that does all the refactoring.
> > 
> 
> [me typing actually the same suggestion in parallel, but you were the first]
> 
> I think it's the best: firstly fix the bug in a simple patch and then
> refactor to make code better.
> 
> I expect something like simply
> s/cluster_offset/start_of_cluster(cluster_offset + offset_in_cluster) in
> qcow2_co_encrypt call from do_perform_cow_encrypt,
> yes?

Yes, I think that's the right fix.

Kevin

Patch
diff mbox series

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index f09cc992af..46b0854d7e 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -463,8 +463,8 @@  static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
 }
 
 static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
-                                                uint64_t src_cluster_offset,
-                                                uint64_t cluster_offset,
+                                                uint64_t guest_cluster_offset,
+                                                uint64_t host_cluster_offset,
                                                 unsigned offset_in_cluster,
                                                 uint8_t *buffer,
                                                 unsigned bytes)
@@ -474,8 +474,9 @@  static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
         assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
         assert((bytes & ~BDRV_SECTOR_MASK) == 0);
         assert(s->crypto);
-        if (qcow2_co_encrypt(bs, cluster_offset,
-                             src_cluster_offset + offset_in_cluster,
+        if (qcow2_co_encrypt(bs,
+                             host_cluster_offset + offset_in_cluster,
+                             guest_cluster_offset + offset_in_cluster,
                              buffer, bytes) < 0) {
             return false;
         }
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 3b1e63fe41..9646243a9b 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,54 @@  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_cluster_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);
+    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);
+    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 0882ff6e92..7ec50157e0 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2065,7 +2065,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;
@@ -2284,7 +2285,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