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
signature.asc
Description: OpenPGP digital signature