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 <[email protected]> > > Reviewed-by: Daniel P. Berrangé <[email protected]> > > --- > > 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 :|
