Re: [v3 PATCH] crypto: api - Retain alg refcount in crypto_grab_spawn

[Eric Biggers <ebiggers@kernel.org>]

On Sun, Dec 15, 2019 at 12:11:19PM +0800, Herbert Xu wrote:
> On Sat, Dec 14, 2019 at 02:44:04PM +0800, Herbert Xu wrote:
> >
> >  			/*
> > -			 * We may encounter an unregistered instance here, since
> > -			 * an instance's spawns are set up prior to the instance
> > -			 * being registered.  An unregistered instance will have
> > -			 * NULL ->cra_users.next, since ->cra_users isn't
> > -			 * properly initialized until registration.  But an
> > -			 * unregistered instance cannot have any users, so treat
> > -			 * it the same as ->cra_users being empty.
> > +			 * We may encounter an unregistered instance
> > +			 * here, since an instance's spawns are set
> > +			 * up prior to the instance being registered.
> > +			 * An unregistered instance cannot have any
> > +			 * users, so treat it the same as ->cra_users
> > +			 * being empty.
> >  			 */
> > -			if (spawns->next == NULL)
> > +			if (!spawn->registered)
> >  				break;
> 
> This is not quite right.  spawn->registered only allows us to
> dereference spawn->inst, it doesn't actually mean that inst itself
> is on the cra_list.  Here is a better patch:
> 
> ---8<---
> This patch changes crypto_grab_spawn 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.
> 
> The reference count will be subsequently dropped by the crypto API
> once the instance has been registered.  The helper crypto_drop_spawn
> will also conditionally drop the reference count depending on whether
> it has been registered.
> 
> Note that the code is actually added to crypto_init_spawn.  However,
> unless the caller activates this by setting spawn->dropref beforehand
> then nothing happens.  The only caller that sets dropref is currently
> crypto_grab_spawn.
> 
> Once all legacy users of crypto_init_spawn disappear, then we can
> kill the dropref flag.
> 
> Internally each instance will maintain a list of its spawns prior
> to registration.  This memory used by this list is shared with
> other fields that are only used after registration.  In order for
> this to work a new flag spawn->registered is added to indicate
> whether spawn->inst can be used.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index cd643e294664..a2a5372efe1d 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -124,8 +124,6 @@ static void crypto_remove_instance(struct crypto_instance *inst,
>  		return;
>  
>  	inst->alg.cra_flags |= CRYPTO_ALG_DEAD;
> -	if (hlist_unhashed(&inst->list))
> -		return;
>  
>  	if (!tmpl || !crypto_tmpl_get(tmpl))
>  		return;
> @@ -179,10 +177,14 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
>  
>  			list_move(&spawn->list, &stack);
>  
> -			if (&inst->alg == nalg)
> +			if (spawn->registered && &inst->alg == nalg)
>  				break;

There's still code above that uses spawn->inst without verifying that
spawn->registered is set.

		inst = spawn->inst;

		BUG_ON(&inst->alg == alg);

Also, the below code looks redundant now that it's only executed when
spawn->registered.  If it's still needed, maybe the comment needs to be updated?

	/*
	 * We may encounter an unregistered instance here, since
	 * an instance's spawns are set up prior to the instance
	 * being registered.  An unregistered instance will have
	 * NULL ->cra_users.next, since ->cra_users isn't
	 * properly initialized until registration.  But an
	 * unregistered instance cannot have any users, so treat
	 * it the same as ->cra_users being empty.
	 */
	if (spawns->next == NULL)
		break;

> @@ -700,6 +724,11 @@ void crypto_drop_spawn(struct crypto_spawn *spawn)
>  	if (!spawn->dead)
>  		list_del(&spawn->list);
>  	up_write(&crypto_alg_sem);
> +
> +	if (!spawn->dropref || spawn->registered)
> +		return;
> +
> +	crypto_mod_put(spawn->alg);
>  }
>  EXPORT_SYMBOL_GPL(crypto_drop_spawn);

How about:

	if (spawn->dropref && !spawn->registered)
		crypto_mod_put(spawn->alg);

> diff --git a/include/crypto/algapi.h b/include/crypto/algapi.h
> index 771a295ac755..29202b8f12fa 100644
> --- a/include/crypto/algapi.h
> +++ b/include/crypto/algapi.h
> @@ -47,7 +47,13 @@ struct crypto_instance {
>  	struct crypto_alg alg;
>  
>  	struct crypto_template *tmpl;
> -	struct hlist_node list;
> +
> +	union {
> +		/* List of instances after registration. */
> +		struct hlist_node list;

This really should say "Node in list of instances after registration."
Otherwise it sounds like it's a list, not an element of a list.

> +		/* List of attached spawns before registration. */
> +		struct crypto_spawn *spawns;
> +	};
>  

- Eric