Re: [PATCH v6 32/33] crypto: delegate permission functions to JobDriver .pre_run

[Hanna Reitz <hreitz@redhat.com>]

On 21.01.22 18:05, Emanuele Giuseppe Esposito wrote:
> block_crypto_amend_options_generic_luks uses the block layer
> permission API, therefore it should be called with the BQL held.
>
> However, the same function is being called by two BlockDriver
> callbacks: bdrv_amend_options (under BQL) and bdrv_co_amend (I/O).
>
> The latter is I/O because it is invoked by block/amend.c's
> blockdev_amend_run(), a .run callback of the amend JobDriver
>
> Therefore we want to 1) change block_crypto driver
> to use the permission API only when the BQL is held, and
> 2) use the .pre_run JobDriver callback to check for
> permissions before switching to the job aiocontext. This has also
> the benefit of applying the same permission operation to all
> amend implementations, not only luks.
>
> Remove the permission check in block_crypto_amend_options_generic_luks()
> and:
> - Add helper functions block_crypto_amend_options_{prepare/cleanup}
>    that take care of checking permissions in
>    block_crypto_amend_options_luks(), so when it is under BQL, and
>
> - Use job->pre_run() and job->clean() to do the same thing when
>    we are in an iothread, by performing these checks before the
>    job runs in its aiocontext. So far job->pre_run() is only defined
>    but not called in job_start(), now it is the moment to use it.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block/crypto.c | 57 ++++++++++++++++++++++++++++++++------------------
>   job.c          | 13 ++++++++++++
>   2 files changed, 50 insertions(+), 20 deletions(-)
>
> diff --git a/block/crypto.c b/block/crypto.c
> index f5e0c7b7c0..bdb4ba5664 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -791,6 +791,28 @@ block_crypto_amend_cleanup(BlockDriverState *bs)
>       crypto->updating_keys = false;
>   }
>   
> +static int
> +block_crypto_amend_options_prepare(BlockDriverState *bs,
> +                                   Error **errp)
> +{
> +    BlockCrypto *crypto = bs->opaque;
> +
> +    /* apply for exclusive read/write permissions to the underlying file*/
> +    crypto->updating_keys = true;
> +    return bdrv_child_refresh_perms(bs, bs->file, errp);
> +}
> +
> +static int
> +block_crypto_amend_options_cleanup(BlockDriverState *bs,
> +                                   Error **errp)
> +{
> +    BlockCrypto *crypto = bs->opaque;
> +
> +    /* release exclusive read/write permissions to the underlying file*/
> +    crypto->updating_keys = false;
> +    return bdrv_child_refresh_perms(bs, bs->file, errp);
> +}
> +

Now that I have this patch applied, it does look like it would be nicer 
if we could skip adding these functions and just reuse 
block_crypto_amend_{pre_run,cleanup}() (which would require them to call 
bdrv_child_refresh_perms()).

>   static int
>   block_crypto_amend_options_generic_luks(BlockDriverState *bs,
>                                           QCryptoBlockAmendOptions *amend_options,
> @@ -798,30 +820,17 @@ block_crypto_amend_options_generic_luks(BlockDriverState *bs,
>                                           Error **errp)
>   {
>       BlockCrypto *crypto = bs->opaque;
> -    int ret;
>   
>       assert(crypto);
>       assert(crypto->block);
>   
> -    /* apply for exclusive read/write permissions to the underlying file*/
> -    crypto->updating_keys = true;
> -    ret = bdrv_child_refresh_perms(bs, bs->file, errp);
> -    if (ret) {
> -        goto cleanup;
> -    }
> -
> -    ret = qcrypto_block_amend_options(crypto->block,
> -                                      block_crypto_read_func,
> -                                      block_crypto_write_func,
> -                                      bs,
> -                                      amend_options,
> -                                      force,
> -                                      errp);
> -cleanup:
> -    /* release exclusive read/write permissions to the underlying file*/
> -    crypto->updating_keys = false;
> -    bdrv_child_refresh_perms(bs, bs->file, errp);
> -    return ret;
> +    return qcrypto_block_amend_options(crypto->block,
> +                                       block_crypto_read_func,
> +                                       block_crypto_write_func,
> +                                       bs,
> +                                       amend_options,
> +                                       force,
> +                                       errp);
>   }
>   
>   static int
> @@ -847,8 +856,16 @@ block_crypto_amend_options_luks(BlockDriverState *bs,
>       if (!amend_options) {
>           goto cleanup;
>       }
> +
> +    ret = block_crypto_amend_options_prepare(bs, errp);
> +    if (ret) {
> +        goto perm_cleanup;
> +    }
>       ret = block_crypto_amend_options_generic_luks(bs, amend_options,
>                                                     force, errp);
> +
> +perm_cleanup:
> +    block_crypto_amend_options_cleanup(bs, errp);

Uh, pre-existing but still dangerous.  We must not pass @errp here, 
because it may (and if we come from ..._prepare() failing, s/may/will/) 
already contain some error, and then, if this fails (which it very 
likely will not), we will get an assertion failure in error_setv().

We could decide that this must not fail and pass &error_abort (but then 
block_crypto_amend_options_cleanup() should do that), or we pass some 
new guaranteed-empty pointer and report it.

In any case, we should probably have 
block_crypto_amend_options_cleanup() (or block_crypto_amend_cleanup()) 
handle this and have that function return void and no error, so we don’t 
have to worry about that here at all.

>   cleanup:
>       qapi_free_QCryptoBlockAmendOptions(amend_options);
>       return ret;
> diff --git a/job.c b/job.c
> index 39bf511949..cf0dc9325a 100644
> --- a/job.c
> +++ b/job.c
> @@ -967,11 +967,24 @@ static void coroutine_fn job_co_entry(void *opaque)
>       aio_bh_schedule_oneshot(qemu_get_aio_context(), job_exit, job);
>   }
>   
> +static int job_pre_run(Job *job)
> +{
> +    assert(qemu_in_main_thread());
> +    if (job->driver->pre_run) {
> +        return job->driver->pre_run(job, &job->err);
> +    }
> +
> +    return 0;
> +}
> +
>   void job_start(Job *job)
>   {
>       assert(job && !job_started(job) && job->paused &&
>              job->driver && job->driver->run);
>       job->co = qemu_coroutine_create(job_co_entry, job);
> +    if (job_pre_run(job)) {
> +        return;

Do we not need to announce the error somehow?  Like, perhaps 
job_pre_run() should set job->ret to the value returned by .pre_run() 
(like job_co_entry() does it for .run()), and then call job_completed() 
on error (or even job_exit()? I’m not sure :/).

The way it is, it looks like the job will just basically leak on error, 
and never complete.

Hanna

> +    }
>       job->pause_count--;
>       job->busy = true;
>       job->paused = false;