Re: [PATCH 2/4] crypto: aead - Retain alg refcount in crypto_grab_aead

[Eric Biggers <ebiggers@kernel.org>]

On Fri, Dec 06, 2019 at 02:38:37PM +0800, Herbert Xu wrote:
> This patch changes crypto_grab_aead to retain the reference count
> on the algorithm.  This is because the caller needs to access the
> algorithm parameters and without the reference count the algorithm
> can be freed at any time.
> 
> All callers have been modified to drop the reference count instead.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> ---
> 
>  crypto/aead.c                   |    7 +------
>  crypto/ccm.c                    |    3 +++
>  crypto/cryptd.c                 |    3 ++-
>  crypto/echainiv.c               |    1 +
>  crypto/essiv.c                  |   10 +++++++++-
>  crypto/gcm.c                    |    6 ++++++
>  crypto/geniv.c                  |    1 +
>  crypto/pcrypt.c                 |    3 +++
>  crypto/seqiv.c                  |    1 +
>  include/crypto/internal/aead.h  |    5 +++++
>  include/crypto/internal/geniv.h |    7 +++++++
>  11 files changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/crypto/aead.c b/crypto/aead.c
> index f548a5c3f74d..47f16d139e8e 100644
> --- a/crypto/aead.c
> +++ b/crypto/aead.c
> @@ -210,13 +210,8 @@ static const struct crypto_type crypto_aead_type = {
>  int crypto_grab_aead(struct crypto_aead_spawn *spawn, const char *name,
>  		     u32 type, u32 mask)
>  {
> -	int err;
> -
>  	spawn->base.frontend = &crypto_aead_type;
> -	err = crypto_grab_spawn(&spawn->base, name, type, mask);
> -	if (!err)
> -		crypto_mod_put(spawn->base.alg);
> -	return err;
> +	return crypto_grab_spawn(&spawn->base, name, type, mask);
>  }
>  EXPORT_SYMBOL_GPL(crypto_grab_aead);
>  
> diff --git a/crypto/ccm.c b/crypto/ccm.c
> index 380eb619f657..6f555342a4a7 100644
> --- a/crypto/ccm.c
> +++ b/crypto/ccm.c
> @@ -819,11 +819,14 @@ static int crypto_rfc4309_create(struct crypto_template *tmpl,
>  	if (err)
>  		goto out_drop_alg;
>  
> +	aead_alg_put(alg);
> +
>  out:
>  	return err;
>  
>  out_drop_alg:
>  	crypto_drop_aead(spawn);
> +	aead_alg_put(alg);
>  out_free_inst:
>  	kfree(inst);
>  	goto out;

This approach seems too error-prone, since the prototype of crypto_grab_aead()
doesn't give any indication that it takes a reference to the algorithm which the
caller *must* drop.

How about returning the alg pointer in the last argument, similar to
skcipher_alloc_instance_simple()?  I know you sent a patch to remove that
argument, but I think it's better to have it...

That would actually simplify the code, since then the callers wouldn't need to
call crypto_spawn_aead_alg() to get the alg pointer.

Likewise for skcipher and akcipher.

> diff --git a/crypto/essiv.c b/crypto/essiv.c
> index 808f2b362106..08e0209d1b09 100644
> --- a/crypto/essiv.c
> +++ b/crypto/essiv.c
> @@ -622,6 +622,12 @@ static int essiv_create(struct crypto_template *tmpl, struct rtattr **tb)
>  		goto out_free_hash;
>  
>  	crypto_mod_put(_hash_alg);
> +
> +	if (type == CRYPTO_ALG_TYPE_SKCIPHER)
> +		;
> +	else
> +		aead_alg_put(aead_alg);

Or more conventionally:

	if (type != CRYPTO_ALG_TYPE_SKCIPHER)
		aead_alg_put(aead_alg);

> @@ -629,8 +635,10 @@ static int essiv_create(struct crypto_template *tmpl, struct rtattr **tb)
>  out_drop_skcipher:
>  	if (type == CRYPTO_ALG_TYPE_SKCIPHER)
>  		crypto_drop_skcipher(&ictx->u.skcipher_spawn);
> -	else
> +	else {
>  		crypto_drop_aead(&ictx->u.aead_spawn);
> +		aead_alg_put(aead_alg);
> +	}

Should have braces around the 'if' clause too.

- Eric