[PATCH 0/4] crypto: api - Retain grabbed refcount until registration

[PATCH 0/4] crypto: api - Retain grabbed refcount until registration

[Herbert Xu]

When an instance grabs an algorithm, it has to hold a reference count
on it until the instance is registered.  This is because otherwise
the algorithm can go away at any time.

A number of such bugs has been fixed in the past.  This patch series
tries to close the remaining holes.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH 1/4] crypto: api - Retain alg refcount in crypto_grab_spawn

[Herbert Xu]

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.

For now this only changes adiantum as all other callers of this
function are internal API users.  The internal API callers will
still drop the ref count immediately.  They will be changed in
subsequent patches.
 
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 crypto/adiantum.c |    2 ++
 crypto/aead.c     |    7 ++++++-
 crypto/akcipher.c |    7 ++++++-
 crypto/algapi.c   |    1 -
 crypto/skcipher.c |    7 ++++++-
 5 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/crypto/adiantum.c b/crypto/adiantum.c
index aded26092268..655fd8921c13 100644
--- a/crypto/adiantum.c
+++ b/crypto/adiantum.c
@@ -627,6 +627,7 @@ static int adiantum_create(struct crypto_template *tmpl, struct rtattr **tb)
 		goto out_drop_hash;
 
 	crypto_mod_put(_hash_alg);
+	crypto_mod_put(blockcipher_alg);
 	return 0;
 
 out_drop_hash:
@@ -635,6 +636,7 @@ static int adiantum_create(struct crypto_template *tmpl, struct rtattr **tb)
 	crypto_mod_put(_hash_alg);
 out_drop_blockcipher:
 	crypto_drop_spawn(&ictx->blockcipher_spawn);
+	crypto_mod_put(blockcipher_alg);
 out_drop_streamcipher:
 	crypto_drop_skcipher(&ictx->streamcipher_spawn);
 out_free_inst:
diff --git a/crypto/aead.c b/crypto/aead.c
index 47f16d139e8e..f548a5c3f74d 100644
--- a/crypto/aead.c
+++ b/crypto/aead.c
@@ -210,8 +210,13 @@ 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;
-	return crypto_grab_spawn(&spawn->base, name, type, mask);
+	err = crypto_grab_spawn(&spawn->base, name, type, mask);
+	if (!err)
+		crypto_mod_put(spawn->base.alg);
+	return err;
 }
 EXPORT_SYMBOL_GPL(crypto_grab_aead);
 
