More issues with arm/aes-neonbs

Hi,

I see there have been multiple attempts to fix this module, but sadly it seems that the problems persist and are not fixed.

On my i.MX6 platforms, since 6.9, I enabled aes-arm-bs support, and I've since been getting a load of hung tasks at boot. I've started to try to debug this evening under 6.10 - involving hacking the kernel code to try and get useful information out of the kernel. I've ended up dumping the entire state of all threads when the hung task fires.

What I find is this - the aes-arm-neonbs module is being probed, and this is its trace:

[   74.803096] task:modprobe        state:D stack:0     pid:613   tgid:613   ppid:37     flags:0x00000000
[   74.812620] Call trace:
[   74.812636] [<c0b784cc>] (__schedule) from [<c0b78bbc>] (schedule+0x50/0x128)
[   74.822586] [<c0b78bbc>] (schedule) from [<c0b82fac>] (schedule_timeout+0xb0/0x1b8)
[   74.830444] [<c0b82fac>] (schedule_timeout) from [<c0b79420>] (__wait_for_common+0x74/0x170)
[   74.839110] [<c0b79420>] (__wait_for_common) from [<c0488b8c>] (crypto_larval_wait+0x14/0x98)
[   74.847852] [<c0488b8c>] (crypto_larval_wait) from [<c0488e14>] (crypto_alg_mod_lookup+0x204/0x20c)
[   74.857118] [<c0488e14>] (crypto_alg_mod_lookup) from [<c0488f5c>] (crypto_alloc_tfm_node+0x48/0xb4)
[   74.866468] [<c0488f5c>] (crypto_alloc_tfm_node) from [<c048c478>] (crypto_alloc_skcipher+0x28/0x30)
[   74.875857] [<c048c478>] (crypto_alloc_skcipher) from [<bf3e88b8>] (cbc_init+0x1c/0x38 [aes_arm_bs])
[   74.885264] [<bf3e88b8>] (cbc_init [aes_arm_bs]) from [<c04889c0>] (crypto_create_tfm_node+0x34/0xd4)
[   74.894736] [<c04889c0>] (crypto_create_tfm_node) from [<c0488f74>] (crypto_alloc_tfm_node+0x60/0xb4)
[   74.894770] [<c0488f74>] (crypto_alloc_tfm_node) from [<c048c478>] (crypto_alloc_skcipher+0x28/0x30)
[   74.894800] [<c048c478>] (crypto_alloc_skcipher) from [<bf3de61c>] (simd_skcipher_create_compat+0x20/0x17c [crypto_simd])
[   74.894849] [<bf3de61c>] (simd_skcipher_create_compat [crypto_simd]) from [<bf3ef06c>] (aes_init+0x6c/0x1000 [aes_arm_bs])
[   74.894896] [<bf3ef06c>] (aes_init [aes_arm_bs]) from [<c0009ffc>] (do_one_initcall+0x60/0x2c0)
[   74.894933] [<c0009ffc>] (do_one_initcall) from [<c00e6640>] (do_init_module+0x54/0x1fc)
[   74.894962] [<c00e6640>] (do_init_module) from [<c00e8644>] (init_module_from_file+0x84/0xa4)
[   74.961860] [<c00e8644>] (init_module_from_file) from [<c00e892c>] (sys_finit_module+0x170/0x21c)
[   74.961897] [<c00e892c>] (sys_finit_module) from [<c0008320>] (ret_fast_syscall+0x0/0x1c)

What seems to be happening here is that we have registered all the main ciphers using crypto_register_skciphers(), and then we walk the array of algos, calling simd_skcipher_create_compat() on each.

We get to the __cbc(aes) entry, and this one seems to trigger the larval_wait thing. With debug in crypto_alg_mod_lookup(), I find this:

[   25.131852] modprobe:613: crypto_alg_mod_lookup: name=cbc(aes) type=0x5 mask=0x218e ok=32769
...
[   87.015070]   name=cbc(aes) alg=0xffffff92

and 0xffffff92 is an error-pointer for ETIMEDOUT.

i.MX6 does have the CAAM hardware that can do cbc(aes), so thinking that may be the issue, I decided to try blacklisting the CAAM modules. This made no difference.

It seems that the issue is centred around the aes-arm-bs module. Even after boot, and having removed the module, manually reloading it also causes the same problem:

# time modprobe aes-arm-bs
modprobe: ERROR: could not insert 'aes_arm_bs': Connection timed out

real    1m1.731s
user    0m0.004s
sys     0m0.052s

The interesting thing is... if I blacklist the aes-arm module, then aes-arm-bs doesn't behave this way and loads successfully. If I pre- load the aes-arm module, then the hanging behaviour returns.

So... with my debug in place, loading aes-arm-bs with aes-arm blacklisted gives me:

[ 4289.026431] modprobe:1786: crypto_alg_mod_lookup: name=cbc(aes) type=0x5 mask=0x218e ok=32769
[ 4289.084516] cryptomgr_probe:1788: crypto_alg_mod_lookup: name=aes type=0x20004 mask=0x218f ok=0
[ 4289.084556]   name=aes alg=0xfffffffe
[ 4289.114602] cryptomgr_probe:1788: crypto_alg_mod_lookup: name=ecb(aes) type=0x20004 mask=0x218f ok=32769
[ 4289.163489] cryptomgr_probe:1793: crypto_alg_mod_lookup: name=aes type=0x20004 mask=0x218f ok=0
[ 4289.163530]   name=aes alg=0xfffffffe
[ 4289.165187]   name=ecb(aes) alg=0xc4b318c0
[ 4289.165367]   name=cbc(aes) alg=0xc4b31cc0

Hence, looking up "aes" returns an immediate -ENOENT (and this is the only "name" that aes-arm provides.) With aes-arm loaded:

[ 3926.164204] modprobe:1691: crypto_alg_mod_lookup: name=cbc(aes) type=0x5 mask
=0x218e ok=32769
[ 3926.212563] cryptomgr_probe:1693: crypto_alg_mod_lookup: name=aes type=0x2000
4 mask=0x218f ok=0
[ 3926.212605]   name=aes alg=0xfffffffe
[ 3988.209746]   name=cbc(aes) alg=0xffffff92
[ 3988.412691] cryptomgr_probe:1693: crypto_alg_mod_lookup: name=ecb(aes) type=0x20004 mask=0x218f ok=32769
[ 3988.462116] cryptomgr_probe:1708: crypto_alg_mod_lookup: name=aes type=0x20004 mask=0x218f ok=0
[ 3988.462159]   name=aes alg=0xfffffffe
[ 3988.462292]   name=ecb(aes) alg=0xc4b320c0

It's interesting in the case where aes-arm is not loaded that the cbc(aes) lookup only succeeds _after_ ecb(aes) has, but in the failing case, we're clearly waiting for cbc(aes) before proceeding to ecb(aes).

This is about as far as I've managed to get debugging this, and I'm starting to hit the maze that is crypto probing/manager code that isn't easy to understand... at least not on a late Monday evening. Any suggestions?

Right now, though, from what I can see the aes-arm-bs module is entirely unusable, and the only way I can get a reasonably bootable system is to avoid loading this module (either by disabling it in the kernel build or blacklisting it in modprobe - the latter being my current solutions to this bug.)

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Looks like something has gone wrong during the instantiation of the fallback cbc algorithm. I'm looking into it.

Thanks,

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

OK I tracked it down to a recursive module load that hangs because of this commit:

commit 9b9879fc03275ffe0da328cf5b864d9e694167c8

Author: Linus Torvalds <[email protected]>
Date:   Mon May 29 21:39:51 2023 -0400

modules: catch concurrent module loads, treat them as idempotent

So what's happening here is that the first modprobe tries to load a fallback CBC implementation, in doing so it triggers a load of the exact same module due to module aliases.

IOW we're loading aes-arm-bs which provides cbc(aes). However, this needs a fallback of cbc(aes) to operate, which is made out of the generic cbc module + any implementation of aes, or ecb(aes). The latter happens to also be provided by aes-arm-cb so that's why it tries to load the same module again.

