Skip to contentSkip to conversation

{twofish,aes}-{x86_64,i586} versus C implementations

PATCH In review Andi Kleen 6 participants 45 messages Aug 20, 2007 – Oct 8, 2007

Subsystems (4)
Crypto API s390 s390 crypto modules, prng driver, arch random AMD geode processor/chipset support
lore
Share & tools
lore

Andi Kleen

author
#0 lore raw

Hallo,

Currently there are two twofish and two aes implementions on x86. Worse when both are enabled as modules a modprobe aes will get the C version which seems to be slower on K8 and about the same speed on Core2 on my tests.

Is there a specific reason why anybody would prefer the C functions over the assembler functions?

Possible reasons I could think of:
- If the assembler functions are optimized for a specific CPU the compiler might be able to do a better job on other CPUs.
Is there evidence for this? I suspect it's not true.

- They are not trusted and might be buggy. I assume they have been validated against the C versions with a wide range of input data, correct?

If none of these reasons are valid it might make sense to disable the C versions for x86 and only offer the assembler versions. Then modprobe aes|twofish would DTRT automatically.

Comments?

-Andi

maintainer
#1 lore raw
Quoted 6 lines · Andi Kleen

Hallo,

Currently there are two twofish and two aes implementions on x86. Worse when both are enabled as modules a modprobe aes will get the C version which seems to be slower on K8 and about the same speed on Core2 on my tests.

Hi Andi:

Are you sure you get the C version when both are built-in or loaded as modules? If so then we have a bug in the priority code.

If you only have the C version loaded then that is understandable since the module loading code doesn't know about the other one.

Quoted 2 lines

Is there a specific reason why anybody would prefer the C functions over the assembler functions?

We don't, but the system is meant to allow multiple implementations to coexist and picking the best one at run-time.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#2 lore raw
Quoted 3 lines

Are you sure you get the C version when both are built-in or loaded as modules? If so then we have a bug in the priority code.

The usual use case is: Somebody -- either admin or some command implicitely -- executes modprobe aes because some text file specifies the aes cipher. At least on my system that loads the C version when both are enabled. modprobe will not load multiple modules in this case.

I don't think modprobe knows anything about these priorities.

Quoted 3 lines

We don't, but the system is meant to allow multiple implementations to coexist and picking the best one at run-time.

But that would require teaching the module loading user space about all this first, right?

Also if one implementation is always better than the other then I see little reason to ever have both.

-Andi

#3 lore raw
Quoted 11 lines · * Andi Kleen | 2007-08-20 12:16:18 [+0200]:
Quoted 3 lines

Are you sure you get the C version when both are built-in or loaded as modules? If so then we have a bug in the priority code.

The usual use case is: Somebody -- either admin or some command implicitely -- executes modprobe aes because some text file specifies the aes cipher. At least on my system that loads the C version when both are enabled. modprobe will not load multiple modules in this case.

I don't think modprobe knows anything about these priorities.

Not modprobe, but the crypto subsystem. If you have the generic C code and the assembly variant it picks the assembly over C. The selection is done before the particular subsystem, dm-crypt for instance, requests the algorithm / module. It makes no sense to load the AES-i586 module _after_ it is in use (dm-crypt loaded the encrypted root partition). However, further requests will get the assembly variant.

Quoted 6 lines
Quoted 3 lines

We don't, but the system is meant to allow multiple implementations to coexist and picking the best one at run-time.

But that would require teaching the module loading user space about all this first, right?

In that case yes. Would it help to add MODULE_ALIAS("aes") to the assembly version in order to load it (atleast both)?

Quoted 2 lines

Also if one implementation is always better than the other then I see little reason to ever have both.

If you are sure that nobody needs aes on machnies prio i586 than you could disable the generic version on i386. Also on x86_64 the generic version isn't required since an assembly optimized version is provided.
BUT: you might get into some trouble if you remove it from selections because some modules select it automaticly, IEEE80211_CRYPT_CCMP for instance.

Quoted 1 line

-Andi

Sebastian

author
#4 lore raw
Quoted 2 lines

Not modprobe, but the crypto subsystem. If you have the generic C code and the assembly variant it picks the assembly over C. The selection is

But only if they're both loaded. Who loads both?

Quoted 2 lines

In that case yes. Would it help to add MODULE_ALIAS("aes") to the assembly version in order to load it (atleast both)?

No, modprobe will only load the first it finds.

Quoted 5 lines
Quoted 2 lines

Also if one implementation is always better than the other then I see little reason to ever have both.

If you are sure that nobody needs aes on machnies prio i586 than you could disable the generic version on i386.

Why should the i586 version not run on 486/386?

Quoted 3 lines

BUT: you might get into some trouble if you remove it from selections because some modules select it automaticly, IEEE80211_CRYPT_CCMP for instance.

Ok that is a problem.

-Andi

#5 lore raw
Quoted 4 lines · * Andi Kleen | 2007-08-20 12:47:14 [+0200]:
Quoted 2 lines

Not modprobe, but the crypto subsystem. If you have the generic C code and the assembly variant it picks the assembly over C. The selection is

But only if they're both loaded. Who loads both?

In my case I do. I have the assembly version compiled into the kernel. The generic aes.o isn't loaded because the crypto API allready has an algorithm.

Quoted 4 lines
Quoted 2 lines

In that case yes. Would it help to add MODULE_ALIAS("aes") to the assembly version in order to load it (atleast both)?

No, modprobe will only load the first it finds.

The s390 guys have MODULE_ALIAS("aes"); in their hw driver [1]. If it doesn't load both (aes.ko + aes_s390.ko) modules, than I wonder what's the reason for this.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

Also if one implementation is always better than the other then I see little reason to ever have both.

If you are sure that nobody needs aes on machnies prio i586 than you could disable the generic version on i386.

Why should the i586 version not run on 486/386?

I assumed it uses some opcodes which are not available on 486.

Quoted 6 lines
Quoted 4 lines

BUT: you might get into some trouble if you remove it from selections because some modules select it automaticly, IEEE80211_CRYPT_CCMP for instance.

Ok that is a problem.

Not really I guess. The aes algorithm shouldn't be directly used by the wlan stack. It should only make sure that the user does not forget to enable aes since it is required for CCMP.

Quoted 2 lines

-Andi

Sebastian

[1] arch/s390/crypto/aes_s390.c

author
#6 lore raw
Quoted 7 lines · Sebastian Siewior
Quoted 4 lines · * Andi Kleen | 2007-08-20 12:47:14 [+0200]:
Quoted 2 lines

Not modprobe, but the crypto subsystem. If you have the generic C code and the assembly variant it picks the assembly over C. The selection is

But only if they're both loaded. Who loads both?

In my case I do.

You're unusual then.

I'm thinking of standard distribution kernel users though. They just want to tell some high level configuration they want aes (or twofish) and expect the most efficient implementation to be loaded automatically.

The distribution kernel could just disable the generic AES, but if that's a good idea there this could as well be done in all kernels.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

In that case yes. Would it help to add MODULE_ALIAS("aes") to the assembly version in order to load it (atleast both)?

No, modprobe will only load the first it finds.

The s390 guys have MODULE_ALIAS("aes"); in their hw driver [1]. If it doesn't load both (aes.ko + aes_s390.ko) modules, than I wonder what's the reason for this.

When only one is enabled then aes_s390 will be loaded.

But when both are enabled only one wins. At least on my system that seems to be the C version.

Quoted 10 lines
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines

Also if one implementation is always better than the other then I see little reason to ever have both.

If you are sure that nobody needs aes on machnies prio i586 than you could disable the generic version on i386.

Why should the i586 version not run on 486/386?

I assumed it uses some opcodes which are not available on 486.

