On Thu, 2020-05-14 at 16:32 +0200, Max Reitz wrote:
> On 14.05.20 16:14, Daniel P. Berrangé wrote:
> > 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 <mlevi...@redhat.com>
> > > > Reviewed-by: Daniel P. Berrangé <berra...@redhat.com>
> > > > ---
> > > > 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.
>
> That might actually be a problem then, because this may cause sporadic
> failure when trying to change (amend) keyslots; while qemu-img info
> holds the CONSISTENT_READ permission, the amend process can’t unshare
> it. That might lead to hard-to-track-down bugs.
>
> > Having said
> > that due to previous QEMU bugs, they unconditonally pass the arg to
> > qemu-img to explicitly disable locking
>
> Well, then it doesn’t matter in this case. But still something to
> consider, probably.
>
> Max
>
So I understand correctly that I should leave the patch as is?
Thanks for the review!
Best regards,
Maxim Levitsky