Now I presume this used to just fail immediately which is OK because user-space would then try to load other aliases of ecb(aes). But it now hangs which causes the whole thing to freeze until a timeout hits somwhere along the line.

Cheers,

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Ahh. That would indeed be very wrong, but yes, the fact that it now just ends up hanging is very annoying and not helpful for debugging.

Sadly, the "return -EBUSY" that the code initially did caused problems because when the loading isn't recursive, but just concurrent (because two separate users show up at the same time), you really do want to wait for the original loader.

What used to happen is that the recursive loader would *also* load the module into memory (so it's loaded twice), but then the two loaders at the end get serialized by the 'module_mutex' when it does the add_unformed_module()

So at that point the module_patient_check_exists() would notice that one or the other loaded module is a duplicate, and would exit with an error (EBUSY or EEXIST depending on whether the "winning" module got to MODULE_STATE_LIVE in the meantime).

The new code exists exactly because on big machines, the "load X modules concurrently" was a huge cost, and would use literally gigabytes of memory loading the same duplicated module multiple times concurrently, only for all but one of them to fail.

So now we catch that "we're already loading this module" early, and wait for the first loader to do it all.

But yes, it does mean that you can't recursively load the same module from the kernel.

... which was obviously always bogus, but now it's actively very wrong.

Sadly, loading modules recursively is very much required in general (because modules depend on each other). So we do need to deal with that. It's only loading the *same* module recurively that is very very bad.

I guess we could at least add a timeout and a big fat warning when it triggers. But what's the right timeout? Sometimes module loading can really be very slow, if it problems hardware.

Let me think about this, because the new behavior is obviously not great for this situation, even if it was triggered by a different kernel bug / misfeature. From a debugging standpoint, that "silent hang" is most definitely bad.

It did apparently take a long time for people to notice (that module loading behavior is over a year old by now). How hard is it to just fix the recursive load?

ANYWAY.

While I think some more about this, does this attached patch at least give you an error printout? It won't fix the situation, but at least the "silently wait forever" should turn into a "wait forever but with a warning".

Which isn't perfect, but is better, and would presumably have made it a whole lot easier to debug this nasty situation.

Hmm?

(Please note: ENTIRELY UNTESTED! It compiles for me, but I might have done something incredibly stupid and maybe there's some silly and fatal bug in what _appears_ trivially correct).

Linus

 kernel/module/main.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

index d9592195c5bb..4150a546a00a 100644
@@ -3183,15 +3183,29 @@ static int idempotent_init_module(struct file *f, const char __user * uargs, int
 	if (!f || !(f->f_mode & FMODE_READ))
 		return -EBADF;
 
-	/* See if somebody else is doing the operation? */
-	if (idempotent(&idem, file_inode(f))) {
-		wait_for_completion(&idem.complete);
-		return idem.ret;
+	/* Are we the winners of the race and get to do this? */
+	if (!idempotent(&idem, file_inode(f))) {
+		int ret = init_module_from_file(f, uargs, flags);
+		return idempotent_complete(&idem, ret);
 	}
 
-	/* Otherwise, we'll do it and complete others */
-	return idempotent_complete(&idem,
-		init_module_from_file(f, uargs, flags));
+	/*
+	 * Somebody else won the race and is loading the module.
+	 *
+	 * We have to wait for it forever, since our 'idem' is
+	 * on the stack and the list entry stays there until
+	 * completed (but we could fix it under the idem_lock)
+	 *
+	 * It's also unclear what a real timeout might be,
+	 * but we could maybe at least make this killable
+	 * and remove the idem entry in that case?
+	 */
+	for (;;) {
+		int ret = wait_for_completion_timeout(&idem.complete, 10*HZ);
+		if (likely(!ret))
+			return idem.ret;
+		pr_warn_once("module '%pD' taking a long time to load", f);
+	}
 }
 
 SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags)

It's like I have a sixth sense.

The wait_for_completion_timeout() test was entirely wrong. It returns the time remaining if it timed out, not an error like some of the other ones.

So t needs to be

if (wait_for_completion_timeout(&idem.complete, 10*HZ))
return idem.ret;

instead.

Of course, it's only going to cause issues if it actually times out, so that patch "works" in normal situations where module loading takes less than 10s. But still - it was completely buggered.

Linus

Ok, I fixed the stupid timeout check condition, and I actually ended up testing this on my system by making the timeout be just 10ms instead of ten seconds.

With that, I get a nice

module 'hid-logitech-dj.ko' taking a long time to load

message about the "problem", so at least the warning seems to work.

I've committed that (with the timeout for the warning set back to 10s), not because it *fixes* anything, but because the warning itself is hopefully useful to avoid having to debug issues like this in the future, and because it also re-organizes the code so that any possible "break dependency on recursion detection" thing would be easier to deal with.

That said, the *proper* fix really is to make sure that a module doesn't recursively try to load itself.

Because the thing that happened to make it work before the "wait until the previous module has completely finished loading" was that we *used* to do "wait until the both module has _almost_ completely finished loading, then return an error if there was another copy of it".

And it turns out that the "return an error if there's a concurrent load" is fundmanetally racy if the concurrent loaders are actually concurrent (not a serial recursion).

The race is typically *small*, but when I made it bigger in commit 9828ed3f695a, it actually ended up triggering in real life for modules that had dependencies and returning an error before the module had finished would then cause cascading errors. So the race does exist in real life.

In the case of the actual recursive invocation, that race isn't an issue - it's all serial - and returning an error before fully initializing the first module is actually what you want (since it won't _become_ fully initialized if you wait for it).

But basically the old recursion avoidance was not really reliable in other situations, which is why we don't want to re-introduce the "error out early" behavior.

I don't know the crypto registration API enough to even guess at what the fix to break the recursion is.

Herbert?

Linus

Yes my plan is to fix this in the Crypto API and not do any recursive loads as we used to do.

The immediate cause of the recursive load is the self-test system (if it is not disabled through Kconfig). The algorithm registration does not return until after the self-test has successfully executed. For the algorithm in question, that involves loading a fallback algorithm which is what triggered the recursive module load.

We had an issue when algorithms were built into the kernel, where due to the random of registration calls, a self-test may invoke an algorithm which is built into the kernel but not yet registered. There it was resolved by postponing all self-tests until all algorithms had been registered (or when an algorithm was first used, which would trigger the self-test for that algorithm there and then).

I will extend this to modules and let the registration return as soon as the new algorithm can be looked up. The self-test can then complete asynchronously.

Russell, is it OK with you if we only resolve this in the mainline kernel or do you want a solution that can be backported as well?

Thanks,

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Ahh. I tried to figure out why it would load the same module recursively, and it was very unclear to me.

We don't have any generic module "do this asynchronously after you've been loaded", but I guess the crypto code itself could just do something like that when a new crypto algorithm has been registered?

The keyword being that "do this _asynchronouysly_" so that it doesn't hold up the module init itself..

I actually had what I thought was a cunning plan, and thought that I could fix this by reorganizing the module loading and relying on the module_mutex itself to avoid the race that happens when you release waiters early.

But it turns out my cunning plan was just me being stupid, because we really can't hold the module mutex over the initcall itself, and that's the part (well, _one_ of the parts) that needs protection.

And in fact, as part of shooting down my not-so-cunning plan I convinced myself that I don't think this recursive load actually ever worked at all, and it would always hang on the recursive module load.

But before that commit 9828ed3f695a, that hang was in module_patient_check_exists(), and it would be interruptible.

So any signal would then cause the nested module loading to break out with an error, the first module load would finish happilt, and at least it wouldn't hang forever.

End result: I now have a new plan - I'll make the wait_for_completion(&idem.complete) be interruptible and return -EINTR (and I'll have to clean up the wait-queues etc).

That should make all this work the effectively the same way the old path in module_patient_check_exists() used to work (and still does, for the non-file-load case).

That should be a distinct improvement, and at least get us back the old historical behavior. It still doesn't make the recursive module load _work_, but it won't be the "hung forever" disaster (and regression) that it is now.

But considering how not-cunning my first plan was, I'll sleep on it first. I think this plan actually works, but I'm not going to start implementing it at 10pm.

And then the crypto layer fixing the actual recursion issue will fix the underlying problem.

Linus

.. and that seems to have been pretty straightforward, and creating a test-module that just recursively does a "request_module()" of itself shows that it all seems to work.

I've committed it and marked it as

them as idempotent")

but it shouldn't actually matter for any non-buggy module situation.

Linus

That's fine - I've blacklisted the arm-aes-bs module in modprobe.conf on the affected machines. Thanks!

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

In order to allow testing to complete asynchronously after the registration process, instance larvals need to complete prior to having a test result. Support this by redoing the lookup for
instance larvals after completion. This should locate the pending
test larval and then repeat the wait on that (if it is still pending).

As the lookup is now repeated there is no longer any need to compute the fulfilment status and all that code can be removed.

---
 crypto/algapi.c | 48 +++---------------------------------------------
 crypto/api.c    | 23 +++++++++++++++++++----
 2 files changed, 22 insertions(+), 49 deletions(-)

index 122cd910c4e1..d2ccc1289f92 100644
@@ -235,7 +235,6 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
 EXPORT_SYMBOL_GPL(crypto_remove_spawns);
 
 static void crypto_alg_finish_registration(struct crypto_alg *alg,
-					   bool fulfill_requests,
 					   struct list_head *algs_to_put)
 {
 	struct crypto_alg *q;

@@ -247,30 +246,8 @@ static void crypto_alg_finish_registration(struct crypto_alg *alg,
 		if (crypto_is_moribund(q))
 			continue;
 
-		if (crypto_is_larval(q)) {
-			struct crypto_larval *larval = (void *)q;
-
-			/*
-			 * Check to see if either our generic name or
-			 * specific name can satisfy the name requested
-			 * by the larval entry q.
-			 */
-			if (strcmp(alg->cra_name, q->cra_name) &&
-			    strcmp(alg->cra_driver_name, q->cra_name))
-				continue;
-
-			if (larval->adult)
-				continue;
-			if ((q->cra_flags ^ alg->cra_flags) & larval->mask)
-				continue;
-
-			if (fulfill_requests && crypto_mod_get(alg))
-				larval->adult = alg;
-			else
-				larval->adult = ERR_PTR(-EAGAIN);
-
+		if (crypto_is_larval(q))
 			continue;
-		}
 
 		if (strcmp(alg->cra_name, q->cra_name))
 			continue;

@@ -359,7 +336,7 @@ __crypto_register_alg(struct crypto_alg *alg, struct list_head *algs_to_put)
 		list_add(&larval->alg.cra_list, &crypto_alg_list);
 	} else {
 		alg->cra_flags |= CRYPTO_ALG_TESTED;
-		crypto_alg_finish_registration(alg, true, algs_to_put);
+		crypto_alg_finish_registration(alg, algs_to_put);
 	}
 
 out:

@@ -376,7 +353,6 @@ void crypto_alg_tested(const char *name, int err)
 	struct crypto_alg *alg;
 	struct crypto_alg *q;
 	LIST_HEAD(list);
-	bool best;
 
 	down_write(&crypto_alg_sem);
 	list_for_each_entry(q, &crypto_alg_list, cra_list) {

@@ -408,25 +384,7 @@ void crypto_alg_tested(const char *name, int err)
 
 	alg->cra_flags |= CRYPTO_ALG_TESTED;
 
-	/*
-	 * If a higher-priority implementation of the same algorithm is
-	 * currently being tested, then don't fulfill request larvals.
-	 */
-	best = true;
-	list_for_each_entry(q, &crypto_alg_list, cra_list) {
-		if (crypto_is_moribund(q) || !crypto_is_larval(q))
-			continue;
-
-		if (strcmp(alg->cra_name, q->cra_name))
-			continue;
-
-		if (q->cra_priority > alg->cra_priority) {
-			best = false;
-			break;
-		}
-	}
-
-	crypto_alg_finish_registration(alg, best, &list);
+	crypto_alg_finish_registration(alg, &list);
 
 complete:
 	complete_all(&test->completion);

index 22556907b3bc..ffb81aa32725 100644
@@ -37,6 +37,8 @@ DEFINE_STATIC_KEY_FALSE(__crypto_boot_test_finished);
 #endif
 
 static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg);
+static struct crypto_alg *crypto_alg_lookup(const char *name, u32 type,
+					    u32 mask);
 
 struct crypto_alg *crypto_mod_get(struct crypto_alg *alg)
 {

@@ -201,9 +203,12 @@ static void crypto_start_test(struct crypto_larval *larval)
 
 static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg)
 {
-	struct crypto_larval *larval = (void *)alg;
+	struct crypto_larval *larval;
 	long time_left;
 
+again:
+	larval = container_of(alg, struct crypto_larval, alg);
+
 	if (!crypto_boot_test_finished())
 		crypto_start_test(larval);
 

@@ -215,9 +220,16 @@ static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg)
 		alg = ERR_PTR(-EINTR);
 	else if (!time_left)
 		alg = ERR_PTR(-ETIMEDOUT);
-	else if (!alg)
-		alg = ERR_PTR(-ENOENT);
-	else if (IS_ERR(alg))
+	else if (!alg) {
+		u32 type;
+		u32 mask;
+
+		alg = &larval->alg;
+		type = alg->cra_flags & ~(CRYPTO_ALG_LARVAL | CRYPTO_ALG_DEAD);
+		mask = larval->mask;
+		alg = crypto_alg_lookup(alg->cra_name, type, mask) ?:
+		      ERR_PTR(-ENOENT);
+	} else if (IS_ERR(alg))
 		;
 	else if (crypto_is_test_larval(larval) &&
 		 !(alg->cra_flags & CRYPTO_ALG_TESTED))

@@ -228,6 +240,9 @@ static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg)
 		alg = ERR_PTR(-EAGAIN);
 	crypto_mod_put(&larval->alg);
 
+	if (!IS_ERR(alg) && crypto_is_larval(alg))
+		goto again;
+
 	return alg;
 }
 

2.39.2

-- 
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Thanks for the report. Indeed the first patch is buggy as the larval isn't marked as dead upon completion which when paired with the new re-lookup triggers a dead-lock. Fix this by adding a DEAD marking prior to calling complete_all.

---8<---
In order to allow testing to complete asynchronously after the registration process, instance larvals need to complete prior to having a test result. Support this by redoing the lookup for
instance larvals after completion. This should locate the pending
test larval and then repeat the wait on that (if it is still pending).

As the lookup is now repeated there is no longer any need to compute the fulfilment status and all that code can be removed.

---
 crypto/algapi.c  | 48 +++---------------------------------------------
 crypto/algboss.c |  1 +
 crypto/api.c     | 23 +++++++++++++++++++----
 3 files changed, 23 insertions(+), 49 deletions(-)

index 122cd910c4e1..d2ccc1289f92 100644
@@ -235,7 +235,6 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
 EXPORT_SYMBOL_GPL(crypto_remove_spawns);
 
 static void crypto_alg_finish_registration(struct crypto_alg *alg,
-					   bool fulfill_requests,
 					   struct list_head *algs_to_put)
 {
 	struct crypto_alg *q;

@@ -247,30 +246,8 @@ static void crypto_alg_finish_registration(struct crypto_alg *alg,
 		if (crypto_is_moribund(q))
 			continue;
 
-		if (crypto_is_larval(q)) {
-			struct crypto_larval *larval = (void *)q;
-
-			/*
-			 * Check to see if either our generic name or
-			 * specific name can satisfy the name requested
-			 * by the larval entry q.
-			 */
-			if (strcmp(alg->cra_name, q->cra_name) &&
-			    strcmp(alg->cra_driver_name, q->cra_name))
-				continue;
-
-			if (larval->adult)
-				continue;
-			if ((q->cra_flags ^ alg->cra_flags) & larval->mask)
-				continue;
-
-			if (fulfill_requests && crypto_mod_get(alg))
-				larval->adult = alg;
-			else
-				larval->adult = ERR_PTR(-EAGAIN);
-
+		if (crypto_is_larval(q))
 			continue;
-		}
 
 		if (strcmp(alg->cra_name, q->cra_name))
 			continue;

@@ -359,7 +336,7 @@ __crypto_register_alg(struct crypto_alg *alg, struct list_head *algs_to_put)
 		list_add(&larval->alg.cra_list, &crypto_alg_list);
 	} else {
 		alg->cra_flags |= CRYPTO_ALG_TESTED;
-		crypto_alg_finish_registration(alg, true, algs_to_put);
+		crypto_alg_finish_registration(alg, algs_to_put);
 	}
 
 out:

@@ -376,7 +353,6 @@ void crypto_alg_tested(const char *name, int err)
 	struct crypto_alg *alg;
 	struct crypto_alg *q;
 	LIST_HEAD(list);
-	bool best;
 
 	down_write(&crypto_alg_sem);
 	list_for_each_entry(q, &crypto_alg_list, cra_list) {

@@ -408,25 +384,7 @@ void crypto_alg_tested(const char *name, int err)
 
 	alg->cra_flags |= CRYPTO_ALG_TESTED;
 
-	/*
-	 * If a higher-priority implementation of the same algorithm is
-	 * currently being tested, then don't fulfill request larvals.
-	 */
-	best = true;
-	list_for_each_entry(q, &crypto_alg_list, cra_list) {
-		if (crypto_is_moribund(q) || !crypto_is_larval(q))
-			continue;
-
-		if (strcmp(alg->cra_name, q->cra_name))
-			continue;
-
-		if (q->cra_priority > alg->cra_priority) {
-			best = false;
-			break;
-		}
-	}
-
-	crypto_alg_finish_registration(alg, best, &list);
+	crypto_alg_finish_registration(alg, &list);
 
 complete:
 	complete_all(&test->completion);

index 1aa5f306998a..d05a5aad2176 100644
@@ -64,6 +64,7 @@ static int cryptomgr_probe(void *data)
 	crypto_tmpl_put(tmpl);
 
 out:
+	param->larval->alg.cra_flags |= CRYPTO_ALG_DEAD;
 	complete_all(&param->larval->completion);
 	crypto_alg_put(&param->larval->alg);
 	kfree(param);

index 22556907b3bc..ffb81aa32725 100644
@@ -37,6 +37,8 @@ DEFINE_STATIC_KEY_FALSE(__crypto_boot_test_finished);
 #endif
 
 static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg);
+static struct crypto_alg *crypto_alg_lookup(const char *name, u32 type,
+					    u32 mask);
 
 struct crypto_alg *crypto_mod_get(struct crypto_alg *alg)
 {

@@ -201,9 +203,12 @@ static void crypto_start_test(struct crypto_larval *larval)
 
 static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg)
 {
-	struct crypto_larval *larval = (void *)alg;
+	struct crypto_larval *larval;
 	long time_left;
 
+again:
+	larval = container_of(alg, struct crypto_larval, alg);
+
 	if (!crypto_boot_test_finished())
 		crypto_start_test(larval);
 

@@ -215,9 +220,16 @@ static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg)
 		alg = ERR_PTR(-EINTR);
 	else if (!time_left)
 		alg = ERR_PTR(-ETIMEDOUT);
-	else if (!alg)
-		alg = ERR_PTR(-ENOENT);
-	else if (IS_ERR(alg))
+	else if (!alg) {
+		u32 type;
+		u32 mask;
+
+		alg = &larval->alg;
+		type = alg->cra_flags & ~(CRYPTO_ALG_LARVAL | CRYPTO_ALG_DEAD);
+		mask = larval->mask;
+		alg = crypto_alg_lookup(alg->cra_name, type, mask) ?:
+		      ERR_PTR(-ENOENT);
+	} else if (IS_ERR(alg))
 		;
 	else if (crypto_is_test_larval(larval) &&
 		 !(alg->cra_flags & CRYPTO_ALG_TESTED))

@@ -228,6 +240,9 @@ static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg)
 		alg = ERR_PTR(-EAGAIN);
 	crypto_mod_put(&larval->alg);
 
+	if (!IS_ERR(alg) && crypto_is_larval(alg))
+		goto again;
+
 	return alg;
 }
 

