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 <mlevi...@redhat.com> > > --- > > 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
