diff mbox series

[5.10,1/4] crypto: api - Fix built-in testing dependency failures

Message ID 20230415101158.1648486-2-cuigaosheng1@huawei.com (mailing list archive)
State Not Applicable
Delegated to: Herbert Xu
Headers show
Series Fix built-in testing dependency failures | expand

Commit Message

cuigaosheng April 15, 2023, 10:11 a.m. UTC 
From: Herbert Xu <herbert@gondor.apana.org.au>

When complex algorithms that depend on other algorithms are built
into the kernel, the order of registration must be done such that
the underlying algorithms are ready before the ones on top are
registered.  As otherwise they would fail during the self-test
which is required during registration.

In the past we have used subsystem initialisation ordering to
guarantee this.  The number of such precedence levels are limited
and they may cause ripple effects in other subsystems.

This patch solves this problem by delaying all self-tests during
boot-up for built-in algorithms.  They will be tested either when
something else in the kernel requests for them, or when we have
finished registering all built-in algorithms, whichever comes
earlier.

Reported-by: Vladis Dronov <vdronov@redhat.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
---
 crypto/algapi.c   | 73 +++++++++++++++++++++++++++++++++--------------
 crypto/api.c      | 52 +++++++++++++++++++++++++++++----
 crypto/internal.h | 10 +++++++
 3 files changed, 108 insertions(+), 27 deletions(-)

Comments

Greg KH April 15, 2023, 3:07 p.m. UTC | #1 
On Sat, Apr 15, 2023 at 06:11:55PM +0800, Gaosheng Cui wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> 
> When complex algorithms that depend on other algorithms are built
> into the kernel, the order of registration must be done such that
> the underlying algorithms are ready before the ones on top are
> registered.  As otherwise they would fail during the self-test
> which is required during registration.
> 
> In the past we have used subsystem initialisation ordering to
> guarantee this.  The number of such precedence levels are limited
> and they may cause ripple effects in other subsystems.
> 
> This patch solves this problem by delaying all self-tests during
> boot-up for built-in algorithms.  They will be tested either when
> something else in the kernel requests for them, or when we have
> finished registering all built-in algorithms, whichever comes
> earlier.
> 
> Reported-by: Vladis Dronov <vdronov@redhat.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
> ---
>  crypto/algapi.c   | 73 +++++++++++++++++++++++++++++++++--------------
>  crypto/api.c      | 52 +++++++++++++++++++++++++++++----
>  crypto/internal.h | 10 +++++++
>  3 files changed, 108 insertions(+), 27 deletions(-)

What is the git commit id of this, and the other 3 patches, in Linus's
tree?  That is required to have here, as you know.

thanks,

greg k-h
Greg KH April 18, 2023, 9:28 a.m. UTC | #2 
On Sun, Apr 16, 2023 at 03:22:18PM +0800, cuigaosheng wrote:
> On 2023/4/15 23:07, Greg KH wrote:
> > On Sat, Apr 15, 2023 at 06:11:55PM +0800, Gaosheng Cui wrote:
> > > From: Herbert Xu <herbert@gondor.apana.org.au>
> > > 
> > > When complex algorithms that depend on other algorithms are built
> > > into the kernel, the order of registration must be done such that
> > > the underlying algorithms are ready before the ones on top are
> > > registered.  As otherwise they would fail during the self-test
> > > which is required during registration.
> > > 
> > > In the past we have used subsystem initialisation ordering to
> > > guarantee this.  The number of such precedence levels are limited
> > > and they may cause ripple effects in other subsystems.
> > > 
> > > This patch solves this problem by delaying all self-tests during
> > > boot-up for built-in algorithms.  They will be tested either when
> > > something else in the kernel requests for them, or when we have
> > > finished registering all built-in algorithms, whichever comes
> > > earlier.
> > > 
> > > Reported-by: Vladis Dronov <vdronov@redhat.com>
> > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> > > Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
> > > ---
> > >   crypto/algapi.c   | 73 +++++++++++++++++++++++++++++++++--------------
> > >   crypto/api.c      | 52 +++++++++++++++++++++++++++++----
> > >   crypto/internal.h | 10 +++++++
> > >   3 files changed, 108 insertions(+), 27 deletions(-)
> > What is the git commit id of this, and the other 3 patches, in Linus's
> > tree?  That is required to have here, as you know.
> > 
> > thanks,
> > 
> > greg k-h
> > .
> 
> Thanks for taking time to review these patch.
> 
> These patches are in Linus's tree, reference as follows:
>   Reference 1: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=adad556efcdd42a1d9e060cbe5f6161cccf1fa28
>   Reference 2: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=cad439fc040efe5f4381e3a7d583c5c200dbc186
>   Reference 3: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e42dff467ee688fe6b5a083f1837d06e3b27d8c0
>   Reference 4: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=beaaaa37c664e9afdf2913aee19185d8e3793b50