2.39.2

-- 
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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.

---
 crypto/algapi.c   | 23 ++++++++++++-----------
 crypto/api.c      | 41 +++++++++++++++++++++--------------------
 crypto/internal.h |  3 +--
 3 files changed, 34 insertions(+), 33 deletions(-)

index d2ccc1289f92..74e2261c184c 100644
@@ -366,7 +366,8 @@ void crypto_alg_tested(const char *name, int err)
 	}
 
 	pr_err("alg: Unexpected test result for %s: %d\n", name, err);
-	goto unlock;
+	up_write(&crypto_alg_sem);
+	return;
 
 found:
 	q->cra_flags |= CRYPTO_ALG_DEAD;

@@ -387,11 +388,12 @@ 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();

index aee31319be2e..711a6a5bfa2b 100644
@@ -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,

2.39.2

-- 
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Algorithm registration is usually carried out during module init, where as little work as possible should be carried out. The SIMD code violated this rule by allocating a tfm, this then triggers a full test of the algorithm which may dead-lock in certain cases.

SIMD is only allocating the tfm to get at the alg object, which is in fact already available as it is what we are registering. Use that directly and remove the crypto_alloc_tfm call.

Also remove some obsolete and unused SIMD API.

---
 arch/arm/crypto/aes-ce-glue.c     |  2 +-
 arch/arm/crypto/aes-neonbs-glue.c |  2 +-
 crypto/simd.c                     | 76 ++++++-------------------------
 include/crypto/internal/simd.h    | 12 +----
 4 files changed, 19 insertions(+), 73 deletions(-)

