Re: [Qemu-devel] [PATCH 00/13] RFC: luks/encrypted qcow2 key management

[Maxim Levitsky]

On Tue, 2019-08-20 at 19:59 +0200, Max Reitz wrote:
> On 14.08.19 22:22, Maxim Levitsky wrote:
> 
> [...]
> 
> > Testing. This was lightly tested with manual testing and with few iotests 
> > that I prepared.
> > I haven't yet tested fully the write sharing behavior, nor did I run the 
> > whole iotests
> > suite to see if this code causes some regressions. Since I will need 
> > probably
> > to rewrite some chunks of it to change to 'amend' interface, I decided to 
> > post it now,
> > to see if you have other ideas/comments to add.
> 
> I can see that, because half of the qcow2 tests that contain the string
> “secret” break:
> 
> Failures: 087 134 158 178 188 198 206
> Failed 7 of 13 tests
> 
> Also, 210 when run with -luks.
> 
> Some are just due to different test outputs (because you change
> _filter_img_create to filter some encrypt.* parameters), but some of
> them are due to aborts.  All of them look like different kinds of heap
> corruptions.
> 
> 
> I can fully understand not running all iotests (because only the
> maintainers do that before pull requests), but just running the iotests
> that immediately concern a series seems prudent to me (unless the series
> is trivial).
> 
> (Just “(cd tests/qemu-iotests && grep -l secret ???)” tells you which
> tests to run that may concern themselves with qcow2 encryption, for
> example.)
> 
> 
> So I suppose I’ll stop reviewing the series in detail and just give a
> more cursory glance from now on.

Sorry about that! I posted this as RFC, and the reason it is mostly done as 
opposed to typical RFC which might not
even contain any code was that for most of the time I was sure that API of this 
is straightforward and won't need
any significant discussion, and in the last minute after I discussed with Kevin 
on IRC one 
obscure case of block backend permissions that was failing, he told me about 
the amend interface.
Next time I guess, when new a API is involved, I will post an API RFC first 
always and then start the implementation.

I fixed both issues that iotests uncovered locally, now all luks and most qcow2 
tests pass 
(118 and 194 sometimes fail with qcow2, and this happens regardless of my 
patches, and same for 162 which seems to fail
always now, also regardless of my patches.
I have a git head after the merge window opened so probably some bugs were 
added, and maybe already fixed)


The first issue was in 'qcrypto-luks: implement the encryption key management'
where I accidentally stored the name of the secret without strdup'ing in the 
create flow, so I got double free,
which indeed caused the heap corruptions you have seen.

Basically this line:
    luks->secret = options->u.luks.key_secret;

The second issue as you mention is indeed the change in filters I did. Do you 
agree with that change btw?
If you ask me, I would even change the filter further and filter all the image 
options from the qemu command line since these are test inputs anyway.
This allowed me to have the same test for both luks and qcow2 luks encrypted 
test.

Also I didn't even expect you to run the iotests for me, but
rather just wanted a general RFC level feedback on the whole thing, that is why 
I even mentioned that I didn't run them.
So sorry for the trouble I caused!

I btw don't agree with you that only maintainers need to run all the iotests 
fully. 
I think that the patch submitter  should run all the tests that he can to catch 
as many problems as he can,
_unless_ of course this is an RFC.


Best regards,
Thanks for the review,
Sorry again for the trouble,

Maxim Levitsky