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