diff mbox series

[2/3] crypto: api - Do not wait for tests during registration

Message ID ZrbTfOViUr3S4V7X@gondor.apana.org.au
State Superseded
Headers show
Series None | expand

Commit Message

Herbert Xu Aug. 10, 2024, 2:42 a.m. UTC 
As registration is usually carried out during module init, this
is a context where as little work as possible should be carried
out.  Testing may trigger module loads of underlying components,
which could even lead back to the module that is registering at
the moment.  This may lead to dead-locks outside of the Crypto API.

Avoid this by not waiting for the tests to complete.  They will
be scheduled but completion will be asynchronous.  Any users will
still wait for completion.

Reported-by: Russell King <linux@armlinux.org.uk>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
 crypto/algapi.c   | 19 ++++++++++---------
 crypto/api.c      | 41 +++++++++++++++++++++--------------------
 crypto/internal.h |  3 +--
 3 files changed, 32 insertions(+), 31 deletions(-)

Comments

Dan Carpenter Aug. 11, 2024, 1:30 p.m. UTC | #1 
Hi Herbert,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Herbert-Xu/crypto-api-Do-not-wait-for-tests-during-registration/20240810-160343
base:   https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
patch link:    https://lore.kernel.org/r/ZrbTfOViUr3S4V7X%40gondor.apana.org.au
patch subject: [PATCH 2/3] crypto: api - Do not wait for tests during registration
config: x86_64-randconfig-161-20240811 (https://download.01.org/0day-ci/archive/20240811/202408110413.vKk2q3qN-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202408110413.vKk2q3qN-lkp@intel.com/

smatch warnings:
crypto/algapi.c:396 crypto_alg_tested() error: uninitialized symbol 'test'.

vim +/test +396 crypto/algapi.c

73d3864a4823ab Herbert Xu     2008-08-03  350  void crypto_alg_tested(const char *name, int err)
73d3864a4823ab Herbert Xu     2008-08-03  351  {
73d3864a4823ab Herbert Xu     2008-08-03  352  	struct crypto_larval *test;
73d3864a4823ab Herbert Xu     2008-08-03  353  	struct crypto_alg *alg;
73d3864a4823ab Herbert Xu     2008-08-03  354  	struct crypto_alg *q;
73d3864a4823ab Herbert Xu     2008-08-03  355  	LIST_HEAD(list);
73d3864a4823ab Herbert Xu     2008-08-03  356  
73d3864a4823ab Herbert Xu     2008-08-03  357  	down_write(&crypto_alg_sem);
73d3864a4823ab Herbert Xu     2008-08-03  358  	list_for_each_entry(q, &crypto_alg_list, cra_list) {
b8e15992b420d0 Herbert Xu     2009-01-28  359  		if (crypto_is_moribund(q) || !crypto_is_larval(q))
73d3864a4823ab Herbert Xu     2008-08-03  360  			continue;

Is it possible for everything to be moribund or larval?

73d3864a4823ab Herbert Xu     2008-08-03  361  
73d3864a4823ab Herbert Xu     2008-08-03  362  		test = (struct crypto_larval *)q;
73d3864a4823ab Herbert Xu     2008-08-03  363  
73d3864a4823ab Herbert Xu     2008-08-03  364  		if (!strcmp(q->cra_driver_name, name))
73d3864a4823ab Herbert Xu     2008-08-03  365  			goto found;
73d3864a4823ab Herbert Xu     2008-08-03  366  	}
73d3864a4823ab Herbert Xu     2008-08-03  367  
c72358571aaadf Karim Eshapa   2017-05-13  368  	pr_err("alg: Unexpected test result for %s: %d\n", name, err);
73d3864a4823ab Herbert Xu     2008-08-03  369  	goto unlock;

This calling crypto_alg_put() on the last item in the list seems wrong either
way.

73d3864a4823ab Herbert Xu     2008-08-03  370  
73d3864a4823ab Herbert Xu     2008-08-03  371  found:
b8e15992b420d0 Herbert Xu     2009-01-28  372  	q->cra_flags |= CRYPTO_ALG_DEAD;
73d3864a4823ab Herbert Xu     2008-08-03  373  	alg = test->adult;
d6097b8d5d55f2 Nicolai Stange 2022-02-21  374  
d6097b8d5d55f2 Nicolai Stange 2022-02-21  375  	if (list_empty(&alg->cra_list))
73d3864a4823ab Herbert Xu     2008-08-03  376  		goto complete;
73d3864a4823ab Herbert Xu     2008-08-03  377  
d6097b8d5d55f2 Nicolai Stange 2022-02-21  378  	if (err == -ECANCELED)
d6097b8d5d55f2 Nicolai Stange 2022-02-21  379  		alg->cra_flags |= CRYPTO_ALG_FIPS_INTERNAL;
d6097b8d5d55f2 Nicolai Stange 2022-02-21  380  	else if (err)
73d3864a4823ab Herbert Xu     2008-08-03  381  		goto complete;
d6097b8d5d55f2 Nicolai Stange 2022-02-21  382  	else
d6097b8d5d55f2 Nicolai Stange 2022-02-21  383  		alg->cra_flags &= ~CRYPTO_ALG_FIPS_INTERNAL;
73d3864a4823ab Herbert Xu     2008-08-03  384  
73d3864a4823ab Herbert Xu     2008-08-03  385  	alg->cra_flags |= CRYPTO_ALG_TESTED;
73d3864a4823ab Herbert Xu     2008-08-03  386  
103961609b0935 Herbert Xu     2024-08-10  387  	crypto_alg_finish_registration(alg, &list);
cce9e06d100df1 Herbert Xu     2006-08-21  388  
73d3864a4823ab Herbert Xu     2008-08-03  389  complete:
862e4618d9321e Herbert Xu     2024-08-10  390  	list_del_init(&test->alg.cra_list);
73d3864a4823ab Herbert Xu     2008-08-03  391  	complete_all(&test->completion);
2825982d9d66eb Herbert Xu     2006-08-06  392  
73d3864a4823ab Herbert Xu     2008-08-03  393  unlock:
73d3864a4823ab Herbert Xu     2008-08-03  394  	up_write(&crypto_alg_sem);
2825982d9d66eb Herbert Xu     2006-08-06  395  
862e4618d9321e Herbert Xu     2024-08-10 @396  	crypto_alg_put(&test->alg);
                                                                ^^^^

73d3864a4823ab Herbert Xu     2008-08-03  397  	crypto_remove_final(&list);
cce9e06d100df1 Herbert Xu     2006-08-21  398  }
Herbert Xu Aug. 12, 2024, 10:33 a.m. UTC | #2 
On Sun, Aug 11, 2024 at 04:30:10PM +0300, Dan Carpenter wrote:
>
> c72358571aaadf Karim Eshapa   2017-05-13  368  	pr_err("alg: Unexpected test result for %s: %d\n", name, err);
> 73d3864a4823ab Herbert Xu     2008-08-03  369  	goto unlock;
> 
> This calling crypto_alg_put() on the last item in the list seems wrong either
> way.

Indeed.  This should just return.

Thanks,
diff mbox series

Patch

diff --git a/crypto/algapi.c b/crypto/algapi.c
index d2ccc1289f92..2a2a7b6a00d0 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -387,11 +387,13 @@  void crypto_alg_tested(const char *name, int err)
 	crypto_alg_finish_registration(alg, &list);
 
 complete:
+	list_del_init(&test->alg.cra_list);
 	complete_all(&test->completion);
 
 unlock:
 	up_write(&crypto_alg_sem);
 
+	crypto_alg_put(&test->alg);
 	crypto_remove_final(&list);
 }
 EXPORT_SYMBOL_GPL(crypto_alg_tested);
