Re: Initcall ordering problem (TTY vs modprobe vs MD5) and cryptomgr problem
On Fri, Aug 6, 2010 at 1:06 AM, Olivier Galibert wrote: > > Maybe Linus would be happier if the self-tests were limited (by > default) to the hardware accelerators? Having a software backup and > the risk of data loss indeed makes things different. No. I'd be happier if it was an OPTION. And it damn well defaults to "off". Like all other options. Then, for people who use it, and worry (and distro test kernels etc), turn it on. But don't make it a forced feature, and don't make it something that people think they _should_ turn on. I have crypto enabled, but I don't _use_ it. The upside for me is zero. Nil. Nada. And I bet that's the common case. And dammit, it you don't trust the hardware, don't send the driver upstreams. And if you worry about alpha-particles, you should run a RAM test on every boot. But don't ask _me_ to run one. It's that simple. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Initcall ordering problem (TTY vs modprobe vs MD5) and cryptomgr problem
On Fri, Aug 06, 2010 at 04:39:00PM +0800, Herbert Xu (herb...@gondor.apana.org.au) wrote: > So you'd rather have a box that doesn't boot rather than one > that automatically falls back to software crypto allowing you > to diagnose and report the problem. What about kernel boot parameter to test hardware accelerators or not? -- Evgeniy Polyakov -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Initcall ordering problem (TTY vs modprobe vs MD5) and cryptomgr problem
Olivier Galibert wrote: > > Of course in practice without the tests your boot would probably just > have failed. Badly-decrypted root partitions tend to be noticed as > such long before trying to write to them. Then you would have bitched > on the list and the driver would have been fixed or removed faster > than having to wait for you (or other people with the hardware issue) > to notice the spew in dmesg. So you'd rather have a box that doesn't boot rather than one that automatically falls back to software crypto allowing you to diagnose and report the problem. Yes that makes a lot of sense. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Initcall ordering problem (TTY vs modprobe vs MD5) and cryptomgr problem
On Fri, Aug 06, 2010 at 12:50:04AM -0400, Kyle Moffett wrote: > You should also realize that crypto drivers are very much *NOT* in the > same situation as most other drivers. Without this test, adding a new > crypto hardware driver to the kernel is a completely unsafe operation, > because it could completely break users setups. You have previously > said you're fine accepting new drivers even after the initial merge > window because they can't break anything, but in crypto that's not > true. Maybe Linus would be happier if the self-tests were limited (by default) to the hardware accelerators? Having a software backup and the risk of data loss indeed makes things different. Of course in practice without the tests your boot would probably just have failed. Badly-decrypted root partitions tend to be noticed as such long before trying to write to them. Then you would have bitched on the list and the driver would have been fixed or removed faster than having to wait for you (or other people with the hardware issue) to notice the spew in dmesg. OG. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Initcall ordering problem (TTY vs modprobe vs MD5) and cryptomgr problem
On Fri, Aug 6, 2010 at 00:20, Linus Torvalds wrote: > On Thu, Aug 5, 2010 at 7:35 PM, Herbert Xu > wrote: >> Because it can save data. Each cryptographic algorithm (such as >> AES) may have multiple impelmentations, some of which are hardware- >> based. > > Umm. The _developer_ had better test the thing. That is absolutely > _zero_ excuse for then forcing every boot for every poor user to re-do > the test over and over again. > > Guys, this comes up every single time: you as a developer may think > that your code is really important, but get over yourself already. > It's not so important that everybody must be forced to do it. Speaking as a user whose been bitten several times by bad crypto implementations, I'd personally rather have this testing on by default (if the crypto API it depends on is on). It's pretty damn inexpensive to do a few brief crypto operations during initialization as a quick smoke test. We already do something somewhat similar when loading the RAID5/RAID6 driver, although admittedly that's a speed-test for picking an optimized algorithm. You should also realize that crypto drivers are very much *NOT* in the same situation as most other drivers. Without this test, adding a new crypto hardware driver to the kernel is a completely unsafe operation, because it could completely break users setups. You have previously said you're fine accepting new drivers even after the initial merge window because they can't break anything, but in crypto that's not true. I've actually had it trigger in exactly the described situation. I had a box with an encrypted filesystem that I downloaded a new distro kernel on with new drivers. The new kernel included a bunch of new "EXPERIMENTAL" drivers for hardware, none of which I thought I cared about until I noticed in "dmesg" that one of them was getting enabled and then failing tests. So there are unique and compelling reasons for default-enabled basic smoke tests of cryptographic support during boot. To be honest, the test and integration engineer in me would like it if there were more intensive in-kernel POST tests that could be enabled by a kernel parameter or something for high-reliability embedded devices. Cheers, Kyle Moffett -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Initcall ordering problem (TTY vs modprobe vs MD5) and cryptomgr problem
On Thu, Aug 5, 2010 at 7:35 PM, Herbert Xu wrote: > > Because it can save data. Each cryptographic algorithm (such as > AES) may have multiple impelmentations, some of which are hardware- > based. Umm. The _developer_ had better test the thing. That is absolutely _zero_ excuse for then forcing every boot for every poor user to re-do the test over and over again. Guys, this comes up every single time: you as a developer may think that your code is really important, but get over yourself already. It's not so important that everybody must be forced to do it. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Initcall ordering problem (TTY vs modprobe vs MD5) and cryptomgr problem
On Thu, Aug 05, 2010 at 07:01:21PM -0700, Linus Torvalds wrote: > On Thu, Aug 5, 2010 at 6:40 PM, Herbert Xu > wrote: > > > > -config CRYPTO_MANAGER_TESTS > > - bool "Run algolithms' self-tests" > > - default y > > - depends on CRYPTO_MANAGER2 > > +config CRYPTO_MANAGER_DISABLE_TESTS > > + bool "Disable run-time self tests" > > + depends on CRYPTO_MANAGER2 && EMBEDDED > > Why do you still want to force-enable those tests? I was going to > complain about the "default y" anyway, now I'm _really_ complaining, > because you've now made it impossible to disable those tests. Why? Because it can save data. Each cryptographic algorithm (such as AES) may have multiple impelmentations, some of which are hardware- based. The purpose of these tests are to make a particular driver or implementation available only if it passes them. So your encrypted disk/file system will not be subject to a hardware/software combo without at least some semblance of testing. The last thing you want to is to upgrade your kernel with a new hardware crypto driver that detects that you have a piece of rarely- used crypto hardeware, decides to use it and ends up making your data toast. But whatever, if you want the default to be no tests, that's fine. Here's the patch to do just that. commit 00ca28a507b215dcd121735f16764ea4173c4ff9 Author: Herbert Xu Date: Fri Aug 6 10:34:00 2010 +0800 crypto: testmgr - Default to no tests On Thu, Aug 05, 2010 at 07:01:21PM -0700, Linus Torvalds wrote: > On Thu, Aug 5, 2010 at 6:40 PM, Herbert Xu wrote: > > > > -config CRYPTO_MANAGER_TESTS > > - bool "Run algolithms' self-tests" > > - default y > > - depends on CRYPTO_MANAGER2 > > +config CRYPTO_MANAGER_DISABLE_TESTS > > + bool "Disable run-time self tests" > > + depends on CRYPTO_MANAGER2 && EMBEDDED > > Why do you still want to force-enable those tests? I was going to > complain about the "default y" anyway, now I'm _really_ complaining, > because you've now made it impossible to disable those tests. Why? As requested, this patch sets the default to y and removes the EMBEDDED dependency. Signed-off-by: Herbert Xu diff --git a/crypto/Kconfig b/crypto/Kconfig index 6f5c50f..e573077 100644 --- a/crypto/Kconfig +++ b/crypto/Kconfig @@ -103,7 +103,8 @@ config CRYPTO_MANAGER2 config CRYPTO_MANAGER_DISABLE_TESTS bool "Disable run-time self tests" - depends on CRYPTO_MANAGER2 && EMBEDDED + default y + depends on CRYPTO_MANAGER2 help Disable run-time self tests that normally take place at algorithm registration. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Initcall ordering problem (TTY vs modprobe vs MD5) and cryptomgr problem
On Thu, Aug 5, 2010 at 7:23 PM, David Howells wrote: > > I wonder if tty_init() should be moved up, perhaps to immediately after > chrdev_init(). I do think that sounds sane. The tty layer is kind of special. I wouldn't call it _after_ chrdev_init(), though, I'd call it _from_ chrdev_init(). Doesn't that make more sense (and keep it out of fs/dcache.c, which is an odd place to have some tty init). But maybe there's some reason why it's an initcall. Unlikely. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Initcall ordering problem (TTY vs modprobe vs MD5) and cryptomgr problem
Herbert Xu wrote: > This patch should do the trick: > > commit 326a6346ffb5b19eb593530d9d3096d409e46f62 > Author: Herbert Xu > Date: Fri Aug 6 09:40:28 2010 +0800 > > crypto: testmgr - Fix test disabling option It does work. David -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Initcall ordering problem (TTY vs modprobe vs MD5) and cryptomgr problem
Linus Torvalds wrote: > People always think that their magical code is so important. I tell > you up-front that is absolutely is not. Just remove the crap entirely, > please. Even if he does remove it, that still leaves the problem that modprobe can be invoked and fail before tty_init() gets called:-/ I wonder if tty_init() should be moved up, perhaps to immediately after chrdev_init(). David -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Initcall ordering problem (TTY vs modprobe vs MD5) and cryptomgr problem
On Thu, Aug 5, 2010 at 6:40 PM, Herbert Xu wrote: > > -config CRYPTO_MANAGER_TESTS > - bool "Run algolithms' self-tests" > - default y > - depends on CRYPTO_MANAGER2 > +config CRYPTO_MANAGER_DISABLE_TESTS > + bool "Disable run-time self tests" > + depends on CRYPTO_MANAGER2 && EMBEDDED Why do you still want to force-enable those tests? I was going to complain about the "default y" anyway, now I'm _really_ complaining, because you've now made it impossible to disable those tests. Why? People always think that their magical code is so important. I tell you up-front that is absolutely is not. Just remove the crap entirely, please. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Initcall ordering problem (TTY vs modprobe vs MD5) and cryptomgr problem
On Fri, Aug 06, 2010 at 09:17:06AM +0800, Herbert Xu wrote:
> On Fri, Aug 06, 2010 at 02:01:03AM +0100, David Howells wrote:
> >
> > but it's not being found by the crypto routines. By GIT bisection, I note
> > that this problem is introduced in the following commit:
> >
> > commit 0b767f96164b2b27488e3daa722ff16e89d49314
> > Author: Alexander Shishkin
> > Date: Thu Jun 3 20:53:43 2010 +1000
> >
> > crypto: testmgr - add an option to disable cryptoalgos' self-tests
>
> Indeed this changeset is buggy. It makes cryptomgr return a value
> as if it was not loaded. This triggers the module load.
>
> I'll send you a patch.
This patch should do the trick:
commit 326a6346ffb5b19eb593530d9d3096d409e46f62
Author: Herbert Xu
Date: Fri Aug 6 09:40:28 2010 +0800
crypto: testmgr - Fix test disabling option
This patch fixes a serious bug in the test disabling patch where
it can cause an spurious load of the cryptomgr module even when
it's compiled in.
It also negates the test disabling option so that its absence
causes tests to be enabled.
The Kconfig option is also now behind EMBEDDED.
Signed-off-by: Herbert Xu
diff --git a/crypto/Kconfig b/crypto/Kconfig
index 1cd497d..6f5c50f 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -101,13 +101,12 @@ config CRYPTO_MANAGER2
select CRYPTO_BLKCIPHER2
select CRYPTO_PCOMP2
-config CRYPTO_MANAGER_TESTS
- bool "Run algolithms' self-tests"
- default y
- depends on CRYPTO_MANAGER2
+config CRYPTO_MANAGER_DISABLE_TESTS
+ bool "Disable run-time self tests"
+ depends on CRYPTO_MANAGER2 && EMBEDDED
help
- Run cryptomanager's tests for the new crypto algorithms being
- registered.
+ Disable run-time self tests that normally take place at
+ algorithm registration.
config CRYPTO_GF128MUL
tristate "GF(2^128) multiplication functions (EXPERIMENTAL)"
diff --git a/crypto/algboss.c b/crypto/algboss.c
index 40bd391..791d194 100644
--- a/crypto/algboss.c
+++ b/crypto/algboss.c
@@ -206,13 +206,16 @@ err:
return NOTIFY_OK;
}
-#ifdef CONFIG_CRYPTO_MANAGER_TESTS
static int cryptomgr_test(void *data)
{
struct crypto_test_param *param = data;
u32 type = param->type;
int err = 0;
+#ifdef CONFIG_CRYPTO_MANAGER_DISABLE_TESTS
+ goto skiptest;
+#endif
+
if (type & CRYPTO_ALG_TESTED)
goto skiptest;
@@ -267,7 +270,6 @@ err_put_module:
err:
return NOTIFY_OK;
}
-#endif /* CONFIG_CRYPTO_MANAGER_TESTS */
static int cryptomgr_notify(struct notifier_block *this, unsigned long msg,
void *data)
@@ -275,10 +277,8 @@ static int cryptomgr_notify(struct notifier_block *this,
unsigned long msg,
switch (msg) {
case CRYPTO_MSG_ALG_REQUEST:
return cryptomgr_schedule_probe(data);
-#ifdef CONFIG_CRYPTO_MANAGER_TESTS
case CRYPTO_MSG_ALG_REGISTER:
return cryptomgr_schedule_test(data);
-#endif
}
return NOTIFY_DONE;
diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index abd980c..fa8c8f7 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -23,7 +23,7 @@
#include "internal.h"
-#ifndef CONFIG_CRYPTO_MANAGER_TESTS
+#ifdef CONFIG_CRYPTO_MANAGER_DISABLE_TESTS
/* a perfect nop */
int alg_test(const char *driver, const char *alg, u32 type, u32 mask)
@@ -2542,6 +2542,6 @@ non_fips_alg:
return -EINVAL;
}
-#endif /* CONFIG_CRYPTO_MANAGER_TESTS */
+#endif /* CONFIG_CRYPTO_MANAGER_DISABLE_TESTS */
EXPORT_SYMBOL_GPL(alg_test);
Thanks,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Initcall ordering problem (TTY vs modprobe vs MD5) and cryptomgr problem
On Fri, Aug 06, 2010 at 02:01:03AM +0100, David Howells wrote: > > but it's not being found by the crypto routines. By GIT bisection, I note > that this problem is introduced in the following commit: > > commit 0b767f96164b2b27488e3daa722ff16e89d49314 > Author: Alexander Shishkin > Date: Thu Jun 3 20:53:43 2010 +1000 > > crypto: testmgr - add an option to disable cryptoalgos' self-tests Indeed this changeset is buggy. It makes cryptomgr return a value as if it was not loaded. This triggers the module load. I'll send you a patch. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
