On Mon, 2020-06-08 at 14:14 +0200, Max Reitz wrote: > On 08.06.20 11: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> > > Reviewed-by: Max Reitz <mre...@redhat.com> > > --- > > block/crypto.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++-- > > block/crypto.h | 34 +++++++++++++ > > 2 files changed, 161 insertions(+), 3 deletions(-) > > > > diff --git a/block/crypto.c b/block/crypto.c > > index 1960b47ceb..b9c40e6922 100644 > > --- a/block/crypto.c > > +++ b/block/crypto.c > > [...] > > > +static void > > +block_crypto_child_perms(BlockDriverState *bs, BdrvChild *c, > > + const BdrvChildRole role, > > Well, it isn’t wrong to have this be a const, nor is it against any > coding guidelines. While I do believe this was an accident, I also > think that in fact, maybe being strict about const-ness is what we > should’ve done everywhere from the start. > > So this is not a complaint, quite the contrary. > > (I felt it was interesting enough to warrant this mail. *shrug*)
Yep, that was 100% accident I confess. Best regards, Maxim Levitsky > > > + BlockReopenQueue *reopen_queue, > > + uint64_t perm, uint64_t shared, > > + uint64_t *nperm, uint64_t *nshared) > > +{ > > + > > + BlockCrypto *crypto = bs->opaque; > > + > > + bdrv_default_perms(bs, c, role, reopen_queue, perm, shared, nperm, > > nshared); > > + > > + /* > > + * For backward compatibility, manually share the write > > + * and resize permission > > + */ > > + *nshared |= (BLK_PERM_WRITE | BLK_PERM_RESIZE); > > + /* > > + * Since we are not fully a format driver, don't always request > > + * the read/resize permission but only when explicitly > > + * requested > > + */ > > + *nperm &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE); > > + *nperm |= perm & (BLK_PERM_WRITE | BLK_PERM_RESIZE); > > Looks good, thanks! > > Max >