There are not many. From a quick scan I didn't find any.

I assume the 586 refers to it being tuned for P5? Although that would be also weird, few people still care about P5 tuning and it's quite different from newer CPUs and likely not beneficial on them.

Quoted 9 lines
Quoted 5 lines
Quoted 3 lines

BUT: you might get into some trouble if you remove it from selections because some modules select it automaticly, IEEE80211_CRYPT_CCMP for instance.

Ok that is a problem.

Not really I guess. The aes algorithm shouldn't be directly used by the wlan stack. It should only make sure that the user does not forget to enable aes since it is required for CCMP.

Well it still would need to be solved to get rid of the generic aes/twofish. I don't know how unfortunately. Or could the select just be dropped?

-Andi

#7 lore raw
Quoted 4 lines · * Andi Kleen | 2007-08-20 13:12:39 [+0200]:

I'm thinking of standard distribution kernel users though. They just want to tell some high level configuration they want aes (or twofish) and expect the most efficient implementation to be loaded automatically.

Sure.

Quoted 3 lines

The distribution kernel could just disable the generic AES, but if that's a good idea there this could as well be done in all kernels.

If the i586 variant is working on all x86 machines than be my guest.

We still have the problem with hardware drivers like the via-aes driver. In that case you don't to have the generic nor the i586 version. Adding a priority to modprobe in case of two drivers with the same name/alias might not to be worst idea. On s390 you might need both, since not every machine provides hardware acceleration.

Quoted 8 lines
Quoted 3 lines

The s390 guys have MODULE_ALIAS("aes"); in their hw driver [1]. If it doesn't load both (aes.ko + aes_s390.ko) modules, than I wonder what's the reason for this.

When only one is enabled then aes_s390 will be loaded.

But when both are enabled only one wins. At least on my system that seems to be the C version.

Okay, same here.

Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Quoted 3 lines

BUT: you might get into some trouble if you remove it from selections because some modules select it automaticly, IEEE80211_CRYPT_CCMP for instance.

Ok that is a problem.

Quoted 7 lines
Quoted 3 lines

Not really I guess. The aes algorithm shouldn't be directly used by the wlan stack. It should only make sure that the user does not forget to enable aes since it is required for CCMP.

Well it still would need to be solved to get rid of the generic aes/twofish. I don't know how unfortunately. Or could the select just be dropped?

Yes, but wait for Herbert's ACK. Technically only the crypto subsystem is required for compilation. AES is required because it is part of CCMP. It is not needed for linking or anything. You have just to convince your users to enable AES if they select CCMP or else it will not work (that's the whole point about select in first place from my POV). This should not be a problem in a distro kernel.

Quoted 1 line

-Andi

maintainer
#8 lore raw
In reply to Andi Kleen
Quoted 8 lines · Andi Kleen

The usual use case is: Somebody -- either admin or some command implicitely -- executes modprobe aes because some text file specifies the aes cipher. At least on my system that loads the C version when both are enabled. modprobe will not load multiple modules in this case.

I don't think modprobe knows anything about these priorities.

Right, in that case we'd only load one of them, usually the generic one since its name is what we're trying to load.

Quoted 2 lines

But that would require teaching the module loading user space about all this first, right?

That would be the best. However, it's not hard to do a simple probing in the kernel until modprobe(8) gets this feature.

I'll code something up.

Quoted 2 lines

Also if one implementation is always better than the other then I see little reason to ever have both.

Well it's not that useful for an assembly implementation that works on all instances of a given architecture.

However, one of the things we need to handle are drivers that only work on a subset of an architecture, such as padlock-aes.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#9 lore raw
Quoted 3 lines

That would be the best. However, it's not hard to do a simple probing in the kernel until modprobe(8) gets this feature.

Sounds like a big hack, and at least for aes / aes-x86_64 and twofish it's not needed. Just disable aes on x86.

The only problem is the select issue with wireless.

Unfortunately

select CRYPTO_AES_X86_64 if X86_64
select CRYPTO_AES_I586 if X86_32
select CRYPTO_AES if !X86

produces warnings for unreferenced symbols :/
Perhaps it can be just removed for now.

Quoted 5 lines
Quoted 2 lines

Also if one implementation is always better than the other then I see little reason to ever have both.

Well it's not that useful for an assembly implementation that works on all instances of a given architecture.

I meant on x86. Sure for other architectures the C version is needed.

-Andi

maintainer
#10 lore raw
Quoted 5 lines · Andi Kleen
Quoted 3 lines

Well it's not that useful for an assembly implementation that works on all instances of a given architecture.

I meant on x86. Sure for other architectures the C version is needed.

My point was that while it's not useful for aes-i586 on x86, it definitely is useful for padlock-aes which is also x86 asm but only works on a subset of x86 processors.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#11 lore raw
In reply to Andi Kleen
Quoted 9 lines · * Andi Kleen | 2007-08-20 15:06:39 [+0200]:
Quoted 3 lines

That would be the best. However, it's not hard to do a simple probing in the kernel until modprobe(8) gets this feature.

Sounds like a big hack, and at least for aes / aes-x86_64 and twofish it's not needed. Just disable aes on x86.

The only problem is the select issue with wireless.

Quoted 8 lines

Unfortunately

select CRYPTO_AES_X86_64 if X86_64
select CRYPTO_AES_I586 if X86_32
select CRYPTO_AES if !X86

produces warnings for unreferenced symbols :/
Perhaps it can be just removed for now.

What about:

[crypto] do not use generic AES on i386 and x86_64

This patch automatically selects the assembly optimized version of AES (if selected) and the generic version can no longer be selected. The module will be called aes.ko

Signed-off-by Sebastian Siewior <[email protected]>
---
 arch/i386/crypto/Makefile               |    4 +-
 arch/i386/crypto/{aes.c => aes_key.c}   |    0 
 arch/x86_64/crypto/Makefile             |    5 +--
 arch/x86_64/crypto/{aes.c => aes_key.c} |    0 
 crypto/Kconfig                          |   46 +++++-------------------------
 crypto/Makefile                         |    2 +-
 6 files changed, 13 insertions(+), 44 deletions(-)
 rename arch/i386/crypto/{aes.c => aes_key.c} (100%)
 rename arch/x86_64/crypto/{aes.c => aes_key.c} (100%)
arch/i386/crypto/Makefile +2 -2
index 3fd19af..e725951 100644
@@ -4,9 +4,9 @@
 # Arch-specific CryptoAPI modules.
 # 
 
-obj-$(CONFIG_CRYPTO_AES_586) += aes-i586.o
+obj-$(CONFIG_CRYPTO_AES_586) += aes.o
 obj-$(CONFIG_CRYPTO_TWOFISH_586) += twofish-i586.o
 
-aes-i586-y := aes-i586-asm.o aes.o
+aes-y := aes-i586-asm.o aes_key.o
 twofish-i586-y := twofish-i586-asm.o twofish.o
arch/i386/crypto/aes_key.c renamed from arch/i386/crypto/aes.c +0 -0
similarity index 100%
rename from arch/i386/crypto/aes.c
rename to arch/i386/crypto/aes_key.c
arch/x86_64/crypto/Makefile +2 -3
index 15b538a..e34e716 100644
@@ -4,9 +4,8 @@
 # Arch-specific CryptoAPI modules.
 # 
 
-obj-$(CONFIG_CRYPTO_AES_X86_64) += aes-x86_64.o
+obj-$(CONFIG_CRYPTO_AES_X86_64) += aes.o
 obj-$(CONFIG_CRYPTO_TWOFISH_X86_64) += twofish-x86_64.o
 
