[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v6 07/14] block/crypto: implement the encryption key manageme
From: |
Max Reitz |
Subject: |
Re: [PATCH v6 07/14] block/crypto: implement the encryption key management |
Date: |
Thu, 14 May 2020 16:32:08 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 |
On 14.05.20 16:14, Daniel P. Berrangé wrote:
> On Thu, May 14, 2020 at 04:09:59PM +0200, Max Reitz wrote:
>> On 10.05.20 15:40, Maxim Levitsky wrote:
>>> This implements the encryption key management using the generic code in
>>> qcrypto layer and exposes it to the user via qemu-img
>>>
>>> This code adds another 'write_func' because the initialization
>>> write_func works directly on the underlying file, and amend
>>> works on instance of luks device.
>>>
>>> This commit also adds a 'hack/workaround' I and Kevin Wolf (thanks)
>>> made to make the driver both support write sharing (to avoid breaking the
>>> users),
>>> and be safe against concurrent metadata update (the keyslots)
>>>
>>> Eventually the write sharing for luks driver will be deprecated
>>> and removed together with this hack.
>>>
>>> The hack is that we ask (as a format driver) for BLK_PERM_CONSISTENT_READ
>>> and then when we want to update the keys, we unshare that permission.
>>> So if someone else has the image open, even readonly, encryption
>>> key update will fail gracefully.
>>>
>>> Also thanks to Daniel Berrange for the idea of
>>> unsharing read, rather that write permission which allows
>>> to avoid cases when the other user had opened the image read-only.
>>>
>>> Signed-off-by: Maxim Levitsky <address@hidden>
>>> Reviewed-by: Daniel P. Berrangé <address@hidden>
>>> ---
>>> block/crypto.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++--
>>> block/crypto.h | 34 +++++++++++++
>>> 2 files changed, 158 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/block/crypto.c b/block/crypto.c
>>> index 2e16b62bdc..b14cb0ff06 100644
>>> --- a/block/crypto.c
>>> +++ b/block/crypto.c
>>
>> [...]
>>
>>> +static void
>>> +block_crypto_child_perms(BlockDriverState *bs, BdrvChild *c,
>>> + const BdrvChildRole *role,
>>> + BlockReopenQueue *reopen_queue,
>>> + uint64_t perm, uint64_t shared,
>>> + uint64_t *nperm, uint64_t *nshared)
>>> +{
>>> +
>>> + BlockCrypto *crypto = bs->opaque;
>>> +
>>> + bdrv_filter_default_perms(bs, c, role, reopen_queue,
>>> + perm, shared, nperm, nshared);
>>> + /*
>>> + * Ask for consistent read permission so that if
>>> + * someone else tries to open this image with this permission
>>> + * neither will be able to edit encryption keys, since
>>> + * we will unshare that permission while trying to
>>> + * update the encryption keys
>>> + */
>>> + if (!(bs->open_flags & BDRV_O_NO_IO)) {
>>> + *nperm |= BLK_PERM_CONSISTENT_READ;
>>> + }
>>
>> I’m not sure this is important, because this really means we won’t do
>> I/O. Its only relevant use in this case is for qemu-img info. Do we
>> really care if someone edits the key slots while qemu-img info is
>> processing?
>
> FWIW, OpenStack runs qemu-img info in a periodic background job, so
> it can be concurrent with anything else they are running.
That might actually be a problem then, because this may cause sporadic
failure when trying to change (amend) keyslots; while qemu-img info
holds the CONSISTENT_READ permission, the amend process can’t unshare
it. That might lead to hard-to-track-down bugs.
> Having said
> that due to previous QEMU bugs, they unconditonally pass the arg to
> qemu-img to explicitly disable locking
Well, then it doesn’t matter in this case. But still something to
consider, probably.
Max
signature.asc
Description: OpenPGP digital signature
- [PATCH v6 08/14] block/qcow2: extend qemu-img amend interface with crypto options, (continued)
[PATCH v6 12/14] block/crypto: implement blockdev-amend, Maxim Levitsky, 2020/05/10
[PATCH v6 06/14] block/crypto: rename two functions, Maxim Levitsky, 2020/05/10
[PATCH v6 11/14] block/core: add generic infrastructure for x-blockdev-amend qmp command, Maxim Levitsky, 2020/05/10
[PATCH v6 13/14] block/qcow2: implement blockdev-amend, Maxim Levitsky, 2020/05/10
[PATCH v6 14/14] iotests: add tests for blockdev-amend, Maxim Levitsky, 2020/05/10