@@ -412,7 +414,6 @@  int crypto_register_alg(struct crypto_alg *alg)
 {
 	struct crypto_larval *larval;
 	LIST_HEAD(algs_to_put);
-	bool test_started = false;
 	int err;
 
 	alg->cra_flags &= ~CRYPTO_ALG_DEAD;
@@ -423,15 +424,16 @@  int crypto_register_alg(struct crypto_alg *alg)
 	down_write(&crypto_alg_sem);
 	larval = __crypto_register_alg(alg, &algs_to_put);
 	if (!IS_ERR_OR_NULL(larval)) {
-		test_started = crypto_boot_test_finished();
+		bool test_started = crypto_boot_test_finished();
+
 		larval->test_started = test_started;
+		if (test_started)
+			crypto_schedule_test(larval);
 	}
 	up_write(&crypto_alg_sem);
 
 	if (IS_ERR(larval))
 		return PTR_ERR(larval);
-	if (test_started)
-		crypto_wait_for_test(larval);
 	crypto_remove_final(&algs_to_put);
 	return 0;
 }
@@ -646,8 +648,10 @@  int crypto_register_instance(struct crypto_template *tmpl,
 	larval = __crypto_register_alg(&inst->alg, &algs_to_put);
 	if (IS_ERR(larval))
 		goto unlock;
-	else if (larval)
+	else if (larval) {
 		larval->test_started = true;
+		crypto_schedule_test(larval);
+	}
 
 	hlist_add_head(&inst->list, &tmpl->instances);
 	inst->tmpl = tmpl;
@@ -657,8 +661,6 @@  int crypto_register_instance(struct crypto_template *tmpl,
 
 	if (IS_ERR(larval))
 		return PTR_ERR(larval);
-	if (larval)
-		crypto_wait_for_test(larval);
 	crypto_remove_final(&algs_to_put);
 	return 0;
 }
@@ -1042,6 +1044,7 @@  static void __init crypto_start_tests(void)
 
 			l->test_started = true;
 			larval = l;
+			crypto_schedule_test(larval);
 			break;
 		}
 