-aes-x86_64-y := aes-x86_64-asm.o aes.o
+aes-y := aes-x86_64-asm.o aes_key.o
 twofish-x86_64-y := twofish-x86_64-asm.o twofish.o
-
arch/x86_64/crypto/aes_key.c renamed from arch/x86_64/crypto/aes.c +0 -0
similarity index 100%
rename from arch/x86_64/crypto/aes.c
rename to arch/x86_64/crypto/aes_key.c
crypto/Kconfig +8 -38

61 lines not on this page · raw message

crypto/Makefile +1 -1
index 0cf17f1..af44fd5 100644
@@ -37,7 +37,7 @@ obj-$(CONFIG_CRYPTO_BLOWFISH) += blowfish.o
 obj-$(CONFIG_CRYPTO_TWOFISH) += twofish.o
 obj-$(CONFIG_CRYPTO_TWOFISH_COMMON) += twofish_common.o
 obj-$(CONFIG_CRYPTO_SERPENT) += serpent.o
-obj-$(CONFIG_CRYPTO_AES) += aes.o
+obj-$(CONFIG_CRYPTO_AES_GENERIC) += aes.o
 obj-$(CONFIG_CRYPTO_CAMELLIA) += camellia.o
 obj-$(CONFIG_CRYPTO_CAST5) += cast5.o
 obj-$(CONFIG_CRYPTO_CAST6) += cast6.o
Signature · 1 line
1.5.3.rc7
author
#12 lore raw
Quoted 2 lines

What about:

Looks good.
-Andi

Quoted 2 lines

[crypto] do not use generic AES on i386 and x86_64

maintainer
#13 lore raw
In reply to Sebastian Siewior
Quoted 6 lines · Sebastian Siewior

[crypto] do not use generic AES on i386 and x86_64

This patch automatically selects the assembly optimized version of AES (if selected) and the generic version can no longer be selected. The module will be called aes.ko

You don't need to rename it because it already provides an alias for aes.

Also please provide a way to build the generic AES code so that it can at least be tested on i386/x86_64.

Thanks,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#14 lore raw
Quoted 10 lines · * Herbert Xu | 2007-09-19 20:29:43 [+0800]:
Quoted 6 lines · Sebastian Siewior

[crypto] do not use generic AES on i386 and x86_64

This patch automatically selects the assembly optimized version of AES (if selected) and the generic version can no longer be selected. The module will be called aes.ko

You don't need to rename it because it already provides an alias for aes.

Right, forgot about that.

Quoted 2 lines

Also please provide a way to build the generic AES code so that it can at least be tested on i386/x86_64.

You want to auto compile aes-x86_64 if you are on x86_64 and additonally the generic version. Is that correct?
If so, why do you want to have them both?

Sebastian

maintainer
#15 lore raw
Quoted 7 lines · Sebastian Siewior
Quoted 3 lines

Also please provide a way to build the generic AES code so that it can at least be tested on i386/x86_64.

You want to auto compile aes-x86_64 if you are on x86_64 and additonally the generic version. Is that correct?
If so, why do you want to have them both?

So that the generic C version can actually be tested on x86. We don't want it to bit-rot which would make non-x86 2nd-class citizens.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#16 lore raw
Quoted 3 lines · * Herbert Xu | 2007-09-20 08:20:58 [+0800]:

So that the generic C version can actually be tested on x86. We don't want it to bit-rot which would make non-x86 2nd-class citizens.

Okey. I try to post something what you might like in the new few days.

Quoted 1 line

Cheers,

Sebastian

#17 lore raw
Quoted 7 lines · * Sebastian Siewior | 2007-09-20 23:09:47 [+0200]:
Quoted 3 lines · * Herbert Xu | 2007-09-20 08:20:58 [+0800]:

So that the generic C version can actually be tested on x86. We don't want it to bit-rot which would make non-x86 2nd-class citizens.

Okey. I try to post something what you might like in the new few days.

This one should do it? Now you should not select both as module in order to get the assembly version auto loaded by the kernel :)

crypto/Kconfig +15 -36
@@ -286,6 +286,9 @@ config CRYPTO_SERPENT
 
 config CRYPTO_AES
 	tristate "AES cipher algorithms"
+	select CRYPTO_AES_586 if (X86 || UML_X86) && !64BIT
+	select CRYPTO_AES_X86_64 if (X86 || UML_X86) && 64BIT
+	select CRYPTO_AES_GENERIC if (!X86 && !UML_X86)
 	select CRYPTO_ALGAPI
 	help
 	  AES cipher algorithms (FIPS-197). AES uses the Rijndael 

@@ -304,47 +307,23 @@ config CRYPTO_AES
 
 	  See <http://csrc.nist.gov/CryptoToolkit/aes/> for more information.
 
-config CRYPTO_AES_586
-	tristate "AES cipher algorithms (i586)"
-	depends on (X86 || UML_X86) && !64BIT
-	select CRYPTO_ALGAPI
-	help
-	  AES cipher algorithms (FIPS-197). AES uses the Rijndael 
-	  algorithm.
-
-	  Rijndael appears to be consistently a very good performer in
-	  both hardware and software across a wide range of computing 
-	  environments regardless of its use in feedback or non-feedback 
-	  modes. Its key setup time is excellent, and its key agility is 
-	  good. Rijndael's very low memory requirements make it very well 
-	  suited for restricted-space environments, in which it also 
-	  demonstrates excellent performance. Rijndael's operations are 
-	  among the easiest to defend against power and timing attacks.	
-
-	  The AES specifies three key sizes: 128, 192 and 256 bits	  
+config CRYPTO_AES_GENERIC
+	tristate
 
-	  See <http://csrc.nist.gov/encryption/aes/> for more information.
-
-config CRYPTO_AES_X86_64
-	tristate "AES cipher algorithms (x86_64)"
-	depends on (X86 || UML_X86) && 64BIT
+config CRYPTO_AES_GENERIC_ENFORCE
+	tristate "AES cipher algorithm (generic Version)"
+	depends on (X86 || UML_X86)
+	select CRYPTO_AES_GENERIC
 	select CRYPTO_ALGAPI
 	help
-	  AES cipher algorithms (FIPS-197). AES uses the Rijndael 
-	  algorithm.
-
-	  Rijndael appears to be consistently a very good performer in
-	  both hardware and software across a wide range of computing 
-	  environments regardless of its use in feedback or non-feedback 
-	  modes. Its key setup time is excellent, and its key agility is 
-	  good. Rijndael's very low memory requirements make it very well 
-	  suited for restricted-space environments, in which it also 
-	  demonstrates excellent performance. Rijndael's operations are 
-	  among the easiest to defend against power and timing attacks.	
+	  This is the generic implementation of AES instead of the assembly
+	  optimized version.
 
-	  The AES specifies three key sizes: 128, 192 and 256 bits	  
+config CRYPTO_AES_586
+	tristate
 
-	  See <http://csrc.nist.gov/encryption/aes/> for more information.
+config CRYPTO_AES_X86_64
+	tristate
 
 config CRYPTO_CAST5
 	tristate "CAST5 (CAST-128) cipher algorithm"
crypto/Makefile +1 -1
@@ -37,7 +37,7 @@ obj-$(CONFIG_CRYPTO_BLOWFISH) += blowfis
 obj-$(CONFIG_CRYPTO_TWOFISH) += twofish.o
 obj-$(CONFIG_CRYPTO_TWOFISH_COMMON) += twofish_common.o
 obj-$(CONFIG_CRYPTO_SERPENT) += serpent.o
-obj-$(CONFIG_CRYPTO_AES) += aes.o
+obj-$(CONFIG_CRYPTO_AES_GENERIC) += aes.o
 obj-$(CONFIG_CRYPTO_CAMELLIA) += camellia.o
 obj-$(CONFIG_CRYPTO_CAST5) += cast5.o
 obj-$(CONFIG_CRYPTO_CAST6) += cast6.o