index b668c97663ec..f5b66f4cf45d 100644
@@ -711,7 +711,7 @@ static int __init aes_init(void)
 		algname = aes_algs[i].base.cra_name + 2;
 		drvname = aes_algs[i].base.cra_driver_name + 2;
 		basename = aes_algs[i].base.cra_driver_name;
-		simd = simd_skcipher_create_compat(algname, drvname, basename);
+		simd = simd_skcipher_create_compat(aes_algs + i, algname, drvname, basename);
 		err = PTR_ERR(simd);
 		if (IS_ERR(simd))
 			goto unregister_simds;

index fd04f855b2f5..f6be80b5938b 100644
@@ -491,7 +491,7 @@ static int __init aes_init(void)
 		algname = aes_algs[i].base.cra_name + 2;
 		drvname = aes_algs[i].base.cra_driver_name + 2;
 		basename = aes_algs[i].base.cra_driver_name;
-		simd = simd_skcipher_create_compat(algname, drvname, basename);
+		simd = simd_skcipher_create_compat(aes_algs + i, algname, drvname, basename);
 		err = PTR_ERR(simd);
 		if (IS_ERR(simd))
 			goto unregister_simds;

index d2316242a988..be97b97a75dd 100644
@@ -14,11 +14,10 @@
 struct simd_skcipher_alg;
 struct skcipher_alg;
 
