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

[Maxim Levitsky]

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 <address@hidden>
> > > > ---
> > > >  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.
All that patch does is to rename block_crypto_write_func to 
block_crypto_create_write_func
and same (for consistency) for block_crypto_init_func -> 
block_crypto_create_init_func

And then in this patch I add the block_crypto_write_func, to be used for 
anything
but creation code, together with existing block_crypto_read_func which is 
already
not used for creation.


Best regards,
        Maxim Levitsky