#18 lore raw
In reply to Herbert Xu
Quoted 7 lines · * Herbert Xu | 2007-08-20 20:06:05 [+0800]:
Quoted 1 line · Andi Kleen

I don't think modprobe knows anything about these priorities.

Right, in that case we'd only load one of them, usually the generic one since its name is what we're trying to load.

Quoted 8 lines
Quoted 2 lines

But that would require teaching the module loading user space about all this first, right?

That would be the best. However, it's not hard to do a simple probing in the kernel until modprobe(8) gets this feature.

I'll code something up.

Herbert, I tried to implement a MODULE_PRIO() macro which would be used by the module loader and I noticed that it may not be required. If there are two modules providing the same alias and we modprobe that alias than modprobe will load both of them. It may be a waste of memory if we load the generic algorithm and the assembly optimized one. However, it would be good for some hardware drivers which need the generic implementation for some "corner cases".
What do you thing?

Sebastian

maintainer
#19 lore raw
Quoted 9 lines · Sebastian Siewior

Herbert, I tried to implement a MODULE_PRIO() macro which would be used by the module loader and I noticed that it may not be required. If there are two modules providing the same alias and we modprobe that alias than modprobe will load both of them. It may be a waste of memory if we load the generic algorithm and the assembly optimized one. However, it would be good for some hardware drivers which need the generic implementation for some "corner cases".
What do you thing?

I think you're brilliant!

Could you please do a patch to rename aes.c to aes-generic.c and add the appropriate alias to it? Please add the aes alias to padlock and geode while you're at it.

Thanks,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#20 lore raw
Quoted 11 lines · * Herbert Xu | 2007-10-03 15:35:22 [+0800]:
Quoted 9 lines · Sebastian Siewior

Herbert, I tried to implement a MODULE_PRIO() macro which would be used by the module loader and I noticed that it may not be required. If there are two modules providing the same alias and we modprobe that alias than modprobe will load both of them. It may be a waste of memory if we load the generic algorithm and the assembly optimized one. However, it would be good for some hardware drivers which need the generic implementation for some "corner cases".
What do you thing?

Quoted 5 lines

I think you're brilliant!

Could you please do a patch to rename aes.c to aes-generic.c and add the appropriate alias to it? Please add the aes alias to padlock and geode while you're at it.

I am sorry that I did not reply earlier but I was busy framing that email :)
I send out rename patches for aes, des (s390 hw driver available) and sha1+252 (s390 + via padlock hw).
Two last questions:
- What about the i386 assembly vs generic implementation? Do you prefer the patch that I have send earlier (choose the assembly by default making the generic optional) or do you want both of them loaded at the same time.
- What might be the best fallback strategy for the s390 + geode aes driver?
I tried to move the key size to the algorithm request function what works good most of the time. All users except the wlan stack do alloc cipher followed by setkey in the same function. The benefit would be that you don't return a cipher that is not able to handle certain key sizes (and you don't have to implement the fallback in every driver (2 right now)) separately.
I started to implement a fallback within the driver (two is not that much right now) and wanted to make sure that this is the prefered way of fixing this.

Quoted 1 line

Thanks,

Sebastian

maintainer
#21 lore raw
Quoted 6 lines · Sebastian Siewior

Two last questions:
- What about the i386 assembly vs generic implementation? Do you prefer the patch that I have send earlier (choose the assembly by default making the generic optional) or do you want both of them loaded at the same time.

I'd prefer both to be built by default so that if something does go wrong we can ask people to check by using aes-generic.

Quoted 2 lines

- What might be the best fallback strategy for the s390 + geode aes driver?

I've been trying to avoid this :)

Yes your proposal is definitely on the right track.

My original plan is to have a template like keylen(aes,16) that only supports key length 16. So perhaps you can take a look at doing that or come up with a different solution.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#22 lore raw
Quoted 9 lines · Herbert Xu
Quoted 5 lines · Sebastian Siewior

Two last questions:
- What about the i386 assembly vs generic implementation? Do you prefer the patch that I have send earlier (choose the assembly by default making the generic optional) or do you want both of them loaded at the same time.

I'd prefer both to be built by default so that if something does go wrong we can ask people to check by using aes-generic.

Is that really needed? How often did you see a broken AES implementation? They tend to be well tested and high quality after all and I haven't ever seen any evidence that the assembler functions are any less stable than C. In fact they're probably more stable because they don't have to worry about being miscompiled.

I also think it is a bad idea to install the generic function by default -- it increases the risk the user ends up with a unnecessary slow implementation

-Andi

#23 lore raw
Quoted 11 lines · * Andi Kleen | 2007-10-04 11:31:56 [+0200]:
Quoted 9 lines · Herbert Xu
Quoted 5 lines · Sebastian Siewior

Two last questions:
- What about the i386 assembly vs generic implementation? Do you prefer the patch that I have send earlier (choose the assembly by default making the generic optional) or do you want both of them loaded at the same time.

I'd prefer both to be built by default so that if something does go wrong we can ask people to check by using aes-generic.

Quoted 8 lines

Is that really needed? How often did you see a broken AES implementation? They tend to be well tested and high quality after all and I haven't ever seen any evidence that the assembler functions are any less stable than C. In fact they're probably more stable because they don't have to worry about being miscompiled.

I also think it is a bad idea to install the generic function by default -- it increases the risk the user ends up with a unnecessary slow implementation