-struct simd_skcipher_alg *simd_skcipher_create_compat(const char *algname,
+struct simd_skcipher_alg *simd_skcipher_create_compat(struct skcipher_alg *ialg,
+						      const char *algname,
 						      const char *drvname,
 						      const char *basename);
-struct simd_skcipher_alg *simd_skcipher_create(const char *algname,
-					       const char *basename);
 void simd_skcipher_free(struct simd_skcipher_alg *alg);
 
 int simd_register_skciphers_compat(struct skcipher_alg *algs, int count,

@@ -32,13 +31,6 @@ void simd_unregister_skciphers(struct skcipher_alg *algs, int count,
 struct simd_aead_alg;
 struct aead_alg;
 
-struct simd_aead_alg *simd_aead_create_compat(const char *algname,
-					      const char *drvname,
-					      const char *basename);
-struct simd_aead_alg *simd_aead_create(const char *algname,
-				       const char *basename);
-void simd_aead_free(struct simd_aead_alg *alg);
-
 int simd_register_aeads_compat(struct aead_alg *algs, int count,
 			       struct simd_aead_alg **simd_algs);
 

2.39.2

-- 
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

I'm getting a test failure with this series applied:

[    0.383128] alg: aead: failed to allocate transform for gcm_base(ctr(aes-generic),ghash-generic): -2
[    0.383500] alg: self-tests for gcm(aes) using gcm_base(ctr(aes-generic),ghash-generic) failed (rc=-2)

This is on x86_64 with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y.

- Eric

Could you please send me your config file?

Thanks,

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Given below in defconfig form, use 'make olddefconfig' to apply. The failures are nondeterministic and sometimes there are different ones, for example:

[    0.358017] alg: skcipher: failed to allocate transform for cbc(twofish-generic): -2
[    0.358365] alg: self-tests for cbc(twofish) using cbc(twofish-generic) failed (rc=-2)
[    0.358535] alg: skcipher: failed to allocate transform for cbc(camellia-generic): -2
[    0.358918] alg: self-tests for cbc(camellia) using cbc(camellia-generic) failed (rc=-2)
[    0.371533] alg: skcipher: failed to allocate transform for xts(ecb(aes-generic)): -2
[    0.371922] alg: self-tests for xts(aes) using xts(ecb(aes-generic)) failed (rc=-2)

Modules are not enabled, maybe that matters (I haven't checked yet).

CONFIG_SYSVIPC=y
CONFIG_POSIX_MQUEUE=y
CONFIG_NO_HZ=y
CONFIG_HIGH_RES_TIMERS=y
CONFIG_IKCONFIG=y
CONFIG_IKCONFIG_PROC=y
CONFIG_CGROUPS=y
CONFIG_USER_NS=y
CONFIG_BLK_DEV_INITRD=y
CONFIG_SMP=y
CONFIG_X86_X2APIC=y
CONFIG_HYPERVISOR_GUEST=y
CONFIG_PARAVIRT=y
CONFIG_MCORE2=y
CONFIG_NR_CPUS=48
CONFIG_NUMA=y
CONFIG_HZ_300=y
# CONFIG_RANDOMIZE_BASE is not set
CONFIG_IA32_EMULATION=y
CONFIG_JUMP_LABEL=y
CONFIG_NET=y
CONFIG_PACKET=y
CONFIG_PACKET_DIAG=y
CONFIG_UNIX=y
CONFIG_UNIX_DIAG=y
CONFIG_INET=y
CONFIG_PCI=y
CONFIG_PCI_MSI=y
CONFIG_DEVTMPFS=y
CONFIG_BLK_DEV_LOOP=y
CONFIG_VIRTIO_BLK=y
CONFIG_NETDEVICES=y
CONFIG_VIRTIO_NET=y
CONFIG_SERIAL_8250=y
CONFIG_SERIAL_8250_CONSOLE=y
CONFIG_SERIAL_8250_NR_UARTS=32
CONFIG_SERIAL_8250_RUNTIME_UARTS=32
CONFIG_HW_RANDOM_VIRTIO=y
CONFIG_VIRT_DRIVERS=y
CONFIG_VIRTIO_PCI=y
CONFIG_VIRTIO_MMIO=y
CONFIG_EXT4_FS=y
CONFIG_EXT4_FS_POSIX_ACL=y
CONFIG_EXT4_FS_SECURITY=y
CONFIG_AUTOFS_FS=y
CONFIG_TMPFS=y
CONFIG_TMPFS_POSIX_ACL=y
CONFIG_CRYPTO_USER=y
# CONFIG_CRYPTO_MANAGER_DISABLE_TESTS is not set
CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y
CONFIG_CRYPTO_PCRYPT=y
CONFIG_CRYPTO_DH_RFC7919_GROUPS=y
CONFIG_CRYPTO_ECDH=y
CONFIG_CRYPTO_ECDSA=y
CONFIG_CRYPTO_ECRDSA=y
CONFIG_CRYPTO_CURVE25519=y
CONFIG_CRYPTO_AES_TI=y
CONFIG_CRYPTO_ANUBIS=y
CONFIG_CRYPTO_BLOWFISH=y
CONFIG_CRYPTO_CAMELLIA=y
CONFIG_CRYPTO_DES=y
CONFIG_CRYPTO_FCRYPT=y
CONFIG_CRYPTO_KHAZAD=y
CONFIG_CRYPTO_SEED=y
CONFIG_CRYPTO_TEA=y
CONFIG_CRYPTO_TWOFISH=y
CONFIG_CRYPTO_ADIANTUM=y
CONFIG_CRYPTO_ARC4=y
CONFIG_CRYPTO_HCTR2=y
CONFIG_CRYPTO_KEYWRAP=y
CONFIG_CRYPTO_LRW=y
CONFIG_CRYPTO_PCBC=y
CONFIG_CRYPTO_AEGIS128=y
CONFIG_CRYPTO_SEQIV=y
CONFIG_CRYPTO_ECHAINIV=y
CONFIG_CRYPTO_ESSIV=y
CONFIG_CRYPTO_BLAKE2B=y
CONFIG_CRYPTO_MD4=y
CONFIG_CRYPTO_RMD160=y
CONFIG_CRYPTO_SM3_GENERIC=y
CONFIG_CRYPTO_VMAC=y
CONFIG_CRYPTO_WP512=y
CONFIG_CRYPTO_XXHASH=y
CONFIG_CRYPTO_CRC32=y
CONFIG_CRYPTO_DEFLATE=y
CONFIG_CRYPTO_LZO=y
CONFIG_CRYPTO_842=y
CONFIG_CRYPTO_LZ4=y
CONFIG_CRYPTO_LZ4HC=y
CONFIG_CRYPTO_ZSTD=y
CONFIG_CRYPTO_ANSI_CPRNG=y
CONFIG_CRYPTO_DRBG_HASH=y
CONFIG_CRYPTO_DRBG_CTR=y
CONFIG_CRYPTO_USER_API_HASH=y
CONFIG_CRYPTO_USER_API_SKCIPHER=y
CONFIG_CRYPTO_USER_API_RNG=y
CONFIG_CRYPTO_USER_API_RNG_CAVP=y
CONFIG_CRYPTO_USER_API_AEAD=y
CONFIG_CRYPTO_CURVE25519_X86=y
CONFIG_CRYPTO_AES_NI_INTEL=y
CONFIG_CRYPTO_BLOWFISH_X86_64=y
CONFIG_CRYPTO_CAMELLIA_AESNI_AVX2_X86_64=y
CONFIG_CRYPTO_CAST5_AVX_X86_64=y
CONFIG_CRYPTO_CAST6_AVX_X86_64=y
CONFIG_CRYPTO_DES3_EDE_X86_64=y
CONFIG_CRYPTO_SERPENT_SSE2_X86_64=y
CONFIG_CRYPTO_SERPENT_AVX2_X86_64=y
CONFIG_CRYPTO_SM4_AESNI_AVX2_X86_64=y
CONFIG_CRYPTO_TWOFISH_AVX_X86_64=y
CONFIG_CRYPTO_ARIA_GFNI_AVX512_X86_64=y
CONFIG_CRYPTO_CHACHA20_X86_64=y
CONFIG_CRYPTO_AEGIS128_AESNI_SSE2=y
CONFIG_CRYPTO_NHPOLY1305_SSE2=y
CONFIG_CRYPTO_NHPOLY1305_AVX2=y
CONFIG_CRYPTO_BLAKE2S_X86=y
CONFIG_CRYPTO_POLYVAL_CLMUL_NI=y
CONFIG_CRYPTO_POLY1305_X86_64=y
CONFIG_CRYPTO_SHA1_SSSE3=y
CONFIG_CRYPTO_SHA256_SSSE3=y
CONFIG_CRYPTO_SHA512_SSSE3=y
CONFIG_CRYPTO_SM3_AVX_X86_64=y
CONFIG_CRYPTO_GHASH_CLMUL_NI_INTEL=y
CONFIG_CRYPTO_CRC32C_INTEL=y
CONFIG_CRYPTO_CRC32_PCLMUL=y
CONFIG_CRYPTO_CRCT10DIF_PCLMUL=y
CONFIG_CRYPTO_DEV_PADLOCK=y
CONFIG_CRYPTO_DEV_PADLOCK_AES=y
CONFIG_CRYPTO_DEV_PADLOCK_SHA=y
CONFIG_CRYPTO_DEV_CCP=y
CONFIG_CRYPTO_DEV_NITROX_CNN55XX=y
CONFIG_CRYPTO_DEV_QAT_DH895xCC=y
CONFIG_CRYPTO_DEV_QAT_C3XXX=y
CONFIG_CRYPTO_DEV_QAT_C62X=y
CONFIG_CRYPTO_DEV_QAT_4XXX=y
CONFIG_CRYPTO_DEV_QAT_DH895xCCVF=y
CONFIG_CRYPTO_DEV_QAT_C3XXXVF=y
CONFIG_CRYPTO_DEV_QAT_C62XVF=y
CONFIG_CRYPTO_DEV_VIRTIO=y
CONFIG_CRYPTO_DEV_SAFEXCEL=y
CONFIG_CRYPTO_DEV_AMLOGIC_GXL=y
CONFIG_CRYPTO_DEV_AMLOGIC_GXL_DEBUG=y
CONFIG_CRYPTO_LIB_CURVE25519=y
CONFIG_CRYPTO_LIB_CHACHA20POLY1305=y
CONFIG_CRC_CCITT=y
CONFIG_CRC_T10DIF=y
CONFIG_CRC64_ROCKSOFT=y
CONFIG_CRC_ITU_T=y
CONFIG_CRC32_SELFTEST=y
CONFIG_CRC32_SLICEBY4=y
CONFIG_CRC4=y
CONFIG_CRC7=y
CONFIG_LIBCRC32C=y
CONFIG_PRINTK_TIME=y
CONFIG_DEBUG_KERNEL=y
CONFIG_DEBUG_FS=y
CONFIG_PANIC_TIMEOUT=5
CONFIG_UNWINDER_FRAME_POINTER=y

Yes I think that was the key. This triggers a massive self-test run which executes in parallel and reveals a few race conditions in the system. I think it boils down to the following scenario:

Base algorithm X-generic, X-optimised
Template Y
Optimised algorithm Y-X-optimised

Everything gets registered, and then the self-tests are started. When Y-X-optimised gets tested, it requests the creation of the generic Y(X-generic). Which then itself undergoes testing.

The race is that after Y(X-generic) gets registered, but just before it gets tested, X-optimised finally finishes self-testing which then causes all spawns of X-generic to be destroyed. So by the time the self-test for Y(X-generic) comes along, it can no longer find the algorithm. This error then bubbles up all the way up to the self-test of Y-X-optimised which then fails.

Note that there is some complexity that I've omitted here because when the generic self-test fails to find Y(X-generic) it actually triggers the construction of it again which then fails for various other reasons (these are not important because the construction should *not* be triggered at this point).

So in a way the error is expected, and we should probably remove the pr_err for the case where ENOENT is returned for the algorithm that we're currently testing.

The solution is two-fold. First when an algorithm undergoes self-testing it should not trigger its construction. Secondly if an instance larval fails to materialise due to it being destroyed by a more optimised algorithm coming along, it should obviously retry the construction.

Remove the check in __crypto_alg_lookup that stops a larval from matching new requests based on differences in the mask. It is better to block new requests even if it is wrong and then simply retry the lookup. If this ends up being the wrong larval it will sort iself out during the retry.

Reduce the CRYPTO_ALG_TYPE_MASK bits in type during larval creation as otherwise LSKCIPHER algorithms may not match SKCIPHER larvals.

Also block the instance creation during self-testing in the function crypto_larval_lookup by checking for CRYPTO_ALG_TESTED in the mask field.

Finally change the return value when crypto_alg_lookup fails in crypto_larval_wait to EAGAIN to redo the lookup.

index bbe29d438815..bfd177a4313a 100644
@@ -70,11 +70,6 @@ static struct crypto_alg *__crypto_alg_lookup(const char *name, u32 type,
 		if ((q->cra_flags ^ type) & mask)
 			continue;
 
-		if (crypto_is_larval(q) &&
-		    !crypto_is_test_larval((struct crypto_larval *)q) &&
-		    ((struct crypto_larval *)q)->mask != mask)
-			continue;
-
 		exact = !strcmp(q->cra_driver_name, name);
 		fuzzy = !strcmp(q->cra_name, name);
 		if (!exact && !(fuzzy && q->cra_priority > best))

@@ -113,6 +108,8 @@ struct crypto_larval *crypto_larval_alloc(const char *name, u32 type, u32 mask)
 	if (!larval)
 		return ERR_PTR(-ENOMEM);
 
+	type &= ~CRYPTO_ALG_TYPE_MASK | (mask ?: CRYPTO_ALG_TYPE_MASK);
+
 	larval->mask = mask;
 	larval->alg.cra_flags = CRYPTO_ALG_LARVAL | type;
 	larval->alg.cra_priority = -1;

@@ -229,7 +226,7 @@ static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg)
 		type = alg->cra_flags & ~(CRYPTO_ALG_LARVAL | CRYPTO_ALG_DEAD);
 		mask = larval->mask;
 		alg = crypto_alg_lookup(alg->cra_name, type, mask) ?:
-		      ERR_PTR(-ENOENT);
+		      ERR_PTR(-EAGAIN);
 	} else if (IS_ERR(alg))
 		;
 	else if (crypto_is_test_larval(larval) &&

@@ -308,8 +305,12 @@ static struct crypto_alg *crypto_larval_lookup(const char *name, u32 type,
 
 	if (!IS_ERR_OR_NULL(alg) && crypto_is_larval(alg))
 		alg = crypto_larval_wait(alg);
-	else if (!alg)
+	else if (alg)
+		;
+	else if (!(mask & CRYPTO_ALG_TESTED))
 		alg = crypto_larval_add(name, type, mask);
+	else
+		alg = ERR_PTR(-ENOENT);
 
 	return alg;
 }

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

As I said earlier, these errors are expected. What's happening is this:

__ecb-sm4-aesni-avx gets registered (but not tested)

cbc(sm4-generic) gets registered (but not tested)

__ecb-sm4-aesni-avx finishes testing
with lskcipher this is equivalent to crypto_cipher sm4 so it triggers the destruction of all instances of sm4

cbc(sm4-generic) gets marked as dead

cbc(sm4-generic) fails self-test because it's already dead (ENOENT)

It's harmless because whatever that is asking for cbc(sm4-generic) (in this case it's the extra-test mechanism) will simply retry the allocation which will then succeed.

I will send a patch to disable the warning when allocating X returns ENOENT while we're testing X itself. This can always happen if X gets killed for the reason mentioned above and it's perfectly harmless.

It's just that the race window was tiny previously because testing occurred immediately after registration. But now we've magnified that window many times with asynchronous testing.

Cheers,

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

The tests are still failing on upstream:

[    0.343845] alg: self-tests for rfc4106(gcm(aes)) using rfc4106(gcm_base(ctr(aes-generic),ghash-generic)) failed (rc=-2)

To me it still seems like commit 37da5d0ffa7b ("crypto: api - Do not wait for tests during registration") is just broken and should be reverted.

Besides the test failures, it looks like there's no longer any guarantee that algorithms are actually available now that their module is loaded.

E.g. consider if someone does 'modprobe aesni-intel' and then immediately creates a dm-crypt device. Now it sounds like the AES-NI algorithms might not have finished being tested yet and the generic algorithms can be used instead, resulting in a performance regression.

I understand that you want to try to fix the edge cases in "fallback" ciphers. But "fallback" ciphers have always seemed like a bad design due to how they use the crypto API recursively. I think the algorithms that use them should generally be migrated off of them, e.g. as I did in commit f235bc11cc95 ("crypto: arm/aes-neonbs - go back to using aes-arm directly"). That fixed the problem in aes-neonbs that seems to have triggered this work in the first place.

- Eric

You're right. I only disabled the warnings at the point of allocation, the overall self-test warning is still there. Let me get rid of them too.

That would indeed be a bug. But I haven't seen it in practice. Although the s390 folks were reporting some weird errors with dm-crypt, they have recently disappeared.

If you do see an actual failure please report it and then I'll consider reverting it until it's fixed.

That is not the case. After modprobe returns, the algorithm is guaranteed to have been registered. Yes it is untested, but that should not be a problem because a test larval will have been created and all users looking for that algorithm will be waiting on that test larval.

Yes getting rid of fallbacks is nice, but this it not the reason why we're making self-test asynchronous. The primary issue with synchronous self-tests is the modprobe dead-lock.

Cheers,

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

I'm not sure about that, since the code that looks up algorithms only looks for algorithms that already have the CRYPTO_ALG_TESTED flag.

That problem is caused by the use of fallback ciphers, though.

- Eric

For normal lookups (one without CRYPTO_ALG_TESTED set in the mask field), the core API will first look for a tested algorithm, and if that fails then it will look for an untested algorithm. The second step should find the larval and then sleep on that until it's done.

Sure that particular deadlock may have been due to a fallback, but such dependencies exist outside of fallbacks too. Especially now that we have the fuzz testing which will dynamically load the generic algorithms, it's easy to envisage a scenario where one module registers an algorithm, which then triggers modprobe's on the generic implementation of the same algorithm that then dead-locks.

PS it looks like there is an actual report of things breaking with async testing in mv_cesa so I might revert/disable the async testing after all.

Thanks,

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Actually that's not quite right. The test larval is registered with TESTED set so normal lookups will latch onto that and then wait.

Cheers,

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

It looks like it wasn't a bug in the async self-test.

Instead this appears to be a real bug that was discovered by the async testing (because we now run all the tests at the same time, thus testing the whether the driver deals with parallel requests or not).

This is a bit accidental, because the driver in question registered multiple hash algorithms. Had it only registered one, then nothing would have changed.

Is this something that we could improve in testmgr? Perhaps we can add a bit of parallelism ourselves to cover the case where a driver only registers one hash algorithm.

Cheers,

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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.

---
 crypto/algapi.c   | 19 ++++++++++---------
 crypto/api.c      | 41 +++++++++++++++++++++--------------------
 crypto/internal.h |  3 +--
 3 files changed, 32 insertions(+), 31 deletions(-)

index d2ccc1289f92..2a2a7b6a00d0 100644
@@ -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();

index aee31319be2e..711a6a5bfa2b 100644
@@ -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,

2.39.2

-- 
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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/[email protected]/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 <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
| Closes: https://lore.kernel.org/r/[email protected]/

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  }

0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Indeed. This should just return.

Thanks,

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

In order to allow testing to complete asynchronously after the registration process, instance larvals need to complete prior to having a test result. Support this by redoing the lookup for
instance larvals after completion. This should locate the pending
test larval and then repeat the wait on that (if it is still pending).

As the lookup is now repeated there is no longer any need to compute the fulfilment status and all that code can be removed.

---
 crypto/algapi.c | 48 +++---------------------------------------------
 crypto/api.c    | 23 +++++++++++++++++++----
 2 files changed, 22 insertions(+), 49 deletions(-)

index 122cd910c4e1..d2ccc1289f92 100644
@@ -235,7 +235,6 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
 EXPORT_SYMBOL_GPL(crypto_remove_spawns);
 
 static void crypto_alg_finish_registration(struct crypto_alg *alg,
-					   bool fulfill_requests,
 					   struct list_head *algs_to_put)
 {
 	struct crypto_alg *q;

@@ -247,30 +246,8 @@ static void crypto_alg_finish_registration(struct crypto_alg *alg,
 		if (crypto_is_moribund(q))
 			continue;
 
-		if (crypto_is_larval(q)) {
-			struct crypto_larval *larval = (void *)q;
-
-			/*
-			 * Check to see if either our generic name or
-			 * specific name can satisfy the name requested
-			 * by the larval entry q.
-			 */
-			if (strcmp(alg->cra_name, q->cra_name) &&
-			    strcmp(alg->cra_driver_name, q->cra_name))
-				continue;
-
-			if (larval->adult)
-				continue;
-			if ((q->cra_flags ^ alg->cra_flags) & larval->mask)
-				continue;
-
-			if (fulfill_requests && crypto_mod_get(alg))
-				larval->adult = alg;
-			else
-				larval->adult = ERR_PTR(-EAGAIN);
-
+		if (crypto_is_larval(q))
 			continue;
-		}
 
 		if (strcmp(alg->cra_name, q->cra_name))
 			continue;

@@ -359,7 +336,7 @@ __crypto_register_alg(struct crypto_alg *alg, struct list_head *algs_to_put)
 		list_add(&larval->alg.cra_list, &crypto_alg_list);
 	} else {
 		alg->cra_flags |= CRYPTO_ALG_TESTED;
-		crypto_alg_finish_registration(alg, true, algs_to_put);
+		crypto_alg_finish_registration(alg, algs_to_put);
 	}
 
 out:

@@ -376,7 +353,6 @@ void crypto_alg_tested(const char *name, int err)
 	struct crypto_alg *alg;
 	struct crypto_alg *q;
 	LIST_HEAD(list);
-	bool best;
 
 	down_write(&crypto_alg_sem);
 	list_for_each_entry(q, &crypto_alg_list, cra_list) {

@@ -408,25 +384,7 @@ void crypto_alg_tested(const char *name, int err)
 
 	alg->cra_flags |= CRYPTO_ALG_TESTED;
 
-	/*
-	 * If a higher-priority implementation of the same algorithm is
-	 * currently being tested, then don't fulfill request larvals.
-	 */
-	best = true;
-	list_for_each_entry(q, &crypto_alg_list, cra_list) {
-		if (crypto_is_moribund(q) || !crypto_is_larval(q))
-			continue;
-
-		if (strcmp(alg->cra_name, q->cra_name))
-			continue;
-
-		if (q->cra_priority > alg->cra_priority) {
-			best = false;
-			break;
-		}
-	}
-
-	crypto_alg_finish_registration(alg, best, &list);
+	crypto_alg_finish_registration(alg, &list);
 
 complete:
 	complete_all(&test->completion);

index 22556907b3bc..ffb81aa32725 100644
@@ -37,6 +37,8 @@ DEFINE_STATIC_KEY_FALSE(__crypto_boot_test_finished);
 #endif
 
 static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg);
+static struct crypto_alg *crypto_alg_lookup(const char *name, u32 type,
+					    u32 mask);
 
 struct crypto_alg *crypto_mod_get(struct crypto_alg *alg)
 {

@@ -201,9 +203,12 @@ static void crypto_start_test(struct crypto_larval *larval)
 
 static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg)
 {
-	struct crypto_larval *larval = (void *)alg;
+	struct crypto_larval *larval;
 	long time_left;
 
+again:
+	larval = container_of(alg, struct crypto_larval, alg);
+
 	if (!crypto_boot_test_finished())
 		crypto_start_test(larval);
 

@@ -215,9 +220,16 @@ static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg)
 		alg = ERR_PTR(-EINTR);
 	else if (!time_left)
 		alg = ERR_PTR(-ETIMEDOUT);
-	else if (!alg)
-		alg = ERR_PTR(-ENOENT);
-	else if (IS_ERR(alg))
+	else if (!alg) {
+		u32 type;
+		u32 mask;
+
+		alg = &larval->alg;
+		type = alg->cra_flags & ~(CRYPTO_ALG_LARVAL | CRYPTO_ALG_DEAD);
+		mask = larval->mask;
+		alg = crypto_alg_lookup(alg->cra_name, type, mask) ?:
+		      ERR_PTR(-ENOENT);
+	} else if (IS_ERR(alg))
 		;
 	else if (crypto_is_test_larval(larval) &&
 		 !(alg->cra_flags & CRYPTO_ALG_TESTED))

@@ -228,6 +240,9 @@ static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg)
 		alg = ERR_PTR(-EAGAIN);
 	crypto_mod_put(&larval->alg);
 
+	if (!IS_ERR(alg) && crypto_is_larval(alg))
+		goto again;
+
 	return alg;
 }
 

2.39.2

-- 
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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.

---
 crypto/algapi.c   | 23 ++++++++++++-----------
 crypto/api.c      | 41 +++++++++++++++++++++--------------------
 crypto/internal.h |  3 +--
 3 files changed, 34 insertions(+), 33 deletions(-)

index d2ccc1289f92..74e2261c184c 100644
@@ -366,7 +366,8 @@ void crypto_alg_tested(const char *name, int err)
 	}
 
 	pr_err("alg: Unexpected test result for %s: %d\n", name, err);