diff --git a/crypto/akcipher.c b/crypto/akcipher.c
index 7d5cf4939423..ce3471102120 100644
--- a/crypto/akcipher.c
+++ b/crypto/akcipher.c
@@ -93,8 +93,13 @@ static const struct crypto_type crypto_akcipher_type = {
 int crypto_grab_akcipher(struct crypto_akcipher_spawn *spawn, const char *name,
 			 u32 type, u32 mask)
 {
+	int err;
+
 	spawn->base.frontend = &crypto_akcipher_type;
-	return crypto_grab_spawn(&spawn->base, name, type, mask);
+	err = crypto_grab_spawn(&spawn->base, name, type, mask);
+	if (!err)
+		crypto_mod_put(spawn->base.alg);
+	return err;
 }
 EXPORT_SYMBOL_GPL(crypto_grab_akcipher);
 
diff --git a/crypto/algapi.c b/crypto/algapi.c
index 9ecb4a57b342..6869feb31c99 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -662,7 +662,6 @@ int crypto_grab_spawn(struct crypto_spawn *spawn, const char *name,
 		return PTR_ERR(alg);
 
 	err = crypto_init_spawn(spawn, alg, spawn->inst, mask);
-	crypto_mod_put(alg);
 	return err;
 }
 EXPORT_SYMBOL_GPL(crypto_grab_spawn);
diff --git a/crypto/skcipher.c b/crypto/skcipher.c
index 41142c8cb912..d7cc271ed76b 100644
--- a/crypto/skcipher.c
+++ b/crypto/skcipher.c
@@ -770,8 +770,13 @@ static const struct crypto_type crypto_skcipher_type = {
 int crypto_grab_skcipher(struct crypto_skcipher_spawn *spawn,
 			  const char *name, u32 type, u32 mask)
 {
+	int err;
+
 	spawn->base.frontend = &crypto_skcipher_type;
-	return crypto_grab_spawn(&spawn->base, name, type, mask);
+	err = crypto_grab_spawn(&spawn->base, name, type, mask);
+	if (!err)
+		crypto_mod_put(spawn->base.alg);
+	return err;
 }
 EXPORT_SYMBOL_GPL(crypto_grab_skcipher);
 

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

[Herbert Xu]

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;
diff --git a/crypto/cryptd.c b/crypto/cryptd.c
index 2c6649b10923..c10ac1f61988 100644
--- a/crypto/cryptd.c
+++ b/crypto/cryptd.c
@@ -899,8 +899,9 @@ static int cryptd_create_aead(struct crypto_template *tmpl,
 	inst->alg.decrypt = cryptd_aead_decrypt_enqueue;
 
 	err = aead_register_instance(tmpl, inst);
-	if (err) {
 out_drop_aead:
+	aead_alg_put(alg);
+	if (err) {
 		crypto_drop_aead(&ctx->aead_spawn);
 out_free_inst:
 		kfree(inst);
diff --git a/crypto/echainiv.c b/crypto/echainiv.c
index a49cbf7b0929..0b4dc95abfcb 100644
--- a/crypto/echainiv.c
+++ b/crypto/echainiv.c
@@ -136,6 +136,7 @@ static int echainiv_aead_create(struct crypto_template *tmpl,
 	inst->free = aead_geniv_free;
 
 	err = aead_register_instance(tmpl, inst);
+	aead_geniv_alg_put(inst);
 	if (err)
 		goto free_inst;
 
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);
+
 	return 0;
 
 out_free_hash:
@@ -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);
+	}
 out_free_inst:
 	kfree(skcipher_inst);
 	kfree(aead_inst);
diff --git a/crypto/gcm.c b/crypto/gcm.c
index 73884208f075..f10af8f50836 100644
--- a/crypto/gcm.c
+++ b/crypto/gcm.c
@@ -941,11 +941,14 @@ static int crypto_rfc4106_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;
@@ -1179,11 +1182,14 @@ static int crypto_rfc4543_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;
diff --git a/crypto/geniv.c b/crypto/geniv.c
index b9e45a2a98b5..be6b8c2c30b8 100644
--- a/crypto/geniv.c
+++ b/crypto/geniv.c
@@ -105,6 +105,7 @@ struct aead_instance *aead_geniv_alloc(struct crypto_template *tmpl,
 
 err_drop_alg:
 	crypto_drop_aead(spawn);
+	aead_alg_put(alg);
 err_free_inst:
 	kfree(inst);
 	inst = ERR_PTR(err);
diff --git a/crypto/pcrypt.c b/crypto/pcrypt.c
index 543792e0ebf0..d7d4ad148a57 100644
--- a/crypto/pcrypt.c
+++ b/crypto/pcrypt.c
@@ -266,11 +266,14 @@ static int pcrypt_create_aead(struct crypto_template *tmpl, struct rtattr **tb,
 	if (err)
 		goto out_drop_aead;
 
+	aead_alg_put(alg);
+
 out:
 	return err;
 
 out_drop_aead:
 	crypto_drop_aead(&ctx->spawn);
+	aead_alg_put(alg);
 out_free_inst:
 	kfree(inst);
 	goto out;
diff --git a/crypto/seqiv.c b/crypto/seqiv.c
index 96d222c32acc..239826e4054c 100644
--- a/crypto/seqiv.c
+++ b/crypto/seqiv.c
@@ -159,6 +159,7 @@ static int seqiv_aead_create(struct crypto_template *tmpl, struct rtattr **tb)
 	inst->alg.base.cra_ctxsize += inst->alg.ivsize;
 
 	err = aead_register_instance(tmpl, inst);
+	aead_geniv_alg_put(inst);
 	if (err)
 		goto free_inst;
 
diff --git a/include/crypto/internal/aead.h b/include/crypto/internal/aead.h
index c509ec30fc65..43c3b33902ae 100644
--- a/include/crypto/internal/aead.h
+++ b/include/crypto/internal/aead.h
@@ -175,6 +175,11 @@ static inline unsigned int crypto_aead_chunksize(struct crypto_aead *tfm)
 	return crypto_aead_alg_chunksize(crypto_aead_alg(tfm));
 }
 
+static inline void aead_alg_put(struct aead_alg *alg)
+{
+	crypto_mod_put(&alg->base);
+}
+
 int crypto_register_aead(struct aead_alg *alg);
 void crypto_unregister_aead(struct aead_alg *alg);
 int crypto_register_aeads(struct aead_alg *algs, int count);
diff --git a/include/crypto/internal/geniv.h b/include/crypto/internal/geniv.h
index 0108c0c7b2ed..462e363bcb68 100644
--- a/include/crypto/internal/geniv.h
+++ b/include/crypto/internal/geniv.h
@@ -25,4 +25,11 @@ void aead_geniv_free(struct aead_instance *inst);
 int aead_init_geniv(struct crypto_aead *tfm);
 void aead_exit_geniv(struct crypto_aead *tfm);
 
+static inline void aead_geniv_alg_put(struct aead_instance *inst)
+{
+	struct crypto_aead_spawn *spawn = aead_instance_ctx(inst);
+	struct aead_alg *alg = crypto_spawn_aead_alg(spawn);
+	aead_alg_put(alg);
+}
+
 #endif	/* _CRYPTO_INTERNAL_GENIV_H */

[PATCH 3/4] crypto: akcipher - Retain alg refcount in crypto_grab_akcipher

[Herbert Xu]

This patch changes crypto_grab_akcipher 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/akcipher.c                  |    7 +------
 crypto/rsa-pkcs1pad.c              |    3 +++
 include/crypto/internal/akcipher.h |    5 +++++
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/crypto/akcipher.c b/crypto/akcipher.c
index ce3471102120..7d5cf4939423 100644
--- a/crypto/akcipher.c
+++ b/crypto/akcipher.c
@@ -93,13 +93,8 @@ static const struct crypto_type crypto_akcipher_type = {
 int crypto_grab_akcipher(struct crypto_akcipher_spawn *spawn, const char *name,
 			 u32 type, u32 mask)
 {
-	int err;
-
 	spawn->base.frontend = &crypto_akcipher_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_akcipher);
 
diff --git a/crypto/rsa-pkcs1pad.c b/crypto/rsa-pkcs1pad.c
index 0aa489711ec4..c445e93216e1 100644
--- a/crypto/rsa-pkcs1pad.c
+++ b/crypto/rsa-pkcs1pad.c
@@ -692,10 +692,13 @@ static int pkcs1pad_create(struct crypto_template *tmpl, struct rtattr **tb)
 	if (err)
 		goto out_drop_alg;
 
+	akcipher_alg_put(rsa_alg);
+
 	return 0;
 
 out_drop_alg:
 	crypto_drop_akcipher(spawn);
+	akcipher_alg_put(rsa_alg);
 out_free_inst:
 	kfree(inst);
 	return err;
diff --git a/include/crypto/internal/akcipher.h b/include/crypto/internal/akcipher.h
index d6c8a42789ad..10c36ed22b78 100644
--- a/include/crypto/internal/akcipher.h
+++ b/include/crypto/internal/akcipher.h
@@ -105,6 +105,11 @@ static inline struct akcipher_alg *crypto_spawn_akcipher_alg(
 	return container_of(spawn->base.alg, struct akcipher_alg, base);
 }
 
+static inline void akcipher_alg_put(struct akcipher_alg *alg)
+{
+	crypto_mod_put(&alg->base);
+}
+
 /**
  * crypto_register_akcipher() -- Register public key algorithm
  *

[PATCH 4/4] crypto: skcipher - Retain alg refcount in crypto_grab_skcipher

[Herbert Xu]

This patch changes crypto_grab_skcipher 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/adiantum.c                  |    2 ++
 crypto/authenc.c                   |    3 +++
 crypto/authencesn.c                |    3 +++
 crypto/ccm.c                       |    3 +++
 crypto/chacha20poly1305.c          |    3 +++
 crypto/cryptd.c                    |    3 ++-
 crypto/ctr.c                       |    3 +++
 crypto/cts.c                       |    3 +++
 crypto/essiv.c                     |    7 ++++---
 crypto/gcm.c                       |    3 +++
 crypto/lrw.c                       |    3 +++
 crypto/skcipher.c                  |    7 +------
 crypto/xts.c                       |    3 +++
 include/crypto/internal/skcipher.h |    5 +++++
 14 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/crypto/adiantum.c b/crypto/adiantum.c
index 655fd8921c13..40f87474049e 100644
--- a/crypto/adiantum.c
+++ b/crypto/adiantum.c
@@ -628,6 +628,7 @@ static int adiantum_create(struct crypto_template *tmpl, struct rtattr **tb)
 
 	crypto_mod_put(_hash_alg);
 	crypto_mod_put(blockcipher_alg);
+	skcipher_alg_put(streamcipher_alg);
 	return 0;
 
 out_drop_hash:
@@ -639,6 +640,7 @@ static int adiantum_create(struct crypto_template *tmpl, struct rtattr **tb)
 	crypto_mod_put(blockcipher_alg);
 out_drop_streamcipher:
 	crypto_drop_skcipher(&ictx->streamcipher_spawn);
+	skcipher_alg_put(streamcipher_alg);
 out_free_inst:
 	kfree(inst);
 	return err;
diff --git a/crypto/authenc.c b/crypto/authenc.c
index 3f0ed9402582..599155378983 100644
--- a/crypto/authenc.c
+++ b/crypto/authenc.c
@@ -473,12 +473,15 @@ static int crypto_authenc_create(struct crypto_template *tmpl,
 	if (err)
 		goto err_drop_enc;
 
+	skcipher_alg_put(enc);
+
 out:
 	crypto_mod_put(auth_base);
 	return err;
 
 err_drop_enc:
 	crypto_drop_skcipher(&ctx->enc);
+	skcipher_alg_put(enc);
 err_drop_auth:
 	crypto_drop_ahash(&ctx->auth);
 err_free_inst:
diff --git a/crypto/authencesn.c b/crypto/authencesn.c
index adb7554fca29..4bc96b8d8cf1 100644
--- a/crypto/authencesn.c
+++ b/crypto/authencesn.c
@@ -488,12 +488,15 @@ static int crypto_authenc_esn_create(struct crypto_template *tmpl,
 	if (err)
 		goto err_drop_enc;
 
+	skcipher_alg_put(enc);
+
 out:
 	crypto_mod_put(auth_base);
 	return err;
 
 err_drop_enc:
 	crypto_drop_skcipher(&ctx->enc);
+	skcipher_alg_put(enc);
 err_drop_auth:
 	crypto_drop_ahash(&ctx->auth);
 err_free_inst:
diff --git a/crypto/ccm.c b/crypto/ccm.c
index 6f555342a4a7..8898b08e10d9 100644
--- a/crypto/ccm.c
+++ b/crypto/ccm.c
@@ -548,12 +548,15 @@ static int crypto_ccm_create_common(struct crypto_template *tmpl,
 	if (err)
 		goto err_drop_ctr;
 
+	skcipher_alg_put(ctr);
+
 out_put_mac:
 	crypto_mod_put(mac_alg);
 	return err;
 
 err_drop_ctr:
 	crypto_drop_skcipher(&ictx->ctr);
+	skcipher_alg_put(ctr);
 err_drop_mac:
 	crypto_drop_ahash(&ictx->mac);
 err_free_inst:
diff --git a/crypto/chacha20poly1305.c b/crypto/chacha20poly1305.c
index 74e824e537e6..95f4b28c1d39 100644
--- a/crypto/chacha20poly1305.c
+++ b/crypto/chacha20poly1305.c
@@ -665,12 +665,15 @@ static int chachapoly_create(struct crypto_template *tmpl, struct rtattr **tb,
 	if (err)
 		goto out_drop_chacha;
 
+	skcipher_alg_put(chacha);
+
 out_put_poly:
 	crypto_mod_put(poly);
 	return err;
 
 out_drop_chacha:
 	crypto_drop_skcipher(&ctx->chacha);
+	skcipher_alg_put(chacha);
 err_drop_poly:
 	crypto_drop_ahash(&ctx->poly);
 err_free_inst:
diff --git a/crypto/cryptd.c b/crypto/cryptd.c
index c10ac1f61988..5e4de88848ea 100644
--- a/crypto/cryptd.c
+++ b/crypto/cryptd.c
@@ -451,8 +451,9 @@ static int cryptd_create_skcipher(struct crypto_template *tmpl,
 	inst->free = cryptd_skcipher_free;
 
 	err = skcipher_register_instance(tmpl, inst);
-	if (err) {
 out_drop_skcipher:
+	skcipher_alg_put(alg);
+	if (err) {
 		crypto_drop_skcipher(&ctx->spawn);
 out_free_inst:
 		kfree(inst);
diff --git a/crypto/ctr.c b/crypto/ctr.c
index e11e58950c0e..59f33840fd78 100644
--- a/crypto/ctr.c
+++ b/crypto/ctr.c
@@ -347,11 +347,14 @@ static int crypto_rfc3686_create(struct crypto_template *tmpl,
 	if (err)
 		goto err_drop_spawn;
 
+	skcipher_alg_put(alg);
+
 out:
 	return err;
 
 err_drop_spawn:
 	crypto_drop_skcipher(spawn);
+	skcipher_alg_put(alg);
 err_free_inst:
 	kfree(inst);
 	goto out;
diff --git a/crypto/cts.c b/crypto/cts.c
index 6b6087dbb62a..2df557231630 100644
--- a/crypto/cts.c
+++ b/crypto/cts.c
@@ -397,11 +397,14 @@ static int crypto_cts_create(struct crypto_template *tmpl, struct rtattr **tb)
 	if (err)
 		goto err_drop_spawn;
 
+	skcipher_alg_put(alg);
+
 out:
 	return err;
 
 err_drop_spawn:
 	crypto_drop_skcipher(spawn);
+	skcipher_alg_put(alg);
 err_free_inst:
 	kfree(inst);
 	goto out;
diff --git a/crypto/essiv.c b/crypto/essiv.c
index 08e0209d1b09..2c0bbef770f9 100644
--- a/crypto/essiv.c
+++ b/crypto/essiv.c
@@ -624,7 +624,7 @@ static int essiv_create(struct crypto_template *tmpl, struct rtattr **tb)
 	crypto_mod_put(_hash_alg);
 
 	if (type == CRYPTO_ALG_TYPE_SKCIPHER)
-		;
+		skcipher_alg_put(skcipher_alg);
 	else
 		aead_alg_put(aead_alg);
 
@@ -633,9 +633,10 @@ static int essiv_create(struct crypto_template *tmpl, struct rtattr **tb)
 out_free_hash:
 	crypto_mod_put(_hash_alg);
 out_drop_skcipher:
-	if (type == CRYPTO_ALG_TYPE_SKCIPHER)
+	if (type == CRYPTO_ALG_TYPE_SKCIPHER) {
 		crypto_drop_skcipher(&ictx->u.skcipher_spawn);
-	else {
+		skcipher_alg_put(skcipher_alg);
+	} else {
 		crypto_drop_aead(&ictx->u.aead_spawn);
 		aead_alg_put(aead_alg);
 	}
diff --git a/crypto/gcm.c b/crypto/gcm.c
index f10af8f50836..d0a0aa93bb5f 100644
--- a/crypto/gcm.c
+++ b/crypto/gcm.c
@@ -676,12 +676,15 @@ static int crypto_gcm_create_common(struct crypto_template *tmpl,
 	if (err)
 		goto out_put_ctr;
 
+	skcipher_alg_put(ctr);
+
 out_put_ghash:
 	crypto_mod_put(ghash_alg);
 	return err;
 
 out_put_ctr:
 	crypto_drop_skcipher(&ctx->ctr);
+	skcipher_alg_put(ctr);
 err_drop_ghash:
 	crypto_drop_ahash(&ctx->ghash);
 err_free_inst:
diff --git a/crypto/lrw.c b/crypto/lrw.c
index be829f6afc8e..4c23f273671e 100644
--- a/crypto/lrw.c
+++ b/crypto/lrw.c
@@ -407,11 +407,14 @@ static int create(struct crypto_template *tmpl, struct rtattr **tb)
 	if (err)
 		goto err_drop_spawn;
 
+	skcipher_alg_put(alg);
+
 out:
 	return err;
 
 err_drop_spawn:
 	crypto_drop_skcipher(spawn);
+	skcipher_alg_put(alg);
 err_free_inst:
 	kfree(inst);
 	goto out;
diff --git a/crypto/skcipher.c b/crypto/skcipher.c
index d7cc271ed76b..41142c8cb912 100644
--- a/crypto/skcipher.c
+++ b/crypto/skcipher.c
@@ -770,13 +770,8 @@ static const struct crypto_type crypto_skcipher_type = {
 int crypto_grab_skcipher(struct crypto_skcipher_spawn *spawn,
 			  const char *name, u32 type, u32 mask)
 {
-	int err;
-
 	spawn->base.frontend = &crypto_skcipher_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_skcipher);
 
diff --git a/crypto/xts.c b/crypto/xts.c
index ab117633d64e..40cf16c103e2 100644
--- a/crypto/xts.c
+++ b/crypto/xts.c
@@ -445,11 +445,14 @@ static int create(struct crypto_template *tmpl, struct rtattr **tb)
 	if (err)
 		goto err_drop_spawn;
 
+	skcipher_alg_put(alg);
+
 out:
 	return err;
 
 err_drop_spawn:
 	crypto_drop_skcipher(&ctx->spawn);
+	skcipher_alg_put(alg);
 err_free_inst:
 	kfree(inst);
 	goto out;
diff --git a/include/crypto/internal/skcipher.h b/include/crypto/internal/skcipher.h
index ad4a6330ff53..7b41fffb9bfc 100644
--- a/include/crypto/internal/skcipher.h
+++ b/include/crypto/internal/skcipher.h
@@ -226,5 +226,10 @@ static inline struct crypto_alg *skcipher_ialg_simple(
 	return spawn->alg;
 }
 
+static inline void skcipher_alg_put(struct skcipher_alg *alg)
+{
+	crypto_mod_put(&alg->base);
+}
+
 #endif	/* _CRYPTO_INTERNAL_SKCIPHER_H */
 

Re: [PATCH 1/4] crypto: api - Retain alg refcount in crypto_grab_spawn

[Eric Biggers]

On Fri, Dec 06, 2019 at 02:38:36PM +0800, Herbert Xu wrote:
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index 9ecb4a57b342..6869feb31c99 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -662,7 +662,6 @@ int crypto_grab_spawn(struct crypto_spawn *spawn, const char *name,
>  		return PTR_ERR(alg);
>  
>  	err = crypto_init_spawn(spawn, alg, spawn->inst, mask);
> -	crypto_mod_put(alg);
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(crypto_grab_spawn);

No need for 'err' anymore.  This should just do 'return crypto_init_spawn(...)'

- Eric

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

[Eric Biggers]

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

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

[Herbert Xu]

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;

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Eric Biggers]

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

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

[Herbert Xu]

On Fri, Dec 06, 2019 at 08:52:34PM -0800, Eric Biggers wrote:
>
> 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_*().

While we should certainly convert them eventually, I don't think
they are an immediate obstacle to this patch.  We just need to
make these legacy entry points grab an extra reference count which
they can then drop as they do now.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Herbert Xu]

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..0d34d519a72e 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;
@@ -173,29 +171,32 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
 
 			spawn = list_first_entry(spawns, struct crypto_spawn,
 						 list);
-			inst = spawn->inst;
-
-			BUG_ON(&inst->alg == alg);
-
 			list_move(&spawn->list, &stack);
-
-			if (&inst->alg == nalg)
-				break;
-
 			spawn->dead = true;
-			spawns = &inst->alg.cra_users;
 
 			/*
-			 * 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;
+
+			inst = spawn->inst;
+
+			BUG_ON(&inst->alg == alg);
+
+			spawns = &inst->alg.cra_users;
+
+			if (&inst->alg != nalg)
+				continue;
+
+			/* Don't kill the newly registered algorithm. */
+			spawn->dead = false;
+			break;
 		}
 	} while ((spawns = crypto_more_spawns(alg, &stack, &top,
 					      &secondary_spawns)));
@@ -208,7 +209,7 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
 	list_for_each_entry_safe(spawn, n, &secondary_spawns, list) {
 		if (!spawn->dead)
 			list_move(&spawn->list, &spawn->alg->cra_users);
-		else
+		else if (spawn->registered)
 			crypto_remove_instance(spawn->inst, list);
 	}
 }
@@ -588,6 +589,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);
@@ -599,6 +601,23 @@ int crypto_register_instance(struct crypto_template *tmpl,
 
 	down_write(&crypto_alg_sem);
 
+	larval = ERR_PTR(-EAGAIN);
+	for (spawn = inst->spawns; spawn;) {
+		struct crypto_spawn *next;
+
+		if (spawn->dead)
+			goto unlock;
+
+		next = spawn->next;
+		spawn->inst = inst;
+		spawn->registered = true;
+
+		if (spawn->dropref)
+			crypto_mod_put(spawn->alg);
+
+		spawn = next;
+	}
+
 	larval = __crypto_register_alg(&inst->alg);
 	if (IS_ERR(larval))
 		goto unlock;
@@ -646,7 +665,9 @@ int crypto_init_spawn(struct crypto_spawn *spawn, struct crypto_alg *alg,
 	if (WARN_ON_ONCE(inst == NULL))
 		return -EINVAL;
 
-	spawn->inst = inst;
+	spawn->next = inst->spawns;
+	inst->spawns = spawn;
+
 	spawn->mask = mask;
 
 	down_write(&crypto_alg_sem);
@@ -688,8 +709,10 @@ int crypto_grab_spawn(struct crypto_spawn *spawn, const char *name,
 	if (IS_ERR(alg))
 		return PTR_ERR(alg);
 
+	spawn->dropref = true;
 	err = crypto_init_spawn(spawn, alg, spawn->inst, mask);
-	crypto_mod_put(alg);
+	if (err)
+		crypto_mod_put(alg);
 	return err;
 }
 EXPORT_SYMBOL_GPL(crypto_grab_spawn);
@@ -700,6 +723,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);
 
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;
+		/* List of attached spawns before registration. */
+		struct crypto_spawn *spawns;
+	};
 
 	void *__ctx[] CRYPTO_MINALIGN_ATTR;
 };