With the first patch I send earlier (where you wrote "it looks good to me") the assembly version is selected by default and the generic is optional (you don't have to compile it at all).
With the patch I've send today both variants (generic + assembly) are loaded (if available of course) and thr crypto API can decide which one is the best.

The end user will not end up with a slow implementation because the distro does not have to ship generic version plus if it does than both modules are loaded and crypto API picks the best.

Quoted 1 line

-Andi

Sebastian

maintainer
#24 lore raw
In reply to Andi Kleen
Quoted 3 lines · Andi Kleen

I also think it is a bad idea to install the generic function by default -- it increases the risk the user ends up with a unnecessary slow implementation

With Sebastian's change this won't be possible at all since modprobe will load all aliases.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#25 lore raw
In reply to Herbert Xu
Quoted 10 lines · * Herbert Xu | 2007-10-04 16:48:18 [+0800]:
Quoted 6 lines · Sebastian Siewior

Two last questions:
- What about the i386 assembly vs generic implementation? Do you prefer the patch that I have send earlier (choose the assembly by default making the generic optional) or do you want both of them loaded at the same time.

I'd prefer both to be built by default so that if something does go wrong we can ask people to check by using aes-generic.

That sounds fair to me. I don't touch twofish since it is not auto selected like aes is and the assembly version is available by default unless the user enabled both. Maybe a hint in Kconfig's help would help but I don't thing it is necessary. The distros should not compile it in first place :)

Quoted 10 lines
Quoted 2 lines

- What might be the best fallback strategy for the s390 + geode aes driver?

I've been trying to avoid this :)

Yes your proposal is definitely on the right track.

My original plan is to have a template like keylen(aes,16) that only supports key length 16. So perhaps you can take a look at doing that or come up with a different solution.

Okey. So I drop my changes to the driver and try to fix the crypto API like I was doing in first place.

Quoted 1 line

Cheers,

Sebastian

#26 lore raw

PATCH 1/1 load the AES module by an alias

In reply to Sebastian Siewior

Loading the crypto algorithm by the alias instead of by module directly has the advantage that all possible implementations of this algorithm are loaded automatically and the crypto API can choose the best one depending on its priority.
Additionally it ensures that the generic implementation as well as the HW driver (if available) is loaded in case the HW driver needs the generic version as fallback in corner cases.

Signed-off-by Sebastian Siewior <[email protected]>
---
 arch/s390/crypto/aes_s390.c     |    2 +-
 crypto/Makefile                 |    2 +-
 crypto/{aes.c => aes_generic.c} |    2 +-
 drivers/crypto/geode-aes.c      |    1 +
 drivers/crypto/padlock-aes.c    |    4 ++--
 5 files changed, 6 insertions(+), 5 deletions(-)
 rename crypto/{aes.c => aes_generic.c} (99%)
arch/s390/crypto/aes_s390.c +1 -1
index 3660ca6..5126696 100644
@@ -7,7 +7,7 @@
  *   Copyright IBM Corp. 2005,2007
  *   Author(s): Jan Glauber ([email protected])
  *
- * Derived from "crypto/aes.c"
+ * Derived from "crypto/aes_generic.c"
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms of the GNU General Public License as published by the Free
crypto/Makefile +1 -1
index e96a07e..7025bd5 100644
@@ -39,7 +39,7 @@ obj-$(CONFIG_CRYPTO_BLOWFISH) += blowfish.o
 obj-$(CONFIG_CRYPTO_TWOFISH) += twofish.o
 obj-$(CONFIG_CRYPTO_TWOFISH_COMMON) += twofish_common.o
 obj-$(CONFIG_CRYPTO_SERPENT) += serpent.o
-obj-$(CONFIG_CRYPTO_AES) += aes.o
+obj-$(CONFIG_CRYPTO_AES) += aes_generic.o
 obj-$(CONFIG_CRYPTO_CAMELLIA) += camellia.o
 obj-$(CONFIG_CRYPTO_CAST5) += cast5.o
 obj-$(CONFIG_CRYPTO_CAST6) += cast6.o
crypto/aes_generic.c renamed from crypto/aes.c +1 -1
similarity index 99%
rename from crypto/aes.c
rename to crypto/aes_generic.c
index e244077..9401dca 100644
@@ -453,4 +453,4 @@ module_exit(aes_fini);
 
 MODULE_DESCRIPTION("Rijndael (AES) Cipher Algorithm");
 MODULE_LICENSE("Dual BSD/GPL");
-
+MODULE_ALIAS("aes");
drivers/crypto/geode-aes.c +1 -0
index 6a86958..f9a34ab 100644
@@ -473,6 +473,7 @@ geode_aes_exit(void)
 MODULE_AUTHOR("Advanced Micro Devices, Inc.");
 MODULE_DESCRIPTION("Geode LX Hardware AES driver");
 MODULE_LICENSE("GPL");
+MODULE_ALIAS("aes");
 
 module_init(geode_aes_init);
 module_exit(geode_aes_exit);
drivers/crypto/padlock-aes.c +2 -2
index d4501dc..abbcff0 100644
@@ -5,7 +5,7 @@
  *
  * Copyright (c) 2004  Michal Ludvig <[email protected]>
  *
- * Key expansion routine taken from crypto/aes.c
+ * Key expansion routine taken from crypto/aes_generic.c
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by

@@ -660,4 +660,4 @@ MODULE_DESCRIPTION("VIA PadLock AES algorithm support");
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Michal Ludvig");
 
-MODULE_ALIAS("aes-padlock");
+MODULE_ALIAS("aes");
Signature · 1 line
1.5.3.2
maintainer
#27 lore raw

Re: [PATCH] [crypto] load the AES module by an alias

Quoted 9 lines · Sebastian Siewior

Loading the crypto algorithm by the alias instead of by module directly has the advantage that all possible implementations of this algorithm are loaded automatically and the crypto API can choose the best one depending on its priority.
Additionally it ensures that the generic implementation as well as the HW driver (if available) is loaded in case the HW driver needs the generic version as fallback in corner cases.

Signed-off-by Sebastian Siewior <[email protected]>

Patch applied. Thanks!

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#28 lore raw

PATCH 1/1 load the DES module by an alias

In reply to Sebastian Siewior

Loading the crypto algorithm by the alias instead of by module directly has the advantage that all possible implementations of this algorithm are loaded automatically and the crypto API can choose the best one depending on its priority.

Signed-off-by Sebastian Siewior <[email protected]>
---
 crypto/Makefile                 |    2 +-
 crypto/{des.c => des_generic.c} |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)
 rename crypto/{des.c => des_generic.c} (99%)
crypto/Makefile +1 -1
index 7025bd5..b6ef5e4 100644
@@ -33,7 +33,7 @@ obj-$(CONFIG_CRYPTO_PCBC) += pcbc.o
 obj-$(CONFIG_CRYPTO_LRW) += lrw.o
 obj-$(CONFIG_CRYPTO_XTS) += xts.o
 obj-$(CONFIG_CRYPTO_CRYPTD) += cryptd.o
-obj-$(CONFIG_CRYPTO_DES) += des.o
+obj-$(CONFIG_CRYPTO_DES) += des_generic.o
 obj-$(CONFIG_CRYPTO_FCRYPT) += fcrypt.o
 obj-$(CONFIG_CRYPTO_BLOWFISH) += blowfish.o
 obj-$(CONFIG_CRYPTO_TWOFISH) += twofish.o
crypto/des_generic.c renamed from crypto/des.c +1 -0
similarity index 99%
rename from crypto/des.c
rename to crypto/des_generic.c
index 1df3a71..59966d1 100644
@@ -1009,3 +1009,4 @@ module_exit(fini);
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("DES & Triple DES EDE Cipher Algorithms");
 MODULE_AUTHOR("Dag Arne Osvik <[email protected]>");
+MODULE_ALIAS("des");
Signature · 1 line
1.5.3.2
maintainer Applied
#29 lore raw

Re: [PATCH] [crypto] load the DES module by an alias

Quoted 6 lines · Sebastian Siewior

Loading the crypto algorithm by the alias instead of by module directly has the advantage that all possible implementations of this algorithm are loaded automatically and the crypto API can choose the best one depending on its priority.

Signed-off-by Sebastian Siewior <[email protected]>

Applied. Thanks!

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#30 lore raw

PATCH 1/1 load the SHA1[1|256] module by an alias

In reply to Sebastian Siewior

Loading the crypto algorithm by the alias instead of by module directly has the advantage that all possible implementations of this algorithm are loaded automatically and the crypto API can choose the best one depending on its priority.
Additionally it ensures that the generic implementation as well as the HW driver (if available) is loaded in case the HW driver needs the generic version as fallback in corner cases.

Signed-off-by Sebastian Siewior <[email protected]>
---
 arch/s390/crypto/sha1_s390.c          |    2 +-
 arch/s390/crypto/sha256_s390.c        |    2 +-
 crypto/Makefile                       |    4 ++--
 crypto/{sha1.c => sha1_generic.c}     |    2 +-
 crypto/{sha256.c => sha256_generic.c} |    2 +-
 drivers/crypto/padlock-sha.c          |    4 ++--
 6 files changed, 8 insertions(+), 8 deletions(-)
 rename crypto/{sha1.c => sha1_generic.c} (99%)
 rename crypto/{sha256.c => sha256_generic.c} (99%)
arch/s390/crypto/sha1_s390.c +1 -1
index af4460e..8ebd3cd 100644
@@ -12,7 +12,7 @@
  *   Author(s): Thomas Spatzier
  *		Jan Glauber ([email protected])
  *
- * Derived from "crypto/sha1.c"
+ * Derived from "crypto/sha1_generic.c"
  *   Copyright (c) Alan Smithee.
  *   Copyright (c) Andrew McDonald <[email protected]>
  *   Copyright (c) Jean-Francois Dive <[email protected]>
