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@xxxxxxxxxxxxxxxxxxx>
>
> 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
