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

[Maxim Levitsky]

On Fri, 2019-11-08 at 14:12 +0100, Max Reitz wrote:
> 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 <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.
> 
> Maybe you forgot to mention anywhere that I should.

Now I remember that this patch was in the original re-factoring patch series,
and for some reason it was dropped.
Its now locally in my git tree, so I forgot that it wasn't in this patch series
already.

Sorry for the mess, I'll soon resend the updated patch series.

Best regards,
        Maxim Levitsky