arch/s390/crypto/sha256_s390.c +1 -1
index 2ced333..c728bd0 100644
@@ -7,7 +7,7 @@
  *   Copyright IBM Corp. 2005,2007
  *   Author(s): Jan Glauber ([email protected])
  *
- * Derived from "crypto/sha256.c"
+ * Derived from "crypto/sha256_generic.c"
  * and "arch/s390/crypto/sha1_s390.c"
  *
  * This program is free software; you can redistribute it and/or modify it
crypto/Makefile +2 -2
index b6ef5e4..43c2a0d 100644
@@ -21,8 +21,8 @@ obj-$(CONFIG_CRYPTO_XCBC) += xcbc.o
 obj-$(CONFIG_CRYPTO_NULL) += crypto_null.o
 obj-$(CONFIG_CRYPTO_MD4) += md4.o
 obj-$(CONFIG_CRYPTO_MD5) += md5.o
-obj-$(CONFIG_CRYPTO_SHA1) += sha1.o
-obj-$(CONFIG_CRYPTO_SHA256) += sha256.o
+obj-$(CONFIG_CRYPTO_SHA1) += sha1_generic.o
+obj-$(CONFIG_CRYPTO_SHA256) += sha256_generic.o
 obj-$(CONFIG_CRYPTO_SHA512) += sha512.o
 obj-$(CONFIG_CRYPTO_WP512) += wp512.o
 obj-$(CONFIG_CRYPTO_TGR192) += tgr192.o
crypto/sha1_generic.c renamed from crypto/sha1.c +1 -1
similarity index 99%
rename from crypto/sha1.c
rename to crypto/sha1_generic.c
index 1bba551..70364dd 100644
@@ -139,4 +139,4 @@ module_exit(fini);
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("SHA1 Secure Hash Algorithm");
 
-MODULE_ALIAS("sha1-generic");
+MODULE_ALIAS("sha1");
crypto/sha256_generic.c renamed from crypto/sha256.c +1 -1
similarity index 99%
rename from crypto/sha256.c
rename to crypto/sha256_generic.c
index 716195b..74bf2f9 100644
@@ -339,4 +339,4 @@ module_exit(fini);
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("SHA256 Secure Hash Algorithm");
 
-MODULE_ALIAS("sha256-generic");
+MODULE_ALIAS("sha256");
drivers/crypto/padlock-sha.c +2 -2
index a781fd2..b8ee645 100644
@@ -314,5 +314,5 @@ MODULE_DESCRIPTION("VIA PadLock SHA1/SHA256 algorithms support.");
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Michal Ludvig");
 
-MODULE_ALIAS("sha1-padlock");
-MODULE_ALIAS("sha256-padlock");
+MODULE_ALIAS("sha1");
+MODULE_ALIAS("sha256");
Signature · 1 line
1.5.3.2
maintainer
#31 lore raw

Re: [PATCH] [crypto] load the SHA1[1|256] module by an alias

Quoted 9 lines · Sebastian Siewior

Loading the crypto algorithm by the alias instead of by module directly has the advantage that all possible implementations of this algorithm are loaded automatically and the crypto API can choose the best one depending on its priority.
Additionally it ensures that the generic implementation as well as the HW driver (if available) is loaded in case the HW driver needs the generic version as fallback in corner cases.

Signed-off-by Sebastian Siewior <[email protected]>

This patch is correct per se but it combined with the code in padlock-sha can cause a dead-lock. Padlock-sha tries to load the generic sha module in its init function. At this point it is still holding the module_mutex so the probe will dead-lock.

The probe is actually pointless since we can always probe when the algorithm is actually used which does not lead to dead-locks like this.

So please delete the padlock_sha_check_fallbacks code in your patch to make it load correctly.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#32 lore raw

Re: [PATCH] [crypto] load the SHA1[1|256] module by an alias

Quoted 4 lines · * Herbert Xu | 2007-10-05 16:57:44 [+0800]:

This patch is correct per se but it combined with the code in padlock-sha can cause a dead-lock. Padlock-sha tries to load the generic sha module in its init function. At this point it is still holding the module_mutex so the probe will dead-lock.

I did not find a module_mutex. We hold a readlock of crypto_alg_sem within the crypto subsystem and request_module() increments kmod_concurrent (which may not exceed a certain limit).

Quoted 3 lines

The probe is actually pointless since we can always probe when the algorithm is actually used which does not lead to dead-locks like this.

Right.

Quoted 2 lines

So please delete the padlock_sha_check_fallbacks code in your patch to make it load correctly.

okey, updated patch will follow.

Quoted 1 line

Cheers,

Sebastian

#33 lore raw

PATCH 1/1 load the SHA1[1|256] module by an alias (v2)

Loading the crypto algorithm by the alias instead of by module directly has the advantage that all possible implementations of this algorithm are loaded automatically and the crypto API can choose the best one depending on its priority.
Additionally it ensures that the generic implementation as well as the HW driver (if available) is loaded in case the HW driver needs the generic version as fallback in corner cases.
Also remove the probe for sha1 in padlock's init code.
Quote from Herbert:
The probe is actually pointless since we can always probe when the algorithm is actually used which does not lead to dead-locks like this.

Signed-off-by Sebastian Siewior <[email protected]>
---
 arch/s390/crypto/sha1_s390.c          |    2 +-
 arch/s390/crypto/sha256_s390.c        |    2 +-
 crypto/Makefile                       |    4 ++--
 crypto/{sha1.c => sha1_generic.c}     |    2 +-
 crypto/{sha256.c => sha256_generic.c} |    8 ++++----
 drivers/crypto/padlock-sha.c          |   19 ++-----------------
 6 files changed, 11 insertions(+), 26 deletions(-)
 rename crypto/{sha1.c => sha1_generic.c} (99%)
 rename crypto/{sha256.c => sha256_generic.c} (99%)
arch/s390/crypto/sha1_s390.c +1 -1
index af4460e..8ebd3cd 100644
@@ -12,7 +12,7 @@
  *   Author(s): Thomas Spatzier
  *		Jan Glauber ([email protected])
  *
- * Derived from "crypto/sha1.c"
+ * Derived from "crypto/sha1_generic.c"
  *   Copyright (c) Alan Smithee.
  *   Copyright (c) Andrew McDonald <[email protected]>
  *   Copyright (c) Jean-Francois Dive <[email protected]>
arch/s390/crypto/sha256_s390.c +1 -1
index 2ced333..c728bd0 100644
@@ -7,7 +7,7 @@
  *   Copyright IBM Corp. 2005,2007
  *   Author(s): Jan Glauber ([email protected])
  *
- * Derived from "crypto/sha256.c"
+ * Derived from "crypto/sha256_generic.c"
  * and "arch/s390/crypto/sha1_s390.c"
  *
  * This program is free software; you can redistribute it and/or modify it
crypto/Makefile +2 -2
index b6ef5e4..43c2a0d 100644
@@ -21,8 +21,8 @@ obj-$(CONFIG_CRYPTO_XCBC) += xcbc.o
 obj-$(CONFIG_CRYPTO_NULL) += crypto_null.o
 obj-$(CONFIG_CRYPTO_MD4) += md4.o
 obj-$(CONFIG_CRYPTO_MD5) += md5.o
-obj-$(CONFIG_CRYPTO_SHA1) += sha1.o
-obj-$(CONFIG_CRYPTO_SHA256) += sha256.o
+obj-$(CONFIG_CRYPTO_SHA1) += sha1_generic.o
+obj-$(CONFIG_CRYPTO_SHA256) += sha256_generic.o
 obj-$(CONFIG_CRYPTO_SHA512) += sha512.o
 obj-$(CONFIG_CRYPTO_WP512) += wp512.o
 obj-$(CONFIG_CRYPTO_TGR192) += tgr192.o