Please resend the patches with the git commit id in the changelog
somewhere, as is normally done (there are thousands of examples on the
mailing list.)

Also be sure that you are also backporting the patches to newer kernel
releases so that someone does not upgrade and have a regression (i.e. if
a patch is also needed in 5.15.y send a backport for that too.)

Thanks,

greg k-h
cuigaosheng April 18, 2023, 1:51 p.m. UTC | #3 
On 2023/4/18 17:28, Greg KH wrote:
> On Sun, Apr 16, 2023 at 03:22:18PM +0800, cuigaosheng wrote:
>> On 2023/4/15 23:07, Greg KH wrote:
>>> On Sat, Apr 15, 2023 at 06:11:55PM +0800, Gaosheng Cui wrote:
>>>> From: Herbert Xu <herbert@gondor.apana.org.au>
>>>>
>>>> When complex algorithms that depend on other algorithms are built
>>>> into the kernel, the order of registration must be done such that
>>>> the underlying algorithms are ready before the ones on top are
>>>> registered.  As otherwise they would fail during the self-test
>>>> which is required during registration.
>>>>
>>>> In the past we have used subsystem initialisation ordering to
>>>> guarantee this.  The number of such precedence levels are limited
>>>> and they may cause ripple effects in other subsystems.
>>>>
>>>> This patch solves this problem by delaying all self-tests during
>>>> boot-up for built-in algorithms.  They will be tested either when
>>>> something else in the kernel requests for them, or when we have
>>>> finished registering all built-in algorithms, whichever comes
>>>> earlier.
>>>>
>>>> Reported-by: Vladis Dronov <vdronov@redhat.com>
>>>> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>>>> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
>>>> ---
>>>>    crypto/algapi.c   | 73 +++++++++++++++++++++++++++++++++--------------
>>>>    crypto/api.c      | 52 +++++++++++++++++++++++++++++----
>>>>    crypto/internal.h | 10 +++++++
>>>>    3 files changed, 108 insertions(+), 27 deletions(-)
>>> What is the git commit id of this, and the other 3 patches, in Linus's
>>> tree?  That is required to have here, as you know.
>>>
>>> thanks,
>>>
>>> greg k-h
>>> .
>> Thanks for taking time to review these patch.
>>
>> These patches are in Linus's tree, reference as follows:
>>    Reference 1: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=adad556efcdd42a1d9e060cbe5f6161cccf1fa28
>>    Reference 2: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=cad439fc040efe5f4381e3a7d583c5c200dbc186
>>    Reference 3: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e42dff467ee688fe6b5a083f1837d06e3b27d8c0
>>    Reference 4: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=beaaaa37c664e9afdf2913aee19185d8e3793b50
> Please resend the patches with the git commit id in the changelog
> somewhere, as is normally done (there are thousands of examples on the
> mailing list.)
>
> Also be sure that you are also backporting the patches to newer kernel
> releases so that someone does not upgrade and have a regression (i.e. if
> a patch is also needed in 5.15.y send a backport for that too.)
>
> Thanks,
>
> greg k-h
> .


As I did more testing, I found that the patch set conflicted with SIMD,
so we needed a more appropriate solution to fix it, please ignore this
patch set, thanks.

Thanks for your time again!
diff mbox series

Patch

