Re: [PATCH v6 07/14] block/crypto: implement the encryption key management

[Daniel P . Berrangé]

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. Having said
that due to previous QEMU bugs, they unconditonally pass the arg to
qemu-img to explicitly disable locking


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|