crypto/sha1_generic.c renamed from crypto/sha1.c +1 -1
similarity index 99%
rename from crypto/sha1.c
rename to crypto/sha1_generic.c
index 1bba551..70364dd 100644
@@ -139,4 +139,4 @@ module_exit(fini);
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("SHA1 Secure Hash Algorithm");
 
-MODULE_ALIAS("sha1-generic");
+MODULE_ALIAS("sha1");
crypto/sha256_generic.c renamed from crypto/sha256.c +1 -1
similarity index 99%
rename from crypto/sha256.c
rename to crypto/sha256_generic.c
index 716195b..c7097dd 100644
@@ -339,4 +339,4 @@ module_exit(fini);
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("SHA256 Secure Hash Algorithm");
 
-MODULE_ALIAS("sha256-generic");
+MODULE_ALIAS("sha256");
drivers/crypto/padlock-sha.c +2 -17
index a781fd2..bc1de34 100644
@@ -253,19 +253,6 @@ static struct crypto_alg sha256_alg = {
 	}
 };
 
-static void __init padlock_sha_check_fallbacks(void)
-{
-	if (!crypto_has_hash("sha1", 0, CRYPTO_ALG_ASYNC |
-					CRYPTO_ALG_NEED_FALLBACK))
-		printk(KERN_WARNING PFX
-		       "Couldn't load fallback module for sha1.\n");
-
-	if (!crypto_has_hash("sha256", 0, CRYPTO_ALG_ASYNC |
-					CRYPTO_ALG_NEED_FALLBACK))
-		printk(KERN_WARNING PFX
-		       "Couldn't load fallback module for sha256.\n");
-}
-
 static int __init padlock_init(void)
 {
 	int rc = -ENODEV;

@@ -280,8 +267,6 @@ static int __init padlock_init(void)
 		return -ENODEV;
 	}
 
-	padlock_sha_check_fallbacks();
-
 	rc = crypto_register_alg(&sha1_alg);
 	if (rc)
 		goto out;

@@ -314,5 +299,5 @@ MODULE_DESCRIPTION("VIA PadLock SHA1/SHA256 algorithms support.");
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Michal Ludvig");
 
-MODULE_ALIAS("sha1-padlock");
-MODULE_ALIAS("sha256-padlock");
+MODULE_ALIAS("sha1");
+MODULE_ALIAS("sha256");
Signature · 1 line
1.5.3.2
maintainer
#34 lore raw

Re: [PATCH] [crypto] load the SHA1[1|256] module by an alias (v2)

Quoted 4 lines · Sebastian Siewior

Loading the crypto algorithm by the alias instead of by module directly has the advantage that all possible implementations of this algorithm are loaded automatically and the crypto API can choose the best one depending on its priority.

Quoted 8 lines

Additionally it ensures that the generic implementation as well as the HW driver (if available) is loaded in case the HW driver needs the generic version as fallback in corner cases.
Also remove the probe for sha1 in padlock's init code. Quote from Herbert:
The probe is actually pointless since we can always probe when the algorithm is actually used which does not lead to dead-locks like this.

Thanks. I'll apply this tomorrow.

BTW, the dead-lock does exist after all. The lock that's held is the user-space file lock held by modprobe. Assuming that padlock-sha comes before sha in the alias list, then the second modprobe will hit padlock-sha again and dead-lock.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#35 lore raw

Re: [PATCH] [crypto] load the SHA1[1|256] module by an alias (v2)

Quoted 14 lines · * Herbert Xu | 2007-10-05 23:10:51 [+0800]:
Quoted 4 lines · Sebastian Siewior

Loading the crypto algorithm by the alias instead of by module directly has the advantage that all possible implementations of this algorithm are loaded automatically and the crypto API can choose the best one depending on its priority.

Quoted 8 lines

Additionally it ensures that the generic implementation as well as the HW driver (if available) is loaded in case the HW driver needs the generic version as fallback in corner cases.
Also remove the probe for sha1 in padlock's init code. Quote from Herbert:
The probe is actually pointless since we can always probe when the algorithm is actually used which does not lead to dead-locks like this.

Quoted 6 lines

Thanks. I'll apply this tomorrow.

BTW, the dead-lock does exist after all. The lock that's held is the user-space file lock held by modprobe. Assuming that padlock-sha comes before sha in the alias list, then the second modprobe will hit padlock-sha again and dead-lock.

Really, hmm. That CRYPTO_ALG_NEED_FALLBACK flag is unused than? I try to remove the hardware specific code and test the via-sha code as it once I have some time in my hands :)

Quoted 1 line

Cheers,

Sebastian

maintainer
#36 lore raw

Re: [PATCH] [crypto] load the SHA1[1|256] module by an alias (v2)

Quoted 7 lines · Sebastian Siewior
Quoted 5 lines

BTW, the dead-lock does exist after all. The lock that's held is the user-space file lock held by modprobe. Assuming that padlock-sha comes before sha in the alias list, then the second modprobe will hit padlock-sha again and dead-lock.

Really, hmm. That CRYPTO_ALG_NEED_FALLBACK flag is unused than?

That flag is only known to the crypto code. Modprobe knows nothing of it so the two probes will look the same to modprobe.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#37 lore raw

Re: [PATCH] [crypto] load the SHA1[1|256] module by an alias (v2)

In reply to Herbert Xu
Quoted 4 lines · * Herbert Xu | 2007-10-05 23:10:51 [+0800]:

BTW, the dead-lock does exist after all. The lock that's held is the user-space file lock held by modprobe. Assuming that padlock-sha comes before sha in the alias list, then the second modprobe will hit padlock-sha again and dead-lock.

I tried to deadlock and I did not succeed, maybe I did something wrong.

# fgrep sha modules.alias 
alias sha256 padlock_sha
alias sha1 padlock_sha
alias sha384 sha512
alias sha256 sha256_generic
alias sha1 sha1_generic

padlock comes first as you said. A "modprobe tcrypt mode=2" worked fine (with the test passed).

# lsmod |grep sha
sha1_generic            3008  0 
padlock_sha             4160  0 
crypto_algapi          13824  4
sha1_generic,padlock_sha,blkcipher,cryptomgr

I have a version of the padlock-sha driver with no HW dependency (fallback only, that one I used) at

git://git.breakpoint.cc/bigeasy/linux via_sha

if you want to test :)

Quoted 1 line

Cheers,

Sebastian

maintainer
#38 lore raw

Re: [PATCH] [crypto] load the SHA1[1|256] module by an alias (v2)

Quoted 9 lines · Sebastian Siewior

I tried to deadlock and I did not succeed, maybe I did something wrong.

# fgrep sha modules.alias 
alias sha256 padlock_sha
alias sha1 padlock_sha
alias sha384 sha512
alias sha256 sha256_generic
alias sha1 sha1_generic

Try reversing this. modprobe aliases are added to the head of the list so they'll come out in reverse.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#39 lore raw

Re: [PATCH] [crypto] load the SHA1[1|256] module by an alias (v2)

Quoted 11 lines · * Herbert Xu | 2007-10-08 11:20:17 [+0800]:
Quoted 9 lines · Sebastian Siewior

I tried to deadlock and I did not succeed, maybe I did something wrong.

# fgrep sha modules.alias 
alias sha256 padlock_sha
alias sha1 padlock_sha
alias sha384 sha512
alias sha256 sha256_generic
alias sha1 sha1_generic
Quoted 2 lines

Try reversing this. modprobe aliases are added to the head of the list so they'll come out in reverse.

