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

[Max Reitz]

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 <mlevitsk@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Reviewed-by: Max Reitz <mreitz@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*)

> +                         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

signature.asc
Description: OpenPGP digital signature