@@ -67,10 +73,17 @@ struct crypto_template {
 struct crypto_spawn {
 	struct list_head list;
 	struct crypto_alg *alg;
-	struct crypto_instance *inst;
+	union {
+		/* Back pointer to instance after registration.*/
+		struct crypto_instance *inst;
+		/* Spawn list pointer prior to registration. */
+		struct crypto_spawn *next;
+	};
 	const struct crypto_type *frontend;
 	u32 mask;
 	bool dead;
+	bool dropref;
+	bool registered;
 };
 
 struct crypto_queue {
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Herbert Xu]

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;
 
 			spawn->dead = true;
+
+			if (!spawn->registered)
+				break;
+
 			spawns = &inst->alg.cra_users;
 
 			/*
@@ -208,7 +210,7 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
 	list_for_each_entry_safe(spawn, n, &secondary_spawns, list) {
 		if (!spawn->dead)
 			list_move(&spawn->list, &spawn->alg->cra_users);
-		else
+		else if (spawn->registered)
 			crypto_remove_instance(spawn->inst, list);
 	}
 }
@@ -588,6 +590,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);
@@ -599,6 +602,23 @@ int crypto_register_instance(struct crypto_template *tmpl,
 
 	down_write(&crypto_alg_sem);
 
+	larval = ERR_PTR(-EAGAIN);
+	for (spawn = inst->spawns; spawn;) {
+		struct crypto_spawn *next;
+
+		if (spawn->dead)
+			goto unlock;
+
+		next = spawn->next;
+		spawn->inst = inst;
+		spawn->registered = true;
+
+		if (spawn->dropref)
+			crypto_mod_put(spawn->alg);
+
+		spawn = next;
+	}
+
 	larval = __crypto_register_alg(&inst->alg);
 	if (IS_ERR(larval))
 		goto unlock;
@@ -646,7 +666,9 @@ int crypto_init_spawn(struct crypto_spawn *spawn, struct crypto_alg *alg,
 	if (WARN_ON_ONCE(inst == NULL))
 		return -EINVAL;
 
-	spawn->inst = inst;
+	spawn->next = inst->spawns;
+	inst->spawns = spawn;
+
 	spawn->mask = mask;
 
 	down_write(&crypto_alg_sem);
@@ -688,8 +710,10 @@ int crypto_grab_spawn(struct crypto_spawn *spawn, const char *name,
 	if (IS_ERR(alg))
 		return PTR_ERR(alg);
 
+	spawn->dropref = true;
 	err = crypto_init_spawn(spawn, alg, spawn->inst, mask);
-	crypto_mod_put(alg);
+	if (err)
+		crypto_mod_put(alg);
 	return err;
 }
 EXPORT_SYMBOL_GPL(crypto_grab_spawn);
@@ -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);
 
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;
+		/* List of attached spawns before registration. */
+		struct crypto_spawn *spawns;
+	};
 
 	void *__ctx[] CRYPTO_MINALIGN_ATTR;
 };
