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

[Max Reitz <mreitz@redhat.com>]

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

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.

Max


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