# fgrep sha modules.alias
alias sha1 sha1_generic
alias sha256 sha256_generic
alias sha384 sha512
alias sha1 padlock_sha
alias sha256 padlock_sha

Still no dead-lock.

Quoted 1 line

Cheers,

Sebastian

maintainer
#40 lore raw

Re: [PATCH] [crypto] load the SHA1[1|256] module by an alias (v2)

In reply to Sebastian Siewior
Quoted 13 lines · Sebastian Siewior

Loading the crypto algorithm by the alias instead of by module directly has the advantage that all possible implementations of this algorithm are loaded automatically and the crypto API can choose the best one depending on its priority.
Additionally it ensures that the generic implementation as well as the HW driver (if available) is loaded in case the HW driver needs the generic version as fallback in corner cases.
Also remove the probe for sha1 in padlock's init code.
Quote from Herbert:
The probe is actually pointless since we can always probe when the algorithm is actually used which does not lead to dead-locks like this.

Quoted 1 line
Signed-off-by Sebastian Siewior <[email protected]>

Patch applied with one small change where I left the *-padlock aliases around since people might already be using them and they're correct as the specific name of those algorithms.

Thanks,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
maintainer
#41 lore raw

Re: [PATCH] [crypto] load the SHA1[1|256] module by an alias

In reply to Sebastian Siewior
Quoted 4 lines · Sebastian Siewior

I did not find a module_mutex. We hold a readlock of crypto_alg_sem within the crypto subsystem and request_module() increments kmod_concurrent (which may not exceed a certain limit).

Yes you're right. The module_mutex (in kernel/module.c) is dropped for the init call so the dead-lock won't actually occur. However, it's still good to remove this code since doing a nested modprobe of "sha" within a modprobe of "sha" just feels wrong.

Thanks,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#42 lore raw

Re: [PATCH] [crypto] load the SHA1[1|256] module by an alias

Quoted 8 lines · * Herbert Xu | 2007-10-05 22:20:22 [+0800]:
Quoted 4 lines · Sebastian Siewior

I did not find a module_mutex. We hold a readlock of crypto_alg_sem within the crypto subsystem and request_module() increments kmod_concurrent (which may not exceed a certain limit).

However, it's still good to remove this code since doing a nested modprobe of "sha" within a modprobe of "sha" just feels wrong.

Yes it feels wrong. It even may show that the fallback is not available if padlock is loaded before the generic ones.

Sebastian

#43 lore raw

Re: [PATCH] [crypto] load the SHA1[1|256] module by an alias

In reply to Sebastian Siewior

Hi Sebastian,

what about SHA-512? Should that not also be renamed to sha512_generic.c ?

--Jan

Quoted 99 lines · Sebastian Siewior

Loading the crypto algorithm by the alias instead of by module directly has the advantage that all possible implementations of this algorithm are loaded automatically and the crypto API can choose the best one depending on its priority.
Additionally it ensures that the generic implementation as well as the HW driver (if available) is loaded in case the HW driver needs the generic version as fallback in corner cases.

Signed-off-by Sebastian Siewior <[email protected]>
---
 arch/s390/crypto/sha1_s390.c          |    2 +-
 arch/s390/crypto/sha256_s390.c        |    2 +-
 crypto/Makefile                       |    4 ++--
 crypto/{sha1.c => sha1_generic.c}     |    2 +-
 crypto/{sha256.c => sha256_generic.c} |    2 +-
 drivers/crypto/padlock-sha.c          |    4 ++--
 6 files changed, 8 insertions(+), 8 deletions(-)
 rename crypto/{sha1.c => sha1_generic.c} (99%)
 rename crypto/{sha256.c => sha256_generic.c} (99%)
arch/s390/crypto/sha1_s390.c +1 -1
index af4460e..8ebd3cd 100644
@@ -12,7 +12,7 @@
  *   Author(s): Thomas Spatzier
  *		Jan Glauber ([email protected])
  *
- * Derived from "crypto/sha1.c"
+ * Derived from "crypto/sha1_generic.c"
  *   Copyright (c) Alan Smithee.
  *   Copyright (c) Andrew McDonald <[email protected]>
  *   Copyright (c) Jean-Francois Dive <[email protected]>
arch/s390/crypto/sha256_s390.c +1 -1
index 2ced333..c728bd0 100644
@@ -7,7 +7,7 @@
  *   Copyright IBM Corp. 2005,2007
  *   Author(s): Jan Glauber ([email protected])
  *
- * Derived from "crypto/sha256.c"
+ * Derived from "crypto/sha256_generic.c"
  * and "arch/s390/crypto/sha1_s390.c"
  *
  * This program is free software; you can redistribute it and/or modify it
crypto/Makefile +2 -2
index b6ef5e4..43c2a0d 100644
@@ -21,8 +21,8 @@ obj-$(CONFIG_CRYPTO_XCBC) += xcbc.o
 obj-$(CONFIG_CRYPTO_NULL) += crypto_null.o
 obj-$(CONFIG_CRYPTO_MD4) += md4.o
 obj-$(CONFIG_CRYPTO_MD5) += md5.o
-obj-$(CONFIG_CRYPTO_SHA1) += sha1.o
-obj-$(CONFIG_CRYPTO_SHA256) += sha256.o
+obj-$(CONFIG_CRYPTO_SHA1) += sha1_generic.o
+obj-$(CONFIG_CRYPTO_SHA256) += sha256_generic.o
 obj-$(CONFIG_CRYPTO_SHA512) += sha512.o
 obj-$(CONFIG_CRYPTO_WP512) += wp512.o
 obj-$(CONFIG_CRYPTO_TGR192) += tgr192.o
crypto/sha1_generic.c renamed from crypto/sha1.c +1 -1
similarity index 99%
rename from crypto/sha1.c
rename to crypto/sha1_generic.c
index 1bba551..70364dd 100644
@@ -139,4 +139,4 @@ module_exit(fini);
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("SHA1 Secure Hash Algorithm");
 
-MODULE_ALIAS("sha1-generic");
+MODULE_ALIAS("sha1");
crypto/sha256_generic.c renamed from crypto/sha256.c +1 -1
similarity index 99%
rename from crypto/sha256.c
rename to crypto/sha256_generic.c
index 716195b..74bf2f9 100644
@@ -339,4 +339,4 @@ module_exit(fini);
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("SHA256 Secure Hash Algorithm");
 
-MODULE_ALIAS("sha256-generic");
+MODULE_ALIAS("sha256");
drivers/crypto/padlock-sha.c +2 -2
index a781fd2..b8ee645 100644
@@ -314,5 +314,5 @@ MODULE_DESCRIPTION("VIA PadLock SHA1/SHA256 algorithms support.");
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Michal Ludvig");
 
-MODULE_ALIAS("sha1-padlock");
-MODULE_ALIAS("sha256-padlock");
+MODULE_ALIAS("sha1");
+MODULE_ALIAS("sha256");
Signature · 6 lines
++49 7031 161911

IBM Deutschland Entwicklung GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter Geschäftsführung: Herbert
Kircher, Sitz der Gesellschaft: Böblingen, Registergericht Amtsgericht
Stuttgart, HRB 243294
#44 lore raw

Re: [PATCH] [crypto] load the SHA1[1|256] module by an alias

Quoted 1 line · * Jan Glauber | 2007-10-08 13:25:23 [+0200]:

Hi Sebastian,

Hi Jan, good to see you back :)

Quoted 1 line

what about SHA-512? Should that not also be renamed to sha512_generic.c ?

We could do it, but I haven't seen a HW implementation yet. If your new CPU has an opcode for this, we could rename it than :)

Quoted 2 lines

--Jan

Sebastian