@@ -67,10 +73,17 @@ struct crypto_template {
 struct crypto_spawn {
 	struct list_head list;
 	struct crypto_alg *alg;
-	struct crypto_instance *inst;
+	union {
+		/* Back pointer to instance after registration.*/
+		struct crypto_instance *inst;
+		/* Spawn list pointer prior to registration. */
+		struct crypto_spawn *next;
+	};
 	const struct crypto_type *frontend;
 	u32 mask;
 	bool dead;
+	bool dropref;
+	bool registered;
 };
 
 struct crypto_queue {
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Eric Biggers]

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

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

[Herbert Xu]

On Sun, Dec 15, 2019 at 08:46:49PM -0800, Eric Biggers wrote:
>
> There's still code above that uses spawn->inst without verifying that
> spawn->registered is set.
> 
> 		inst = spawn->inst;
> 
> 		BUG_ON(&inst->alg == alg);

This is actually safe because spawn->inst is a real pointer to
a spawn or NULL.  However, I agree that it is needlessly confusing
and I've changed it in the new version.

> 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?

It's not actually redundant, because we set spawn->registered
before the instance is fully registered.  It could actually fail
during registration which would still trigger this case.  I've
added some more comments for it.

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

Done.
 
> 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.

Changed.

---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.

Fixes: d6ef2f198d4c ("crypto: api - Add crypto_grab_spawn primitive")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/algapi.c b/crypto/algapi.c
index cd643e294664..8ffaa1dca23b 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;
@@ -175,17 +173,26 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
 						 list);
 			inst = spawn->inst;
 