-	goto unlock;
+	up_write(&crypto_alg_sem);
+	return;
 
 found:
 	q->cra_flags |= CRYPTO_ALG_DEAD;

@@ -387,11 +388,12 @@ 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();

index aee31319be2e..711a6a5bfa2b 100644
@@ -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,

2.39.2

-- 
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Algorithm registration is usually carried out during module init, where as little work as possible should be carried out. The SIMD code violated this rule by allocating a tfm, this then triggers a full test of the algorithm which may dead-lock in certain cases.

SIMD is only allocating the tfm to get at the alg object, which is in fact already available as it is what we are registering. Use that directly and remove the crypto_alloc_tfm call.

Also remove some obsolete and unused SIMD API.

---
 arch/arm/crypto/aes-ce-glue.c     |  2 +-
 arch/arm/crypto/aes-neonbs-glue.c |  2 +-
 crypto/simd.c                     | 76 ++++++-------------------------
 include/crypto/internal/simd.h    | 12 +----
 4 files changed, 19 insertions(+), 73 deletions(-)

index b668c97663ec..f5b66f4cf45d 100644
@@ -711,7 +711,7 @@ static int __init aes_init(void)
 		algname = aes_algs[i].base.cra_name + 2;
 		drvname = aes_algs[i].base.cra_driver_name + 2;
 		basename = aes_algs[i].base.cra_driver_name;