diff --git a/crypto/algapi.c b/crypto/algapi.c
index 9de27daa98b4..d6cd860bca4f 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -389,29 +389,10 @@  void crypto_remove_final(struct list_head *list)
 }
 EXPORT_SYMBOL_GPL(crypto_remove_final);
 
-static void crypto_wait_for_test(struct crypto_larval *larval)
-{
-	int err;
-
-	err = crypto_probing_notify(CRYPTO_MSG_ALG_REGISTER, larval->adult);
-	if (err != NOTIFY_STOP) {
-		if (WARN_ON(err != NOTIFY_DONE))
-			goto out;
-		crypto_alg_tested(larval->alg.cra_driver_name, 0);
-	}
-
-	err = wait_for_completion_killable(&larval->completion);
-	WARN_ON(err);
-	if (!err)
-		crypto_notify(CRYPTO_MSG_ALG_LOADED, larval);
-
-out:
-	crypto_larval_kill(&larval->alg);
-}
-
 int crypto_register_alg(struct crypto_alg *alg)
 {
 	struct crypto_larval *larval;
+	bool test_started;
 	int err;
 
 	alg->cra_flags &= ~CRYPTO_ALG_DEAD;
@@ -421,12 +402,15 @@  int crypto_register_alg(struct crypto_alg *alg)
 
 	down_write(&crypto_alg_sem);
 	larval = __crypto_register_alg(alg);
+	test_started = static_key_enabled(&crypto_boot_test_finished);
+	larval->test_started = test_started;
 	up_write(&crypto_alg_sem);
 
 	if (IS_ERR(larval))
 		return PTR_ERR(larval);
 
-	crypto_wait_for_test(larval);
+	if (test_started)
+		crypto_wait_for_test(larval);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(crypto_register_alg);
@@ -633,6 +617,8 @@  int crypto_register_instance(struct crypto_template *tmpl,
 	if (IS_ERR(larval))
 		goto unlock;
 
+	larval->test_started = true;
+
 	hlist_add_head(&inst->list, &tmpl->instances);
 	inst->tmpl = tmpl;
 
@@ -1279,9 +1265,48 @@  void crypto_stats_skcipher_decrypt(unsigned int cryptlen, int ret,
 EXPORT_SYMBOL_GPL(crypto_stats_skcipher_decrypt);
 #endif
 
+static void __init crypto_start_tests(void)
+{
+	for (;;) {
+		struct crypto_larval *larval = NULL;
+		struct crypto_alg *q;
+
+		down_write(&crypto_alg_sem);
+
+		list_for_each_entry(q, &crypto_alg_list, cra_list) {
+			struct crypto_larval *l;
+
+			if (!crypto_is_larval(q))
+				continue;
+
+			l = (void *)q;
+
+			if (!crypto_is_test_larval(l))
+				continue;
+
+			if (l->test_started)
+				continue;
+
+			l->test_started = true;
+			larval = l;
+			break;
+		}
+
+		up_write(&crypto_alg_sem);
+
+		if (!larval)
+			break;
+
+		crypto_wait_for_test(larval);
+	}
+
+	static_branch_enable(&crypto_boot_test_finished);
+}
+
 static int __init crypto_algapi_init(void)
 {
 	crypto_init_proc();
+	crypto_start_tests();
 	return 0;
 }
 
@@ -1290,7 +1315,11 @@  static void __exit crypto_algapi_exit(void)
 	crypto_exit_proc();
 }
 
-module_init(crypto_algapi_init);
+/*
+ * We run this at late_initcall so that all the built-in algorithms
+ * have had a chance to register themselves first.
+ */
+late_initcall(crypto_algapi_init);
 module_exit(crypto_algapi_exit);
 
 MODULE_LICENSE("GPL");
diff --git a/crypto/api.c b/crypto/api.c
index 5ffcd3ab4a75..44d3e4f8bb58 100644
--- a/crypto/api.c
+++ b/crypto/api.c
@@ -12,6 +12,7 @@ 
 
 #include <linux/err.h>
 #include <linux/errno.h>
+#include <linux/jump_label.h>
 #include <linux/kernel.h>
 #include <linux/kmod.h>
 #include <linux/module.h>
@@ -30,6 +31,8 @@  EXPORT_SYMBOL_GPL(crypto_alg_sem);
 BLOCKING_NOTIFIER_HEAD(crypto_chain);
 EXPORT_SYMBOL_GPL(crypto_chain);
 
+DEFINE_STATIC_KEY_FALSE(crypto_boot_test_finished);
+
 static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg);
 
 struct crypto_alg *crypto_mod_get(struct crypto_alg *alg)
@@ -47,11 +50,6 @@  void crypto_mod_put(struct crypto_alg *alg)
 }
 EXPORT_SYMBOL_GPL(crypto_mod_put);
 