-			BUG_ON(&inst->alg == alg);
-
 			list_move(&spawn->list, &stack);
+			spawn->dead = !spawn->registered || &inst->alg != nalg;
+
+			if (!spawn->registered)
+				break;
+
+			BUG_ON(&inst->alg == alg);
 
 			if (&inst->alg == nalg)
 				break;
 
-			spawn->dead = true;
 			spawns = &inst->alg.cra_users;
 
 			/*
+			 * Even if spawn->registered is true, the
+			 * instance itself may still be unregistered.
+			 * This is because it may have failed during
+			 * registration.  Therefore we still need to
+			 * make the following test.
+			 *
 			 * 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
@@ -208,7 +215,7 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
 	list_for_each_entry_safe(spawn, n, &secondary_spawns, list) {
 		if (!spawn->dead)
 			list_move(&spawn->list, &spawn->alg->cra_users);
-		else
+		else if (spawn->registered)
 			crypto_remove_instance(spawn->inst, list);
 	}
 }
@@ -588,6 +595,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);
@@ -599,6 +607,23 @@ int crypto_register_instance(struct crypto_template *tmpl,
 
 	down_write(&crypto_alg_sem);
 
+	larval = ERR_PTR(-EAGAIN);
+	for (spawn = inst->spawns; spawn;) {
+		struct crypto_spawn *next;
+
+		if (spawn->dead)
+			goto unlock;
+
+		next = spawn->next;
+		spawn->inst = inst;
+		spawn->registered = true;
+
+		if (spawn->dropref)
+			crypto_mod_put(spawn->alg);
+
+		spawn = next;
+	}
+
 	larval = __crypto_register_alg(&inst->alg);
 	if (IS_ERR(larval))
 		goto unlock;
@@ -646,7 +671,9 @@ int crypto_init_spawn(struct crypto_spawn *spawn, struct crypto_alg *alg,
 	if (WARN_ON_ONCE(inst == NULL))
 		return -EINVAL;
 
-	spawn->inst = inst;
+	spawn->next = inst->spawns;
+	inst->spawns = spawn;
+
 	spawn->mask = mask;
 
 	down_write(&crypto_alg_sem);
@@ -688,8 +715,10 @@ int crypto_grab_spawn(struct crypto_spawn *spawn, const char *name,
 	if (IS_ERR(alg))
 		return PTR_ERR(alg);
 
+	spawn->dropref = true;
 	err = crypto_init_spawn(spawn, alg, spawn->inst, mask);
-	crypto_mod_put(alg);
+	if (err)
+		crypto_mod_put(alg);
 	return err;
 }
 EXPORT_SYMBOL_GPL(crypto_grab_spawn);
@@ -700,6 +729,9 @@ 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)
+		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..25bc54121848 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 {
+		/* Node in list of instances after registration. */
+		struct hlist_node list;
+		/* List of attached spawns before registration. */
+		struct crypto_spawn *spawns;
+	};
 
 	void *__ctx[] CRYPTO_MINALIGN_ATTR;
 };
@@ -67,10 +73,17 @@ struct crypto_template {
 struct crypto_spawn {
 	struct list_head list;
 	struct crypto_alg *alg;
-	struct crypto_instance *inst;
+	union {
+		/* Back pointer to instance after registration.*/
+		struct crypto_instance *inst;
+		/* Spawn list pointer prior to registration. */
+		struct crypto_spawn *next;
+	};
 	const struct crypto_type *frontend;
 	u32 mask;
 	bool dead;
+	bool dropref;
+	bool registered;
 };
 
 struct crypto_queue {
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt