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

[Eric Biggers <ebiggers@kernel.org>]

On Sat, Dec 07, 2019 at 11:30:59AM +0800, Herbert Xu wrote:
> On Fri, Dec 06, 2019 at 02:41:55PM -0800, Eric Biggers wrote:
> >
> > 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.
> 
> Fair point.
> 
> > 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...
> 
> You probably guessed that I don't really like returning two objects
> from the same function :)
> 
> So how about this: we let the Crypto API manage the refcount and
> hide it from all the users.  Something like this patch:
> 
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index adb516380be9..34473ab992f2 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -563,6 +563,7 @@ int crypto_register_instance(struct crypto_template *tmpl,
>  			     struct crypto_instance *inst)
>  {
>  	struct crypto_larval *larval;
> +	struct crypto_spawn *spawn;
>  	int err;
>  
>  	err = crypto_check_alg(&inst->alg);
> @@ -588,6 +589,9 @@ int crypto_register_instance(struct crypto_template *tmpl,
>  	if (IS_ERR(larval))
>  		goto err;
>  
> +	hlist_for_each_entry(spawn, &inst->spawn_list, spawn_list)
> +		crypto_mod_put(spawn->alg);
> +
>  	crypto_wait_for_test(larval);
>  	err = 0;
>  
> @@ -623,6 +627,7 @@ int crypto_init_spawn(struct crypto_spawn *spawn, struct crypto_alg *alg,
>  
>  	spawn->inst = inst;
>  	spawn->mask = mask;
> +	hlist_add_head(&spawn->spawn_list, &inst->spawn_list);
>  
>  	down_write(&crypto_alg_sem);
>  	if (!crypto_is_moribund(alg)) {
> @@ -674,6 +679,9 @@ void crypto_drop_spawn(struct crypto_spawn *spawn)
>  	if (!spawn->dead)
>  		list_del(&spawn->list);
>  	up_write(&crypto_alg_sem);
> +
> +	if (hlist_unhashed(&spawn->inst->list))
> +		crypto_mod_put(spawn->alg);
>  }
>  EXPORT_SYMBOL_GPL(crypto_drop_spawn);
>  
> diff --git a/include/crypto/algapi.h b/include/crypto/algapi.h
> index 771a295ac755..284e96f2eda2 100644
> --- a/include/crypto/algapi.h
> +++ b/include/crypto/algapi.h
> @@ -48,6 +48,7 @@ struct crypto_instance {
>  
>  	struct crypto_template *tmpl;
>  	struct hlist_node list;
> +	struct hlist_head spawn_list;
>  
>  	void *__ctx[] CRYPTO_MINALIGN_ATTR;
>  };
> @@ -66,6 +67,7 @@ struct crypto_template {
>  
>  struct crypto_spawn {
>  	struct list_head list;
> +	struct hlist_node spawn_list;
>  	struct crypto_alg *alg;
>  	struct crypto_instance *inst;
>  	const struct crypto_type *frontend;
> 

I think the general idea is much better.  But it's not going to work as-is due
to all the templates that directly use crypto_init_spawn(),
crypto_init_shash_spawn(), and crypto_init_ahash_spawn().  I think they should
be converted to use new functions crypto_grab_cipher(), crypto_grab_shash(), and
crypto_grab_cipher(), so that everyone is consistently using crypto_grab_*().

- Eric