-static inline int crypto_is_test_larval(struct crypto_larval *larval)
-{
-	return larval->alg.cra_driver_name[0];
-}
-
 static struct crypto_alg *__crypto_alg_lookup(const char *name, u32 type,
 					      u32 mask)
 {
@@ -163,11 +161,55 @@  void crypto_larval_kill(struct crypto_alg *alg)
 }
 EXPORT_SYMBOL_GPL(crypto_larval_kill);
 
+void crypto_wait_for_test(struct crypto_larval *larval)
+{
+	int err;
+
+	err = crypto_probing_notify(CRYPTO_MSG_ALG_REGISTER, larval->adult);
+	if (err != NOTIFY_STOP) {
+		if (WARN_ON(err != NOTIFY_DONE))
+			goto out;
+		crypto_alg_tested(larval->alg.cra_driver_name, 0);
+	}
+
+	err = wait_for_completion_killable(&larval->completion);
+	WARN_ON(err);
+	if (!err)
+		crypto_notify(CRYPTO_MSG_ALG_LOADED, larval);
+
+out:
+	crypto_larval_kill(&larval->alg);
+}
+EXPORT_SYMBOL_GPL(crypto_wait_for_test);
+
+static void crypto_start_test(struct crypto_larval *larval)
+{
+	if (!crypto_is_test_larval(larval))
+		return;
+
+	if (larval->test_started)
+		return;
+
+	down_write(&crypto_alg_sem);
+	if (larval->test_started) {
+		up_write(&crypto_alg_sem);
+		return;
+	}
+
+	larval->test_started = true;
+	up_write(&crypto_alg_sem);
+
+	crypto_wait_for_test(larval);
+}
+
 static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg)
 {
 	struct crypto_larval *larval = (void *)alg;
 	long timeout;
 
+	if (!static_branch_likely(&crypto_boot_test_finished))
+		crypto_start_test(larval);
+
 	timeout = wait_for_completion_killable_timeout(
 		&larval->completion, 60 * HZ);
 
diff --git a/crypto/internal.h b/crypto/internal.h
index 976ec9dfc76d..0a8986a9ca8c 100644
--- a/crypto/internal.h
+++ b/crypto/internal.h
@@ -10,6 +10,7 @@ 
 
 #include <crypto/algapi.h>
 #include <linux/completion.h>
+#include <linux/jump_label.h>
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/notifier.h>
@@ -27,12 +28,15 @@  struct crypto_larval {
 	struct crypto_alg *adult;
 	struct completion completion;
 	u32 mask;
+	bool test_started;
 };
 
 extern struct list_head crypto_alg_list;
 extern struct rw_semaphore crypto_alg_sem;
 extern struct blocking_notifier_head crypto_chain;
 
+DECLARE_STATIC_KEY_FALSE(crypto_boot_test_finished);
+
 #ifdef CONFIG_PROC_FS
 void __init crypto_init_proc(void);
 void __exit crypto_exit_proc(void);
@@ -58,6 +62,7 @@  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_alg_tested(const char *name, int err);
 
 void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
@@ -144,5 +149,10 @@  static inline void crypto_yield(u32 flags)
 		cond_resched();
 }
 
+static inline int crypto_is_test_larval(struct crypto_larval *larval)
+{
+	return larval->alg.cra_driver_name[0];
+}
+
 #endif	/* _CRYPTO_INTERNAL_H */