-		simd = simd_skcipher_create_compat(algname, drvname, basename);
+		simd = simd_skcipher_create_compat(aes_algs + i, algname, drvname, basename);
 		err = PTR_ERR(simd);
 		if (IS_ERR(simd))
 			goto unregister_simds;

index 201eb35dde37..735a2441ad48 100644
@@ -540,7 +540,7 @@ static int __init aes_init(void)
 		algname = aes_algs[i].base.cra_name + 2;
 		drvname = aes_algs[i].base.cra_driver_name + 2;
 		basename = aes_algs[i].base.cra_driver_name;
-		simd = simd_skcipher_create_compat(algname, drvname, basename);
+		simd = simd_skcipher_create_compat(aes_algs + i, algname, drvname, basename);
 		err = PTR_ERR(simd);
 		if (IS_ERR(simd))
 			goto unregister_simds;

index d2316242a988..be97b97a75dd 100644
@@ -14,11 +14,10 @@
 struct simd_skcipher_alg;
 struct skcipher_alg;
 
-struct simd_skcipher_alg *simd_skcipher_create_compat(const char *algname,
+struct simd_skcipher_alg *simd_skcipher_create_compat(struct skcipher_alg *ialg,
+						      const char *algname,
 						      const char *drvname,
 						      const char *basename);
-struct simd_skcipher_alg *simd_skcipher_create(const char *algname,
-					       const char *basename);
 void simd_skcipher_free(struct simd_skcipher_alg *alg);
 
 int simd_register_skciphers_compat(struct skcipher_alg *algs, int count,

@@ -32,13 +31,6 @@ void simd_unregister_skciphers(struct skcipher_alg *algs, int count,
 struct simd_aead_alg;
 struct aead_alg;
 
-struct simd_aead_alg *simd_aead_create_compat(const char *algname,
-					      const char *drvname,
-					      const char *basename);
-struct simd_aead_alg *simd_aead_create(const char *algname,
-				       const char *basename);
-void simd_aead_free(struct simd_aead_alg *alg);
-
 int simd_register_aeads_compat(struct aead_alg *algs, int count,
 			       struct simd_aead_alg **simd_algs);
 

2.39.2

-- 
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Algorithm registration is usually carried out during module init, where as little work as possible should be carried out. The SIMD code violated this rule by allocating a tfm, this then triggers a full test of the algorithm which may dead-lock in certain cases.

SIMD is only allocating the tfm to get at the alg object, which is in fact already available as it is what we are registering. Use that directly and remove the crypto_alloc_tfm call.

Also remove some obsolete and unused SIMD API.

---
 arch/arm/crypto/aes-ce-glue.c     |  2 +-
 arch/arm/crypto/aes-neonbs-glue.c |  2 +-
 crypto/simd.c                     | 76 ++++++-------------------------
 include/crypto/internal/simd.h    | 12 +----
 4 files changed, 19 insertions(+), 73 deletions(-)

index b668c97663ec..f5b66f4cf45d 100644
@@ -711,7 +711,7 @@ static int __init aes_init(void)
 		algname = aes_algs[i].base.cra_name + 2;
 		drvname = aes_algs[i].base.cra_driver_name + 2;
 		basename = aes_algs[i].base.cra_driver_name;
-		simd = simd_skcipher_create_compat(algname, drvname, basename);
+		simd = simd_skcipher_create_compat(aes_algs + i, algname, drvname, basename);
 		err = PTR_ERR(simd);
 		if (IS_ERR(simd))
 			goto unregister_simds;

index 201eb35dde37..735a2441ad48 100644
@@ -540,7 +540,7 @@ static int __init aes_init(void)
 		algname = aes_algs[i].base.cra_name + 2;
 		drvname = aes_algs[i].base.cra_driver_name + 2;
 		basename = aes_algs[i].base.cra_driver_name;
-		simd = simd_skcipher_create_compat(algname, drvname, basename);
+		simd = simd_skcipher_create_compat(aes_algs + i, algname, drvname, basename);
 		err = PTR_ERR(simd);
 		if (IS_ERR(simd))
 			goto unregister_simds;

index d2316242a988..be97b97a75dd 100644
@@ -14,11 +14,10 @@
 struct simd_skcipher_alg;
 struct skcipher_alg;
 
-struct simd_skcipher_alg *simd_skcipher_create_compat(const char *algname,
+struct simd_skcipher_alg *simd_skcipher_create_compat(struct skcipher_alg *ialg,
+						      const char *algname,
 						      const char *drvname,
 						      const char *basename);
-struct simd_skcipher_alg *simd_skcipher_create(const char *algname,
-					       const char *basename);
 void simd_skcipher_free(struct simd_skcipher_alg *alg);
 
 int simd_register_skciphers_compat(struct skcipher_alg *algs, int count,

@@ -32,13 +31,6 @@ void simd_unregister_skciphers(struct skcipher_alg *algs, int count,
 struct simd_aead_alg;
 struct aead_alg;
 
-struct simd_aead_alg *simd_aead_create_compat(const char *algname,
-					      const char *drvname,
-					      const char *basename);
-struct simd_aead_alg *simd_aead_create(const char *algname,
-				       const char *basename);
-void simd_aead_free(struct simd_aead_alg *alg);
-
 int simd_register_aeads_compat(struct aead_alg *algs, int count,
 			       struct simd_aead_alg **simd_algs);
 

2.39.2

-- 
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

IMO, for CBC encryption aes-neonbs should just implement it itself on top of the assembly function __aes_arm_encrypt(), which is actually what it did originally before commit b56f5cbc7e08 ("crypto: arm/aes-neonbs - resolve fallback cipher at runtime"). I don't find the motivation of that commit particularly convincing, since aes-arm has a higher priority than aes-fixed-time anyway. Also since commit 913a3aa07d16, aes-arm is partially hardened against cache-timing attacks.

- Eric