@@ -1049,8 +1052,6 @@  static void __init crypto_start_tests(void)
 
 		if (!larval)
 			break;
-
-		crypto_wait_for_test(larval);
 	}
 
 	set_crypto_boot_test_finished();
diff --git a/crypto/api.c b/crypto/api.c
index ffb81aa32725..bbe29d438815 100644
--- a/crypto/api.c
+++ b/crypto/api.c
@@ -154,32 +154,31 @@  static struct crypto_alg *crypto_larval_add(const char *name, u32 type,
 	return alg;
 }
 
-void crypto_larval_kill(struct crypto_alg *alg)
+static void crypto_larval_kill(struct crypto_larval *larval)
 {
-	struct crypto_larval *larval = (void *)alg;
+	bool unlinked;
 
 	down_write(&crypto_alg_sem);
-	list_del(&alg->cra_list);
+	unlinked = list_empty(&larval->alg.cra_list);
+	if (!unlinked)
+		list_del_init(&larval->alg.cra_list);
 	up_write(&crypto_alg_sem);
-	complete_all(&larval->completion);
-	crypto_alg_put(alg);
-}
-EXPORT_SYMBOL_GPL(crypto_larval_kill);
 
-void crypto_wait_for_test(struct crypto_larval *larval)
+	if (unlinked)
+		return;
+
+	complete_all(&larval->completion);
+	crypto_alg_put(&larval->alg);
+}
+
+void crypto_schedule_test(struct crypto_larval *larval)
 {
 	int err;
 
 	err = crypto_probing_notify(CRYPTO_MSG_ALG_REGISTER, larval->adult);
-	if (WARN_ON_ONCE(err != NOTIFY_STOP))
-		goto out;
-
-	err = wait_for_completion_killable(&larval->completion);
-	WARN_ON(err);
-out:
-	crypto_larval_kill(&larval->alg);
+	WARN_ON_ONCE(err != NOTIFY_STOP);
 }
-EXPORT_SYMBOL_GPL(crypto_wait_for_test);
+EXPORT_SYMBOL_GPL(crypto_schedule_test);
 
 static void crypto_start_test(struct crypto_larval *larval)
 {
@@ -198,7 +197,7 @@  static void crypto_start_test(struct crypto_larval *larval)
 	larval->test_started = true;
 	up_write(&crypto_alg_sem);
 
-	crypto_wait_for_test(larval);
+	crypto_schedule_test(larval);
 }
 
 static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg)
@@ -218,9 +217,11 @@  static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg)
 	alg = larval->adult;
 	if (time_left < 0)
 		alg = ERR_PTR(-EINTR);
-	else if (!time_left)
+	else if (!time_left) {
+		if (crypto_is_test_larval(larval))
+			crypto_larval_kill(larval);
 		alg = ERR_PTR(-ETIMEDOUT);
-	else if (!alg) {
+	} else if (!alg) {
 		u32 type;
 		u32 mask;
 
@@ -355,7 +356,7 @@  struct crypto_alg *crypto_alg_mod_lookup(const char *name, u32 type, u32 mask)
 		crypto_mod_put(larval);
 		alg = ERR_PTR(-ENOENT);
 	}
-	crypto_larval_kill(larval);
+	crypto_larval_kill(container_of(larval, struct crypto_larval, alg));
 	return alg;
 }
 EXPORT_SYMBOL_GPL(crypto_alg_mod_lookup);
diff --git a/crypto/internal.h b/crypto/internal.h
index aee31319be2e..711a6a5bfa2b 100644
--- a/crypto/internal.h
+++ b/crypto/internal.h
@@ -113,8 +113,7 @@  struct crypto_alg *crypto_mod_get(struct crypto_alg *alg);
 struct crypto_alg *crypto_alg_mod_lookup(const char *name, u32 type, u32 mask);
 
 struct crypto_larval *crypto_larval_alloc(const char *name, u32 type, u32 mask);
-void crypto_larval_kill(struct crypto_alg *alg);
-void crypto_wait_for_test(struct crypto_larval *larval);
+void crypto_schedule_test(struct crypto_larval *larval);
 void crypto_alg_tested(const char *name, int err);
 
 void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,