[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v7 00/14] LUKS: encryption slot management using amend interf
From: |
Max Reitz |
Subject: |
Re: [PATCH v7 00/14] LUKS: encryption slot management using amend interface |
Date: |
Tue, 2 Jun 2020 18:29:38 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 |
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
- Re: [PATCH v7 00/14] LUKS: encryption slot management using amend interface,
Max Reitz <=