Re: [PATCH v7 00/14] LUKS: encryption slot management using amend interface

[Max Reitz]

On 18.05.20 14:20, Maxim Levitsky wrote:
> Hi!
> Here is the updated series of my patches, incorporating all the feedback I 
> received.

You asked me on IRC what to do to get this series to move forward;
considering I don’t think there were objections from anyone but me on
v6, there are no further (substantial) objections from anyone on v7, and
I gave all feedback I had on v6, I don’t think there’s much you can do
right now.  (Sorry for the delay, but, well, I was on PTO as you know.)

As usual, I’ll try to get around to see whether I can rebase your series
myself (because Dan hinted at some conflicts), and whether I feel like
my comments were appropriately addressed (which I have little doubt
about).  That’s the plan.

Note from a couple minutes later: From what I can see, the rebase
conflicts don’t look too wild, but I don’t feel quite comfortable
addressing all of them.  There’s a functional one I could address in
qemu-img.c (patch 3), where we need to bump OPTION_FORCE from 269 to
276.  I could do that, but that’s not the only one, unfortunately.

Patch 7 needs a bit more extensive modification: First, we need
%s/bdrv_filter_default_perms/bdrv_default_perms/.  Second, this will
still not compile, because the .bdrv_child_perm interface has changed.
BdrvChildRole is now an integer, so we also need
s/const BdrvChildRole \*/BdrvChildRole /.
(That gives us the nice side effect of being able to align the second
line of the bdrv_default_perms() parameters (called from
block_crypto_child_perms()) on the opening parenthesis.)

Third, it becomes really interesting.  With these changes, it would be
wrong, because bdrv_default_perms() will then not use the permissions
for a filter but for an image file with metadata – because that’s what
the LUKS file child is.

But that’s actually a bug that’s already there (and that I introduced).
 It makese sense to fix it in your series here, because to fix it we
need a dedicated block_crypto_child_perms() function anyway.

So, well.  Do we want to cheat?  Just let block_crypto_child_perms()
call bdrv_default_perms() with role=BDRV_CHILD_FILTERED?  That would be
the previous behavior, but it would also be bad cheating.

The comment that exists (before patch 7) above
bdrv_crypto_luks.bdrv_child_perm says that we just want to allow
share-rw=on, and we could achieve that simply by force-sharing the WRITE
permission after invoking bdrv_default_perms() with the actual role
(which is BDRV_CHILD_IMAGE).

But what about the other stuff that sets
bdrv_default_perms_for_storage() apart from bdrv_filter_default_perms()?
 I.e., force-taking WRITE and RESIZE permissions if the file is
writable; force-taking the CONSISTENT_READ permission because we need
the metadata; and unsharing the RESIZE permission?

I think the best thing to do would be to call bdrv_default_perms() with
the @role passed to block_crypto_child_perms() (i.e., BDRV_CHILD_IMAGE),
and then relaxing the permissions, that is to share the WRITE and RESIZE
persmissions, and to perhaps restore the WRITE and RESIZE permissions
from what’s given to block_crypto_child_perms() (i.e., *nperm &= ~(WRITE
| RESIZE); *nperm |= perm & (WRITE | RESIZE);), because LUKS doesn’t
need them unless the image may actually be written.

(OTOH, force-taking CONSISTENT_READ seems entirely reasonable.)

Max