Re: [PATCH v2 05/11] block/crypto: implement the encryption key management

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3750 bytes --]

On 08.11.19 12:04, Maxim Levitsky wrote:
> On Fri, 2019-11-08 at 11:49 +0100, Max Reitz wrote:
>> On 08.11.19 10:30, Maxim Levitsky wrote:
>>> On Fri, 2019-10-04 at 20:41 +0200, Max Reitz wrote:
>>>> On 13.09.19 00:30, Maxim Levitsky wrote:
>>>>> This implements the encryption key management
>>>>> using the generic code in qcrypto layer
>>>>> (currently only for qemu-img amend)
>>>>>
>>>>> This code adds another 'write_func' because the initialization
>>>>> write_func works directly on the underlying file,
>>>>> because during the creation, there is no open instance
>>>>> of the luks driver, but during regular use, we have it,
>>>>> and should use it instead.
>>>>>
>>>>>
>>>>> This commit also adds a	'hack/workaround' I and	Kevin Wolf (thanks)
>>>>> made to	make the driver	still support write sharing,
>>>>> but be safe against concurrent  metadata update (the keys)
>>>>> Eventually 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 always
>>>>> (technically always unless opened with BDRV_O_NO_IO)
>>>>>
>>>>> and then when we want to update	the keys, we
>>>>> unshare	that permission. So if someone else
>>>>> has the	image open, even readonly, this	will fail.
>>>>>
>>>>> Also thanks to Daniel Berrange for the variant of
>>>>> that hack that involves	asking for read,
>>>>> rather that write permission
>>>>>
>>>>> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
>>>>> ---
>>>>>  block/crypto.c | 118 +++++++++++++++++++++++++++++++++++++++++++++++--
>>>>>  1 file changed, 115 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/block/crypto.c b/block/crypto.c
>>>>> index a6a3e1f1d8..f42fa057e6 100644
>>>>> --- a/block/crypto.c
>>>>> +++ b/block/crypto.c
>>>>> @@ -36,6 +36,7 @@ typedef struct BlockCrypto BlockCrypto;
>>>>>  
>>>>>  struct BlockCrypto {
>>>>>      QCryptoBlock *block;
>>>>> +    bool updating_keys;
>>>>>  };
>>>>>  
>>>>>  
>>>>> @@ -70,6 +71,24 @@ static ssize_t block_crypto_read_func(QCryptoBlock *block,
>>>>>      return ret;
>>>>>  }
>>>>>  
>>>>> +static ssize_t block_crypto_write_func(QCryptoBlock *block,
>>>>> +                                       size_t offset,
>>>>> +                                       const uint8_t *buf,
>>>>> +                                       size_t buflen,
>>>>> +                                       void *opaque,
>>>>> +                                       Error **errp)
>>>>
>>>> There’s already a function of this name for creation.
>>>
>>> There is a long story why two write functions are needed.
>>> i tried to use only one, but at the end I and Daniel both agreed
>>> that its just better to have two functions.
>>>
>>> The reason is that during creation, the luks BlockDriverState doesn't exist yet,
>>> and so the creation routine basically just writes to the underlying protocol driver.
>>>
>>> Thats is why the block_crypto_create_write_func receives a BlockBackend pointer,
>>> to which the BlockDriverState of the underlying protocol driver is inserted.
>>>
>>>
>>> On the other hand, for amend, the luks block device is open, and it only knows
>>> about its own BlockDriverState, and thus the io should be done on bs->file
>>>
>>> So instead of trying to coerce a single callback to do both of this,
>>> we decided to just have a little code duplication.
>>
>> I meant: This doesn’t compile.  There’s already another function of this
>> name.
>>
> 
> You probably didn't apply the 'block-crypto: misc refactoring' patch, 
> or I forgot to send it.

Maybe you forgot to mention anywhere that I should.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]