Re: [lkp-robot] [x86/kconfig] 81d3871900: BUG:unable_to_handle_kernel

[Linus Torvalds]

On Fri, Oct 13, 2017 at 6:56 AM, Andrey Ryabinin
 wrote:
>
> This could be fixed by s/vmovdqa/vmovdqu change like bellow, but maybe the 
> right fix
> would be to align the data properly?

I suspect anything that has the SHA extensions should also do
unaligned loads efficiently. The whole "aligned only" model is broken.
It's just doing two loads from the state pointer, there's likely no
point in trying to align it.

So your patch looks fine, but maybe somebody could add the required
alignment to the sha256 context allocation (which I don't know where
it is).

But yeah, that other SLOB panic looks unrelated to this.

   Linus

Re: [PATCH] crypto: cavium/nitrox - Remove default m setting from Kconfig

[Linus Torvalds]

Heh, I already did this in commit b4b8cbf679c4.

I complained because I want maintainers to be aware of this issue -
adding Kconfig options with defaults that don't make sense should be
caught earlier than when I do a test built..

  Linus

On Mon, Jul 10, 2017 at 3:15 AM, Herbert Xu <herb...@gondor.apana.org.au> wrote:
> Drivers should not enable themselves by default, unless they're
> an integral part of the platform.
>
> Reported-by: Linus Torvalds <torva...@linux-foundation.org>
> Signed-off-by: Herbert Xu <herb...@gondor.apana.org.au>
>
> diff --git a/drivers/crypto/cavium/nitrox/Kconfig 
> b/drivers/crypto/cavium/nitrox/Kconfig
> index 731e6a5..181a1df 100644
> --- a/drivers/crypto/cavium/nitrox/Kconfig
> +++ b/drivers/crypto/cavium/nitrox/Kconfig
> @@ -12,7 +12,6 @@ config CRYPTO_DEV_NITROX_CNN55XX
> tristate "Support for Cavium CNN55XX driver"
> depends on PCI_MSI && 64BIT
> select CRYPTO_DEV_NITROX
> -   default m
> help
>   Support for Cavium NITROX family CNN55XX driver
>   for accelerating crypto workloads.
> --
> Email: Herbert Xu <herb...@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Crypto Update for 4.13

[Linus Torvalds]

On Wed, Jul 5, 2017 at 6:01 AM, Herbert Xu  wrote:
>
> Drivers:
>
> - Add support for CNN55XX adapters in cavium.

Grr. I noticed this too late to fix it in the merge.

That stupid CNN55XX driver was added with a default of "m"?

WTF? Hell no. We don't add random new drivers and default them on -
and we do so even less when they are for very unusual hardware.

  Linus

Re: [PATCH] Revert "hwrng: core - zeroize buffers with random data"

[Linus Torvalds]

Stephan, Herbert? The zeroes in /dev/hwrng output are obviously
complete crap, so there's something badly wrong somewhere.

The locking, for example, is completely buggered. There's even a
comment about it, but that comment makes the correct observation of
"but y'know: randomness". But the memset() also being outside the lock
makes a complete joke of the whole thing.

Is the hwrng thing even worth maintaining? Compared to something like
/dev/urandom, it clearly does not do a very good job.

So I'm inclined to take the revert, but I'm also somewhat inclined to
simply mark this crud broken when we have other things that clearly do
a lot better.

  Linus

On Tue, Feb 7, 2017 at 4:23 PM, David Daney  wrote:
> This reverts commit 2cc751545854d7bd7eedf4d7e377bb52e176cd07.

Re: x86-64: Maintain 16-byte stack alignment

[Linus Torvalds]

On Thu, Jan 12, 2017 at 12:55 PM, Josh Poimboeuf  wrote:
>
> - Who's going to run sparse all the time to catch unauthorized users of
>   __aligned__(16)?

Well, considering that we apparently only have a small handful of
existing users without anybody having ever run any tool at all, I
don't think this is necessarily a huge problem.

One of the build servers could easily add the "make C=2" case to a
build test, and just grep the error reports for the 'excessive
alignment' string. The zero-day build bot already does much fancier
things.

So I don't think it would necessarily be all that hard to get a clean
build, and just say "if you need aligned stack space, you have to do
it yourself by hand".

That saId, if we now always enable frame pointers on x86 (and it has
gotten more and more difficult to avoid it), then the 16-byte
alignment would fairly natural.

The 8-byte alignment mainly makes sense when the basic call sequence
just adds 8 bytes, and you have functions without frames (that still
call other functions).

   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: x86-64: Maintain 16-byte stack alignment

[Linus Torvalds]

On Thu, Jan 12, 2017 at 6:02 AM, Josh Poimboeuf  wrote:
>
> Just to clarify, I think you're asking if, for versions of gcc which
> don't support -mpreferred-stack-boundary=3, objtool can analyze all C
> functions to ensure their stacks are 16-byte aligned.
>
> It's certainly possible, but I don't see how that solves the problem.
> The stack will still be misaligned by entry code.  Or am I missing
> something?

I think the argument is that we *could* try to align things, if we
just had some tool that actually then verified that we aren't missing
anything.

I'm not entirely happy with checking the generated code, though,
because as Ingo says, you have a 50:50 chance of just getting it right
by mistake. So I'd much rather have some static tool that checks
things at a code level (ie coccinelle or sparse).

Almost totally untested "sparse" patch appended. The problem with
sparse, obviously, is that few enough people run it, and it gives a
lot of other warnings. But maybe Herbert can test whether this would
actually have caught his situation, doing something like an
allmodconfig build with "C=2" to force a sparse run on everything, and
redirecting the warnings to stderr.

But this patch does seem to give a warning for the patch that Herbert
had, and that caused problems.

And in fact it seems to find a few other possible problems (most, but
not all, in crypto). This run was with the broken chacha20 patch
applied, to verify that I get a warning for that case:

   arch/x86/crypto/chacha20_glue.c:70:13: warning: symbol 'state' has
excessive alignment (16)
   arch/x86/crypto/aesni-intel_glue.c:724:12: warning: symbol 'iv' has
excessive alignment (16)
   arch/x86/crypto/aesni-intel_glue.c:803:12: warning: symbol 'iv' has
excessive alignment (16)
   crypto/shash.c:82:12: warning: symbol 'ubuf' has excessive alignment (16)
   crypto/shash.c:118:12: warning: symbol 'ubuf' has excessive alignment (16)
   drivers/char/hw_random/via-rng.c:89:14: warning: symbol 'buf' has
excessive alignment (16)
   net/bridge/netfilter/ebtables.c:1809:31: warning: symbol 'tinfo'
has excessive alignment (64)
   drivers/crypto/padlock-sha.c:85:14: warning: symbol 'buf' has
excessive alignment (16)
   drivers/crypto/padlock-sha.c:147:14: warning: symbol 'buf' has
excessive alignment (16)
   drivers/crypto/padlock-sha.c:304:12: warning: symbol 'buf' has
excessive alignment (16)
   drivers/crypto/padlock-sha.c:388:12: warning: symbol 'buf' has
excessive alignment (16)
   net/openvswitch/actions.c:797:33: warning: symbol 'ovs_rt' has
excessive alignment (64)
   drivers/net/ethernet/neterion/vxge/vxge-config.c:1006:38: warning:
symbol 'vpath' has excessive alignment (64)

although I think at least some of these happen to be ok.

There are a few places that clearly don't care about exact alignment,
and use "__attribute__((aligned))" without any specific alignment
value.

It's just sparse that thinks that implies 16-byte alignment (it
doesn't, really - it's unspecified, and is telling gcc to use "maximum
useful alignment", so who knows _what_ gcc will assume).

But some of them may well be real issues - if the alignment is about
correctness rather than anything else.

Anyway, the advantage of this kind of source-level check is that it
should really catch things regardless of "luck" wrt alignment.

Linus
 flow.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/flow.c b/flow.c
index 7db9548..c876869 100644
--- a/flow.c
+++ b/flow.c
@@ -601,6 +601,20 @@ static void simplify_one_symbol(struct entrypoint *ep, 
struct symbol *sym)
unsigned long mod;
int all, stores, complex;
 
+   /*
+* Warn about excessive local variable alignment.
+*
+* This needs to be linked up with some flag to enable
+* it, and specify the alignment. The 'max_int_alignment'
+* just happens to be what we want for the kernel for x86-64.
+*/
+   mod = sym->ctype.modifiers;
+   if (!(mod & (MOD_NONLOCAL | MOD_STATIC))) {
+   unsigned int alignment = sym->ctype.alignment;
+   if (alignment > max_int_alignment)
+   warning(sym->pos, "symbol '%s' has excessive alignment 
(%u)", show_ident(sym->ident), alignment);
+   }
+
/* Never used as a symbol? */
pseudo = sym->pseudo;
if (!pseudo)

Re: x86-64: Maintain 16-byte stack alignment

[Linus Torvalds]

On Tue, Jan 10, 2017 at 7:30 PM, Linus Torvalds
<torva...@linux-foundation.org> wrote:
>
> If you really want more stack alignment, you have to generate that
> alignment yourself by hand (and have a bigger buffer that you do that
> alignment inside).

Side note: gcc can (and does) actually generate forced alignment using
"and" instructions on %rsp rather than assuming pre-existing
alignment.  And that would be valid.

The problem with "alignof(16)" is not that gcc couldn't generate the
alignment itself, it's just the broken "it's already aligned to 16
bytes" assumption because -mpreferred-stack-boundary=3 doesn't work.

You *could* try to hack around it by forcing a 32-byte alignment
instead. That (I think) will make gcc generate the "and" instruction
mess.

And it shouldn't actually use any more memory than doing it by hand
(by having twice the alignment and hand-aligning the pointer).

So we *could* try to just have a really hacky rule saying that you can
align stack data to 8 or 32 bytes, but *not* to 16 bytes.

That said, I do think that the "don't assume stack alignment, do it by
hand" may be the safer thing. Because who knows what the random rules
will be on other architectures.

   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: x86-64: Maintain 16-byte stack alignment

[Linus Torvalds]

On Tue, Jan 10, 2017 at 7:11 PM, Herbert Xu  wrote:
>
> Well the only other alternative I see is to ban compilers which
> enforce 16-byte stack alignment, such as gcc 4.7.2.

No,  you don't have to ban the compiler - it's just a "generate overly
stupid code that just uses extra instructions to likely mis-align the
stack more" issue. So it's "stupid code generation" vs "buggy".

What we should ban is code that assumes that stack objects can be
aligned to more than word boundary.

__attribute__((align)) simply doesn't work on stack objects, because
the stack isn't aligned.

If you really want more stack alignment, you have to generate that
alignment yourself by hand (and have a bigger buffer that you do that
alignment inside).

So this was just simply buggy:

  u32 state[16] __aligned(CHACHA20_STATE_ALIGN);

because you just can't do that. It's that simple. There is a reason
why the code does the dance with

u32 *state, state_buf[16 + (CHACHA20_STATE_ALIGN / sizeof(u32)) - 1];

state = (u32 *)roundup((uintptr_t)state_buf, CHACHA20_STATE_ALIGN);

rather than ask the compiler to do something invalid.

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: x86-64: Maintain 16-byte stack alignment

[Linus Torvalds]

On Tue, Jan 10, 2017 at 6:39 AM, Herbert Xu  wrote:
>
> BTW this is with Debian gcc 4.7.2 which does not allow an 8-byte
> stack alignment as attempted by the Makefile:

I'm pretty sure we have random asm code that may not maintain a
16-byte stack alignment when it calls other code (including, in some
cases, calling C code).

So I'm not at all convinced that this is a good idea. We shouldn't
expect 16-byte alignment to be something trustworthy.

 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: HalfSipHash Acceptable Usage

[Linus Torvalds]

On Wed, Dec 21, 2016 at 7:55 AM, George Spelvin
 wrote:
>
> How much does kernel_fpu_begin()/kernel_fpu_end() cost?

It's now better than it used to be, but it's absolutely disastrous
still. We're talking easily many hundreds of cycles. Under some loads,
thousands.

And I warn you already: it will _benchmark_ a hell of a lot better
than it will work in reality. In benchmarks, you'll hit all the
optimizations ("oh, I've already saved away all the FP registers, no
need to do it again").

In contrast, in reality, especially with things like "do it once or
twice per incoming packet", you'll easily hit the absolute worst
cases, where not only does it take a few hundred cycles to save the FP
state, you'll then return to user space in between packets, which
triggers the slow-path return code and reloads the FP state, which is
another few hundred cycles plus.

Similarly, in benchmarks you'll hit the "modern CPU's power on the AVX
unit and keep it powered up for a while afterwards", while in real
life you would quite easily hit the "oh, AVX is powered down because
we were idle, now it powers up at half speed which is another latency
hit _and_ the AVX unit won't run full out anyway".

Don't do it. There are basically no real situations where the AVX
state optimizations help for the kernel. We just don't have the loop
counts to make up for the problems it causes.

The one exception is likely if you're doing things like
high-throughput disk IO encryption, and then you'd be much better off
using SHA256 instead (which often has hw encryption on modern CPU's -
both x86 and ARM).

(I'm sure that you could see it on some high-throughput network
benchmark too when the benchmark entirely saturates the CPU. And then
in real life it would suck horribly for all the reasons above).

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: [kernel-hardening] Re: [PATCH v2 1/4] siphash: add cryptographically secure hashtable function

[Linus Torvalds]

On Thu, Dec 15, 2016 at 1:11 PM, Jason A. Donenfeld  wrote:
>
> Indeed, I stand corrected. But in any case, the use of __aligned(8) in
> the patchset ensures that things are fixed and that we don't have this
> issue.

I think you can/should just use the natural alignment for "u64".

For architectures that need 8-byte alignment, u64 will already be
properly aligned. For architectures (like x86-32) that only need
4-byte alignment, you get 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: [PATCH v3 1/3] siphash: add cryptographically secure hashtable function

[Linus Torvalds]

On Wed, Dec 14, 2016 at 3:34 PM, Jason A. Donenfeld  wrote:
>
> Or does your reasonable dislike of "word" still allow for the use of
> dword and qword, so that the current function names of:

dword really is confusing to people.

If you have a MIPS background, it means 64 bits. While to people with
Windows programming backgrounds it means 32 bits.

Please try to avoid using it.

As mentioned, I think almost everybody agrees on the "q" part being 64
bits, but that may just be me not having seen it in any other context.

And before anybody points it out - yes, we already have lots of uses
of "dword" in various places. But they tend to be mostly
hardware-specific - either architectures or drivers.

So I'd _prefer_ to try to keep "word" and "dword" away from generic
helper routines. But it's not like anything is really black and white.

   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: [PATCH v3 1/3] siphash: add cryptographically secure hashtable function

[Linus Torvalds]

On Wed, Dec 14, 2016 at 2:56 PM, Jason A. Donenfeld  wrote:
>
> So actually jhash_Nwords makes no sense, since it takes dwords
> (32-bits) not words (16-bits). The siphash analog should be called
> siphash24_Nqwords.

No. The bug is talking about "words" in the first place.

Depending on your background, a "word" can be generally be either 16
bits or 32 bits (or, in some cases, 18 bits).

In theory, a 64-bit entity can be a "word" too, but pretty much nobody
uses that. Even architectures that started out with a 64-bit register
size and never had any smaller historical baggage (eg alpha) tend to
call 32-bit entities "words".

So 16 bits can be a word, but some people/architectures will call it a
"half-word".

To make matters even more confusing, a "quadword" is generally always
64 bits, regardless of the size of "word".

So please try to avoid the use of "word" entirely. It's too ambiguous,
and it's not even helpful as a "size of the native register". It's
almost purely random.

For the kernel, we tend use

 - uX for types that have specific sizes (X being the number of bits)

 - "[unsigned] long" for native register size

But never "word".

   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: [PATCH v3] siphash: add cryptographically secure hashtable function

[Linus Torvalds]

On Tue, Dec 13, 2016 at 12:39 AM, Eric Biggers  wrote:
>
> Hmm, I don't think you can really do load_unaligned_zeropad() without first
> checking for 'left != 0'.

Right you are. If the allocation is at the end of a page, the 0-size
case would be entirely outside the page and there's no fixup.

Of course, that never happens in normal code, but DEBUG_PAGE_ALLOC can
trigger 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: [PATCH v3] siphash: add cryptographically secure hashtable function

[Linus Torvalds]

On Mon, Dec 12, 2016 at 3:04 PM, Jason A. Donenfeld  wrote:
>
> Indeed this would be a great first candidate. There are lots of places
> where MD5 (!!) is pulled in for this sort of thing, when SipHash could
> be a faster and leaner replacement (and arguably more secure than
> rusty MD5).

Yeah,. the TCP sequence number md5_transform() cases are likely the
best example of something where siphash might be good. That tends to
be really just a couple words of data (the address and port info) plus
the net_secret[] hash. I think they currently simply just fill in the
fixed-sized 64-byte md5-round area.

I wonder it's worth it to have a special spihash version that does
that same "fixed 64-byte area" thing.

But please talk to the netwotrking people. Maybe that's the proper way
to get this merged?

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: [PATCH v2] siphash: add cryptographically secure hashtable function

[Linus Torvalds]

On Sun, Dec 11, 2016 at 9:48 PM, Jason A. Donenfeld  wrote:
> I modified the test to hash data of size 0 through 7 repeatedly
> 1 times, and benchmarked that a few times on a Skylake laptop.
> The `load_unaligned_zeropad & bytemask_from_count` version was
> consistently 7% slower.
>
> I then modified it again to simply hash a 4 byte constant repeatedly
> 10 times. The `load_unaligned_zeropad & bytemask_from_count`
> version was around 6% faster. I tried again with a 7 byte constant and
> got more or less a similar result.
>
> Then I tried with a 1 byte constant, and found that the
> `load_unaligned_zeropad & bytemask_from_count` version was slower.
>
> So, it would seem that between the `if (left)` and the `switch
> (left)`, there's the same number of branches.

Interesting.

For the dcache code (which is where that trick comes from), we used to
have a loop (rather than the duff's device thing), and it performed
badly due to the consistently badly predicted branch of the loop. But
I never compared it against the duff's device version.

I guess you could try to just remove the "if (left)" test entirely, if
it is at least partly the mispredict. It should do the right thing
even with a zero count, and it might schedule the code better. Code
size _should_ be better with the byte mask model (which won't matter
in the hot loop example, since it will all be cached, possibly even in
the uop cache for really tight benchmark loops).

 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: [PATCH v2] siphash: add cryptographically secure hashtable function

[Linus Torvalds]

On Sun, Dec 11, 2016 at 7:48 PM, Jason A. Donenfeld  wrote:
> +   switch (left) {
> +   case 7: b |= ((u64)data[6]) << 48;
> +   case 6: b |= ((u64)data[5]) << 40;
> +   case 5: b |= ((u64)data[4]) << 32;
> +   case 4: b |= ((u64)data[3]) << 24;
> +   case 3: b |= ((u64)data[2]) << 16;
> +   case 2: b |= ((u64)data[1]) <<  8;
> +   case 1: b |= ((u64)data[0]); break;
> +   case 0: break;
> +   }

The above is extremely inefficient. Considering that most kernel data
would be expected to be smallish, that matters (ie the usual benchmark
would not be about hashing megabytes of data, but instead millions of
hashes of small data).

I think this could be rewritten (at least for 64-bit architectures) as

#ifdef CONFIG_DCACHE_WORD_ACCESS

if (left)
b |= le64_to_cpu(load_unaligned_zeropad(data) &
bytemask_from_count(left));

#else

.. do the duff's device thing with the switch() ..

#endif

which should give you basically perfect code generation (ie a single
64-bit load and a byte mask).

Totally untested, just looking at the code and trying to make sense of it.

... and obviously, it requires an actual high-performance use-case to
make any difference.

  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: [PATCH v2 00/11] getting back -Wmaybe-uninitialized

[Linus Torvalds]

On Thu, Nov 10, 2016 at 8:44 AM, Arnd Bergmann  wrote:
>
> Please merge these directly if you are happy with the result.

I will take this.

I do see two warnings, but they both seem to be valid and recent,
though, so I have no issues with the spurious cases.

Warning #1:

  sound/soc/qcom/lpass-platform.c: In function ‘lpass_platform_pcmops_open’:
  sound/soc/qcom/lpass-platform.c:83:29: warning: ‘dma_ch’ may be used
uninitialized in this function [-Wmaybe-uninitialized]
drvdata->substream[dma_ch] = substream;
~~~^~~

and 'dma_ch' usage there really is crazy and wrong. Broken by
022d00ee0b55 ("ASoC: lpass-platform: Fix broken pcm data usage")

Warning #2 is not a real bug, but it's reasonable that gcc doesn't
know that storage_bytes (chip->read_size) has to be 2/4. Again,
introduced recently by commit 231147ee77f3 ("iio: maxim_thermocouple:
Align 16 bit big endian value of raw reads"), so you didn't see it.

  drivers/iio/temperature/maxim_thermocouple.c: In function
‘maxim_thermocouple_read_raw’:
  drivers/iio/temperature/maxim_thermocouple.c:141:5: warning: ‘ret’
may be used uninitialized in this function [-Wmaybe-uninitialized]
if (ret)
   ^
  drivers/iio/temperature/maxim_thermocouple.c:128:6: note: ‘ret’ was
declared here
int ret;
^~~

and I guess that code can just initialize 'ret' to '-EINVAL' or
something to just make the theoretical "somehow we had a wrong
chip->read_size" case error out cleanly.

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: [GIT PULL] /dev/random driver changes for 4.8

[Linus Torvalds]

On Wed, Jul 27, 2016 at 6:12 AM, Theodore Ts'o  wrote:
>
> Are you planning on pulling the random tree this cycle?  I'm not sure
> if you wanted to let it soak for a few days in linux-next, or whether
> you want to wait another full release cycle

It's next in line in my queue, so unless it blows up spectacularly in
my face I'm pulling it this cycle. It's not liek the pull request
looks particularly scary,

  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: [patch] crypto: sha256-mb - cleanup a || vs | typo

[Linus Torvalds]

[ rare comment rant. I think I'll do this once, and then ignore the discussion ]

On Fri, Jul 8, 2016 at 9:45 AM, Herbert Xu  wrote:
>
> Nack.  As I said the commenting style in the crypto API is the
> same as the network stack.  So unless we decide to change both
> please stick to the current style.

Can we please get rid of the brain-damaged stupid networking comment
syntax style, PLEASE?

If the networking people cannot handle the pure awesomeness that is a
balanced and symmetric traditional multi-line C style comments, then
instead of the disgusting unbalanced crap that you guys use now,
please just go all the way to the C++ mode.

In other words, these three models are good:

 (a)
  /* This is a comment *./

 (b)
  /*
   * This is also a comment, but it can now be cleanly
   * split over multiple lines
   */

 (c)
  // This can be a single line. Or many. Your choice.

and they are all obviously visually balanced. Sometimes you want (b)
even for a single line, if you want the white-space to make it stand
out more, but you can obviously do that with (c) too, by just
surrounding it with two empty (comment) lines.

The (c) form is particularly good for things like enum or structure
member comments at the end of code, where you might want to align
things up, but the ending comment marker ends up being visually pretty
distracting (and lining _that_ up is too much make-believe work).

There's also another acceptablr traditional multi-line style that
you'll find in some places, but it's not the common kernel style:

 (d)
  /* This is an alternate multi-line format
 that isn't horrible, but not kernel style */

Note how all the above comment styles have a certain visual symmatry
and balance.

But no, the networking code picked *none* of the above sane formats.
Instead, it picked these two models that are just half-arsed
shit-for-brains:

 (no)
 /* This is disgusting drug-induced
   * crap, and should die
   */

  (no-no-no)
  /* This is also very nasty
   * and visually unbalanced */

Please. The networking code actually has the *worst* possible comment
style. You can literally find that (no-no-no) style, which is just
really horribly disgusting and worse than the otherwise fairly similar
(d) in pretty much every way.

I'm not even going to start talking about the people who prefer to
"box in" their comments, and line up both ends and have fancy boxes of
stars around the whole thing. I'm sure that looks really nice if you
are out of your mind on LSD, and have nothing better to do than to
worry about the right alignment of the asterisks.

I'd be happy to start moving the whole kernel over to the C++ style,
it's been many many years since we had compatibility issues and we are
all used to it by now, even if we weren't all fans originally.

I really don't understand why the networking people think that their
particularly ugly styles are fine. They are the most visually
unbalanced version of _all_ the common comment styles, and have no
actual advantages.

So just get rid of the (no-no) and (no-no-no) forms. Not in one big
go, but as people touch the code, just fix that mess up.

 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: linux/bitops.h

[Linus Torvalds]

On Wed, May 4, 2016 at 5:30 PM, H. Peter Anvin  wrote:
>
> Yes. d7e35dfa is baloney IMNSHO.  All it does is produce worse code, and the
> description even says so.
>
> As I said, gcc has treated the former code as idiomatic since gcc 2, so that
> support is beyond ancient.

Well, we're *trying* to get clang supported, so the argument that gcc
has always supported it and compiles correct code for it is not
necessarily the whole story yet.

The problem with "32 - shift" is that it really could generate garbage
in situations where shift ends up being a constant zero..

That said, the fact that the other cases weren't changed
(rol64/ror64/ror32) does make that argument less interesting. Unless
there was some particular code that actively ended up using
"rol32(..0)" but not the other cases.

(And yes, rol32(x,0) is obviously nonsense, but it could easily happen
with inline functions or macros that end up generating that).

Note that there may be 8 "rol/ror" functions, but the 16-bit and 8-bit
ones don't have the undefined semantics. So there are only four that
had _that_ problem, although I agree that changing just one out of
four is wrong.

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: Crypto Fixes for 4.3

[Linus Torvalds]

On Tue, Oct 13, 2015 at 5:17 AM, Herbert Xu  wrote:
>
> This push fixes the following issues:
>
> * Fix AVX detection to prevent use of non-existent AESNI.
> * Some SPARC ciphers did not set their IV size which may lead
>   to memory corruption.

Hmm. It looks like you also quietly added a ahash fix this morning.

I took it despite it not being described or in the diffstat. But
please send new pull requests when you update a branch you asked me to
pull.

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: Crypto Fixes for 4.3

[Linus Torvalds]

On Tue, Oct 13, 2015 at 6:03 PM, Herbert Xu  wrote:
>
> Oops, I should've waited for you to pull the previous one before
> pushing this one out.

You might try to start using signed tags for your pull requests. That
lessens this kind of issue, because now only will you write the tag
message and then your signing key pass phrase etc, the tag would
specify one very particular commit at the time of the tagging rather
than just have it be the default branch of your public repository.

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: Crypto Fixes for 4.2

[Linus Torvalds]

On Fri, Jun 26, 2015 at 11:56 PM, Herbert Xu
herb...@gondor.apana.org.au wrote:

 So I think Tadeusz's patch is the simplest fix for 4.2.  Could you
 please test it to see if it makes your warning go away?

Seems to silence it here.

I get the feeling that the patch is still wrong - why are not the
*tests* run at late time when everything is properly set up, rather
than forcing ordering at the code init level - but at least I don't
see the annoying error, so it's certainly better than it was before.

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: Crypto Fixes for 4.2

[Linus Torvalds]

On Fri, Jun 26, 2015 at 3:22 AM, Herbert Xu herb...@gondor.apana.org.au wrote:

 * Kill testmgr warning for gcm-aes-aesni.

Hmm. You killed one of the warnings, but the setkey one remains.

alg: aead: setkey failed on test 1 for rfc4106-gcm-aesni: flags=0

Expected?

   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: [PATCH] crypto: aesni - fix failing setkey for rfc4106-gcm-aesni

[Linus Torvalds]

On Thu, Jun 25, 2015 at 7:25 AM, Herbert Xu herb...@gondor.apana.org.au wrote:

 Linus, could you confirm that you have AESNI built into the kernel
 and not as a module?

Yes, confirmed.

 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: Crypto Update for 4.2

[Linus Torvalds]

On Mon, Jun 22, 2015 at 1:44 AM, Herbert Xu herb...@gondor.apana.org.au wrote:

 Here is the crypto update for 4.2:

Hmm. I noticed a new annoyance:

I get this at bootup:

  [  +0.001504] alg: No test for __gcm-aes-aesni (__driver-gcm-aes-aesni)
  [  +0.002233] alg: aead: setkey failed on test 1 for
rfc4106-gcm-aesni: flags=0

in general, I'm not at all convinced that the crypto tests make sense.
I absolutely destest that horrid testmgr.h file that is 32
_thousand_ lines of noise. And now it's apparently complaining about a
missing test, so that nasty mess will presumably grow.

Could you not make the test infrastructure be something that gets run
in user space?

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: Crypto Update for 4.2

[Linus Torvalds]

On Mon, Jun 22, 2015 at 1:44 AM, Herbert Xu herb...@gondor.apana.org.au wrote:

 Here is the crypto update for 4.2:

So this generates conflicts with your earlier changes (that I got
through the networking tree - they are your patches, but they went
through Steffen Klassert and then David Miller).

I resolved them, but I want you to double-check the end result.

Some of the conflicts are just trivial (but annoying) conflicts due to
whitespace changes to the vmx routines.

But the changes to net/ipv4/esp4.c and net/ipv6/esp6.c are actual real
code conflicts, even though the in the merge they look like no change
at all, because I picked your side and the changes on the other side
just went away.

I did that, because ss far as I can tell, the changes in commits
7021b2e1cddd and 000ae7b2690e (that switch esp4/6 over to the new AEAD
interface) obviate the commits I got earlier to use the high-order
sequence number bits for IV generation.

So it looks to me like those AEAD interface changes already make sure
to use the full 64 bits of the sequence number.

But if I'm wrong, please holler. You clearly know both sides of this,
since you wrote all the patches involved, so I'd like you to
double-check me.

  Linus
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in

Re: Crypto Fixes for 4.1

[Linus Torvalds]

On Thu, May 21, 2015 at 9:05 PM, Herbert Xu herb...@gondor.apana.org.au wrote:

 Please pull from

 git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git

 or

 master.kernel.org:/pub/scm/linux/kernel/git/herbert/crypto-2.6.git

Mind fixing your script to not have that old master.kernel.org' thing
that no longer works and hasn't worked in a long time? I thought I
asked you earlier, but it turns out that was Dmitry and the input tree
who had the same old script...

   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: Crypto Update for 4.1

[Linus Torvalds]

Oh, and I forgot to add Stephan to the email recipients list..

Sorry for the duplicate email,

  Linus

On Wed, Apr 15, 2015 at 7:37 PM, Linus Torvalds
torva...@linux-foundation.org wrote:
 On Wed, Apr 15, 2015 at 6:58 PM, Linus Torvalds
 torva...@linux-foundation.org wrote:
 On Tue, Apr 14, 2015 at 8:39 PM, Herbert Xu herb...@gondor.apana.org.au 
 wrote:

 Here is the crypto update for 4.1:

 Just a heads-up: this breaks iwlwifi for me after suspend.

 Ok, bisect completed:

 [torvalds@vaio linux]$ git bisect bad
 9c521a200bc3c12bd724e48a75c57d5358f672be is the first bad commit
 commit 9c521a200bc3c12bd724e48a75c57d5358f672be
 Author: Stephan Mueller smuel...@chronox.de
 Date:   Thu Apr 9 12:09:55 2015 +0200

 crypto: api - remove instance when test failed
 ...

 and while I have no idea *why* it breaks iwlwifi after a
 suspend/resume cycle, it is 100% repeatable. The bisect zoomed right
 to that commit, and reverting it on top of the current tree also makes
 everything work again.

 So it gets reverted. I'll be happy to test things out, but with the
 merge window *and* travel, I may or may not be quick about 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: Crypto Update for 4.1

[Linus Torvalds]

On Tue, Apr 14, 2015 at 8:39 PM, Herbert Xu herb...@gondor.apana.org.au wrote:

 Here is the crypto update for 4.1:

Just a heads-up: this breaks iwlwifi for me after suspend.

I'm bisecting right now. But because this laptop is what I expect to
travel with tomorrow, I will ruthlessly revert anything I find,
because I need it to work.

 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: Crypto Update for 4.1

[Linus Torvalds]

On Wed, Apr 15, 2015 at 8:07 PM, Herbert Xu herb...@gondor.apana.org.au wrote:

 Thanks! It actually appears to be a very simple bug that I somehow
 missed during reviewing.

Ok, this patch seems to fix it for me, so I undid my revert that I
hadn't pushed out yet, and pushed out this instead.

Thanks,

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: Crypto Fixes for 4.0

[Linus Torvalds]

On Tue, Mar 17, 2015 at 10:25 PM, Herbert Xu
herb...@gondor.apana.org.au wrote:
 Hi Linus:

 On Mon, Mar 09, 2015 at 04:19:50PM +1100, Herbert Xu wrote:

 This push fixes a bug in the ARM XTS implementation that can
 cause failures to in decrypting encrypted disks.

 For some reason this didn't get pulled so I'm resending it with
 another fix.

Hmm. I never got your original email. Maybe it was in my spam folder
and my spam scan never noticed..

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: [PATCH v3 11/12] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c

[Linus Torvalds]

On Mon, Sep 15, 2014 at 12:30 AM,  beh...@converseincode.com wrote:
 From: Behan Webster beh...@converseincode.com

 Replaced the use of a Variable Length Array In Struct (VLAIS) with a C99
 compliant equivalent. This patch allocates the appropriate amount of memory
 using a char array using the SHASH_DESC_ON_STACK macro.

You only made the first case use SHASH_DESC_ON_STACK, the two other
cases you left in the ugly format. Was that just an oversight, or was
there some reason for 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: Crypto Update for 3.16

[Linus Torvalds]

On Wed, Jun 4, 2014 at 11:23 PM, Herbert Xu herb...@gondor.apana.org.au wrote:

 Here is the crypto update for 3.16:

There's something odd going on with bfin_crc.h.

You moved it in commit 52e6e543f2d8 (crypto: bfin_crc - access crc
registers by readl and writel functions).

It got *deleted* by commit 3356c99ea392 (bfin_crc: Move architecture
independant crc header file out of the blackfin folder) which claims
to just move things.

Both of those commits are by Sonic Zhang, just came to me through two
different trees (though your crypto tree, and through Steven Miao's
bfin tree).

I'm assuming that the delete was actually incorrect, and should have
been a move, because it looks like the bfin_crc.c file won't compile
without it. So I've re-instated that file.

But the state of the bfin tree seems to be crap. Somebody should take
a look at what happened here. My suspicion is that commit 3356c99ea392
was broken by Steven Miao trying to only touch files in arch/blackfin
or something.

   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: Crypto Fixes for 3.12

[Linus Torvalds]

On Fri, Sep 13, 2013 at 4:30 AM, Herbert Xu herb...@gondor.apana.org.au wrote:

 Herbert Xu (2):
   crypto: api - Fix race condition in larval lookup
   crypto: crct10dif - Add fallback for broken initrds

  crypto/Makefile |2 +-
  crypto/api.c|7 +-
  crypto/{crct10dif.c = crct10dif_common.c}  |  100 
 +--
  crypto/{crct10dif.c = crct10dif_generic.c} |   53 +-
  lib/crc-t10dif.c|   11 ++-
  5 files changed, 20 insertions(+), 153 deletions(-)

Please fix your script. You apparently have it using -C to find
copies, which can be very useful to see what is going on especially
with --summary (which you don't have), but is misleading when
sending diffstats when people don't expect it.

The pull request does not have 20 insertions, it has 146
insertions, and it's just that a fair chunk of them come from a file
being essentially duplicated. See the difference:

With copy detection (git diff -C --stat --summary)
 crypto/Makefile |   2 +-
 crypto/api.c|   7 +-
 crypto/{crct10dif.c = crct10dif_common.c}  | 100 +---
 crypto/{crct10dif.c = crct10dif_generic.c} |  53 +--
 lib/crc-t10dif.c|  11 ++-
 5 files changed, 20 insertions(+), 153 deletions(-)
 copy crypto/{crct10dif.c = crct10dif_common.c} (63%)
 rename crypto/{crct10dif.c = crct10dif_generic.c} (55%)

With just rename detection (git diff -M --stat --summary)
 crypto/Makefile|   2 +-
 crypto/api.c   |   7 +-
 crypto/{crct10dif.c = crct10dif_common.c} | 100 +---
 crypto/crct10dif_generic.c | 127 +
 lib/crc-t10dif.c   |  11 +-
 5 files changed, 146 insertions(+), 101 deletions(-)
 rename crypto/{crct10dif.c = crct10dif_common.c} (63%)
 create mode 100644 crypto/crct10dif_generic.c

and your pull request looked really misleading because it did -C but
didn't have that summary pointing out that one of them was a copy.

So please use -M --stat --summary. That's what git shows me when I
do a git pull, so that's what I'm going to compare with..

As mentioned -C _is_ useful, but it's useful when you're
specifically looking for that's a lot of new lines, is it copying old
files kind of things.

  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: Kernel 3.7.0-rc1 crash after TrueCrypt mount device on a computer with Intel i5

[Linus Torvalds]

Krzysztof, please try to cc the appropriate people/list.

I've added linux-crypto and the people who touched aesni-intel since
3.6, and am re-quoting the whole email (except for the continuation
oopses that won't be relevant)

It seems to crash on the very first instruction of _aesni_enc1, which is just a

   movaps (KEYP), KEY

where on x86-32, KEYP is %edi and KEY is %xmm2.

In the oops register dump, %edi is 0xf169fe64, which looks like a
valid kernel pointer (depending on amount of memory), but it looks
like the problem is that it's not 16-byte aligned.

I dunno. None of the asm code seems to have changed since 3.6 afaik,
so some calling code change triggers this? Guys, ideas?

   Linus

On Thu, Oct 18, 2012 at 8:53 AM, Krzysztof Kolasa kkol...@winsoft.pl wrote:
 after mount crypted device (volume, pendrive) kernel crash on HP machine
 (mounting on AMILO Pro v3405 working properly ), rs232 console output :

 [  124.613648] general protection fault:  [#1] SMP
 [  124.672862] Modules linked in: dm_crypt fglrx(PO) bnep rfcomm bluetooth
 binfmt_misc snd_hda_codec_hdmi snd_hda_codec_idt snd_hda_ine
 [  125.356439] Pid: 55, comm: kworker/0:1 Tainted: P   O
 3.7.0-rc1-winsoft-pae #1 Hewlett-Packard HP ProBook 6560b/1619
 [  125.490351] EIP: 0060:[f85bb2bc] EFLAGS: 00010216 CPU: 0
 [  125.555762] EIP is at _aesni_enc1+0x0/0x9c [aesni_intel]
 [  125.619087] EAX: c1959000 EBX: 0001 ECX: f001392c EDX: f06de000
 [  125.693829] ESI: f169fdb4 EDI: f169fe64 EBP: f169fda4 ESP: f169fd30
 [  125.768578]  DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
 [  125.832951] CR0: 80050033 CR2: b771 CR3: 01963000 CR4: 000407f0
 [  125.907694] DR0:  DR1:  DR2:  DR3: 
 [  125.982436] DR6: 0ff0 DR7: 0400
 [  126.028115] Process kworker/0:1 (pid: 55, ti=f169e000 task=f1679940
 task.ti=f169e000)
 [  126.121547] Stack:
 [  126.145429]  f85bb2b4 0008 f169fe34 f85703a9 f169fe64 c1959000
 f06de000 0008
 [  126.237894]  0200  f06de000 f23b3bc0 f06de000 f23b3bc0
 f06de000 f00138f4
 [  126.330358]   0200 f001390c  0200 
  f001392c
 [  126.422804] Call Trace:
 [  126.451881]  [f85bb2b4] ? aesni_enc+0x1c/0x24 [aesni_intel]
 [  126.520399]  [f85703a9] ? init_tfm+0x129/0x250 [xts]
 [  126.581652]  [f85bbd6e] xts_decrypt+0x7e/0xc0 [aesni_intel]
 [  126.650174]  [c15c5a95] ? notifier_call_chain+0x45/0x60
 [  126.714543]  [c107f61d] ? update_curr+0x20d/0x380
 [  126.772681]  [c1142f48] ? __kmalloc+0xd8/0x1f0
 [  126.827715]  [c11077f3] ? mempool_kmalloc+0x13/0x20
 [  126.887935]  [f85bb298] ? aesni_set_key+0x1d8/0x1d8 [aesni_intel]
 [  126.962681]  [f85bb960] ? __aes_encrypt+0x30/0x30 [aesni_intel]
 [  127.035353]  [f855e157] ablk_decrypt+0x47/0xb0 [ablk_helper]
 [  127.104912]  [f8ae1c9b] crypt_convert+0x26b/0x2d0 [dm_crypt]
 [  127.174468]  [f8ae23b0] kcryptd_crypt+0x280/0x360 [dm_crypt]
 [  127.244028]  [c10647a0] process_one_work+0x110/0x380
 [  127.305282]  [c15c97b3] ? common_interrupt+0x33/0x38
 [  127.366537]  [c10624a0] ? wake_up_worker+0x30/0x30
 [  127.425716]  [f8ae2130] ? crypt_convert_init.isra.15+0x70/0x70
 [dm_crypt]
 [  127.508768]  [c10651d9] worker_thread+0x119/0x350
 [  127.566910]  [c10650c0] ? manage_workers+0x260/0x260
 [  127.628164]  [c1069854] kthread+0x94/0xa0
 [  127.678001]  [c107] ? blocking_notifier_chain_unregister+0x50/0xa0
 [  127.757938]  [c15c91b7] ret_from_kernel_thread+0x1b/0x28
 [  127.823350]  [c10697c0] ? flush_kthread_worker+0x90/0x90
 [  127.888757] Code: 31 c0 5f c3 8d 76 00 57 53 8b 7c 24 0c 8b 44 24 10 8b
 54 24 14 8b 9f e0 01 00 00 0f 10 02 e8 08 00 00 00 0f 11 004
 [  128.113500] EIP: [f85bb2bc] _aesni_enc1+0x0/0x9c [aesni_intel] SS:ESP
 0068:f169fd30
 [  128.207699] ---[ end trace ff0828d34a0b516e ]---

 subsequent errors after the above kernel crash (one cpu 100% busy) :

 [  128.262830] BUG: scheduling while atomic: kworker/0:1/55/0x1001

.. snipped - not interesting, since a kworker dying will always result
in lots of noise.
--
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: Linux 3.6-rc5

[Linus Torvalds]

On Sun, Sep 9, 2012 at 5:54 AM, Jussi Kivilinna
jussi.kivili...@mbnet.fi wrote:

 Does reverting e46e9a46386bca8e80a6467b5c643dc494861896 help?

 That commit added crypto selftest for authenc(hmac(sha1),cbc(aes)) in 3.6,
 and probably made this bug visible (but not directly causing it).

So Romain said it does - where do we go from here? Revert testing it,
or fix the authenc() case? I'd prefer the fix..

 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: [PATCH 5/6] workqueue: introduce NR_WORKER_POOLS and for_each_worker_pool()

[Linus Torvalds]

Seeing code like this

+   return (*nr_running)[0];

just makes me go WTF?

Why are you taking the address of something you just dereferenced (the
 [0] part).

And you actually do that *twice*, except the inner one is more
complicated. When you assign nr_runing, you take the address of it, so
the *nr_running is actually just the same kind of odd thing (except
in reverse - you take dereference something you just took the
address-of).

Seriously, this to me is a sign of *deeply* confused code. And the
fact that your first version of that code was buggy *EXACTLY* due to
this confusion should have made you take a step back.

As far as I can tell, what you actually want that function to do is:

  static atomic_t *get_pool_nr_running(struct worker_pool *pool)
  {
int cpu = pool-gcwq-cpu;

if (cpu != WORK_CPU_UNBOUND)
return per_cpu(pool_nr_running, cpu);

return unbound_pool_nr_running;
  }

Notice how there isn't an 'address-of' operator anywhere in sight
there. Those things are arrays, they get turned into atomic_t *
automatically. And there isn't a single dereference (not a '*', and
not a [0] - they are the exact same thing, btw) in sight either.

What am I missing? Are there some new drugs that all the cool kids
chew that I should be trying? Because I really don't think the kinds
of insane take the address of a dereference games are a good idea.
They really look to me like somebody is having a really bad drug
experience.

I didn't test the code, btw. I just looked at the patch and went WTF.

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: [PATCH 5/6] workqueue: introduce NR_WORKER_POOLS and for_each_worker_pool()

[Linus Torvalds]

On Fri, Jul 13, 2012 at 9:44 PM, Tejun Heo t...@kernel.org wrote:

 nr_running is atomic_t (*nr_running)[2].  Ignoring the pointer to
 array part, it's just returning the address of N'th element of the
 array.  ARRAY + N == ARRAY[N].

None of this matters one whit.

You did (x)[0].

That's insane. It's crazy. It doesn't even matter what x is in
between, it's crazy regardless.

It's just a really confused way of saying x (*). Except it makes the
code look like an insane monkey on crack got a-hold of your keyboard
when you weren't looking.

And to make it worse, x itself was the result of doing *y. Which
was probably written by the insane monkey's older brother, Max, who
has been chewing Quaaludes for a few years, and as a result _his_
brain really isn't doing too well either. Even for a monkey. And now
you're letting *him* at your keyboard too?

So you had two separately (but similarly) insane ways of complicating
the code so that it was really obfuscated. When it really just
computed y to begin with, it just added all those x=*y and
(x)[0] games around it to make it look complicated.

Linus

(*) Technically, (x)[0] is actually a really confused way of saying
(x+0) while making sure that x was a valid pointer. It basically
guarantees that if x started out as an array, it has now been
demoted to a pointer - but since arrays will be demoted to pointers by
pretty much any subsequent operation except for sizeof() and a
couple of other special cases anyway, you can pretty much just say
that (x)[0] is (x+0) is x.

And *y really is exactly the same as y, except for again some
syntactic checking (ie it is basically an odd way to verify that y
is an lvalue, since you cannot do an address-of of a non-lvalue).
--
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: Crypto Update for 3.5

[Linus Torvalds]

On Tue, May 22, 2012 at 6:35 PM, Herbert Xu herb...@gondor.apana.org.au wrote:

 Here is the crypto update for 3.5:

I pulled this, but quite frankly, some of it looks like utter garbage.

There's a declaration for dbx500_add_platform_device_noirq() that does
not exist and is not used anywhere. Why? It was added in commit
585d188f8072, and I see no rhyme or reason to it.

I only noticed because I happened to get a conflict due to the
location it was added. I removed it. WTF is going on?

 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: Crypto Fixes for 3.3

[Linus Torvalds]

On Wed, Jan 25, 2012 at 6:43 PM, Herbert Xu herb...@gondor.apana.org.au wrote:

 This push fixes a race condition in sha512 that affects users
 who use it in process context and softirq context concurrently,
 in particular, this affects IPsec.  The result of the race is
 the production of incorrect hashes, which for IPsec leands to
 loss of connectivity.

Ugh. This once more has the crazy signed integer modulus operator,
which can be quite expensive depending on whether the compiler can
tell whether it is always positive or not.

Also, that modulus is exposed everywhere.

In git, the sha1 implementation (which has many of the same issues) does this:

  /* This rolls over the 512-bit array */
  #define W(x) (array[(x)15])

which means that the modulus exists in just one place (and is the
correct binary 'and', not the possibly-expensive division).

We also avoid the problem with absolutely horrible gcc register usage
by having an arch-specific accessor macro:

  /*
   * If you have 32 registers or more, the compiler can (and should)
   * try to change the array[] accesses into registers. However, on
   * machines with less than ~25 registers, that won't really work,
   * and at least gcc will make an unholy mess of it.
   *
   * So to avoid that mess which just slows things down, we force
   * the stores to memory to actually happen (we might be better off
   * with a 'W(t)=(val);asm(:+m (W(t))' there instead, as
   * suggested by Artur Skawina - that will also make gcc unable to
   * try to do the silly optimize away loads part because it won't
   * see what the value will be).
   *
   * Ben Herrenschmidt reports that on PPC, the C version comes close
   * to the optimized asm with this (ie on PPC you don't want that
   * 'volatile', since there are lots of registers).
   *
   * On ARM we get the best code generation by forcing a full memory barrier
   * between each SHA_ROUND, otherwise gcc happily get wild with spilling and
   * the stack frame size simply explode and performance goes down the drain.
   */

  #if defined(__i386__) || defined(__x86_64__)
#define setW(x, val) (*(volatile unsigned int *)W(x) = (val))
  #elif defined(__GNUC__)  defined(__arm__)
#define setW(x, val) do { W(x) = (val); __asm__(:::memory); } while (0)
  #else
#define setW(x, val) (W(x) = (val))
  #endif

which is not pretty, but as you guys found out, the alternative can be
much worse (ie totally crazy gcc register spilling)

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: Crypto Fixes for 3.3

[Linus Torvalds]

On Wed, Jan 25, 2012 at 8:07 PM, Herbert Xu herb...@gondor.apana.org.au wrote:

 Oops, I had incorrectly applied the first patch in the thread.

 I've fixed it in the tree now.

Oh well, I already pulled your tree. I just wanted to voice a few
comments on it.

 We also avoid the problem with absolutely horrible gcc register usage
 by having an arch-specific accessor macro:

 We could certainly do something like that.  Although I'd be
 more comfortable with pushing this through linux-next, OK?

Absolutely.

   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: [PATCH 2/3] sha512: reduce stack usage to safe number

[Linus Torvalds]

On Sat, Jan 14, 2012 at 10:40 AM, Alexey Dobriyan adobri...@gmail.com wrote:

 Line by line explanation:
 * BLEND_OP
  array is circular now, all indexes have to be modulo 16.
  Round number is positive, so remainder operation should be
  without surprises.

Don't use % except on unsigned values. Even if it's positive, if
it's a signed number and the compiler doesn't *see* that it is
absolutely positive, division is nontrivial. Even when you divide by a
constant.

For example, % 16 on an 'int' on x86-64 will generate

movl%edi, %edx
sarl$31, %edx
shrl$28, %edx
leal(%rdi,%rdx), %eax
andl$15, %eax
subl%edx, %eax

in order to get the signed case right. The fact that the end result is
correct for unsigned numbers is irrelevant: it's still stupid and
slow.

With an unsigned int, '% 16' will generate the obvious

andl$15, %eax

instead.

Quite frankly, stop using division in the first place. Dividing by
powers-of-two and expecting the compiler to fix things up is just
stupid, *exactly* because of issues like these: you either have to
think about it carefully, or the compiler may end up creating crap
code.

So just use  15 instead. That doesn't have these kinds of issues.
It is a *good* thing when the C code is close to the end result you
want to generate. It is *not* a good thing to write code that looks
nothing like the end result and just expect the compiler to do the
right thing. Even if the compiler does do the right thing, what was
the advantage?

 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: [PATCH 2/3] sha512: reduce stack usage to safe number

[Linus Torvalds]

On Sat, Jan 14, 2012 at 12:41 PM, Alexey Dobriyan adobri...@gmail.com wrote:

 For the record, it generates andl $15 here.

Ok. That means that gcc was able to prove that it never had any signed
values (which is certainly reasonable when you do things like for
(i=0; iX;i++)). But it's better to simply not rely on gcc always
getting details like this right.

It's also better to use a model that simply doesn't even require you
as a programmer to have to even *think* about signed values.

It's easy to get % wrong by mistake (signed integer modulus didn't
even use to have well-defined semantics in traditional C), and there
is almost never any excuse for using it for powers-of-two.

 Here is updated patch which explicitly uses  (equally tested):

Thanks,

 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: [PATCH 1/3] sha512: make it work, undo percpu message schedule

[Linus Torvalds]

On Sat, Jan 14, 2012 at 1:46 PM, Eric Dumazet eric.duma...@gmail.com wrote:

 This is too risky, and we provided an alternate patch, not just for fun.

Did you see the second patch?

The one that got rid of the *stupid* 80-entry array?

I don't know why so many sha implementations do that idiotic full
array, when the circular one is much better.

In fact, the 16-entry circular array allows machines with lots of
registers to keep all the state in registers and the C implementation
can often be as good as hand-tuned assembly. At least that's true for
sha1, I'm not sure you can do the same with sha512.

But that actually *requires* that the 16-entry array be done on the
stack as an automatic array. Anything else, and the compiler won't be
able 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: Crypto Update for 3.2

[Linus Torvalds]

On Mon, Oct 31, 2011 at 9:42 AM, Randy Dunlap rdun...@xenotime.net wrote:

 Actually adds select NET, a reverse dependency.  :(

 Linus was quite vocal about not allowing MD to select BLOCK.
 See https://lkml.org/lkml/2011/8/10/527
 and https://lkml.org/lkml/2011/8/10/533

 To me this is very similar.

I do agree.

select makes sense when it's a way for a user to not have to care
about some small helper thing that is really not obvious for a casual
user.

But darn it, if somebody has said no networking, then some random
small feature shouldn't suddenly select it.

IOW, it's about relative importance. We should use select when
some feature that should be user-visible selects some details. And we
should use depends on when there's a major subsystem that some small
detail depends on.

So classic and obvious uses where select is appropriate is when a
driver needs some helper library to work (eg select FW_LOADER or
select CRC32).

And a classic and obvious case where depends on is the appropriate
choice is when it depends on a major subsystem (depends on PCI or
depends on USB or depends on X86).

And I think NET definitely falls into that second category - exactly
the same way BLOCK fell into it. You don't select major subsystems
- if somebody turned off the subsystem, we turn off the stuff that
depends on it.

(Of course, in reality, pretty much nobody turns off NET, I suspect.
But if some embedded place really doesn't want it, then damn it, we
shouldn't ask about the odd crypto user interfaces, because they
really aren't major enough, and the embedded platform is clearly
trying very hard to run small).

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: [PATCH] treewide: Update sha_transform

[Linus Torvalds]

On Tue, Aug 9, 2011 at 1:58 AM, Joe Perches j...@perches.com wrote:

 Eliminate possible sha_transform unaligned accesses to data by copying
 data to an aligned __u32 array if necessary.

This is wrong. Not only does it double the stack space, when I tried
it for git it just made things slower. So don't do it.

If some architecture has a bad get_unaligned_be32(), then that's
just something for the arch maintainers to fix.

 Add sha_transform wipe argument to force workspace clearing if desired.

I disagree. As already mentioned, any competent compiler would make
that thing go away anyway. And there is no reason to believe that it's
actually a real fix for any real security issue, so it's voodoo
programming for several reasons.

We shouldn't do voodoo stuff. Or rather, I'm perfectly ok if you guys
all do your little wax figures of me in the privacy of your own homes
- freedom of religion and all that - but please don't do it in the
kernel.

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: [PATCH] lib/sha1: remove memsets and allocate workspace on the stack

[Linus Torvalds]

On Mon, Aug 8, 2011 at 4:07 PM, Mandeep Singh Baines m...@chromium.org wrote:

 There is no loss of security due to removing the memset. It would be a
 bug for the stack to leak to userspace. However, a defence-in-depth
 argument could be made for keeping the clearing of the workspace.

So I'm nervous about this just because I can see the security crazies
rising up about this.

The fact is, in our current code in drivers/char/random.c, we do have
a memset() of the workspace buffer on the stack, and any competent
compiler should actually just remove it, because it's dead memory (and
the compiler can *see* that it's dead memory). Of course, I don't know
if gcc does notice that, but it's a prime example of code that looks
secure, but has absolutely zero actual real security. Getting rid of
the memset() is actually better for *real* security, in that at least
it's not some kind of pointless security theater. But I  can see some
people wanting to add a memory barrier or something to force the
memset() to actually take place.

So I dunno.

Arguably it's theoretically possible to find random data on the stack,
and maybe it can even be interesting (although I don't think the last
64 bytes of SHA1 state is all that exciting myself). Personally, I
consider it unlikely as hell to be relevant to anybody, and anybody
who has access to the kernel stack has *much* more direct security
holes than some random data that they can use. But the patch still
makes me worry about the brouhaha from some people.

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: [PATCH] lib/sha1: use the git implementation of SHA-1

[Linus Torvalds]

There aren't many users of that define, could you just turn it back to the 
proper 16, and then try changing it to 80 in each place that uses it?

That way we'd see exactly *which* use is the buggy one..

Linus

Joachim  Eastwood manab...@gmail.com wrote:

On Sun, Aug 7, 2011 at 7:44 PM, Linus Torvalds
torva...@linux-foundation.org wrote:
 On Sun, Aug 7, 2011 at 10:36 AM, Joachim  Eastwood
manab...@gmail.com wrote:

 These printk's come from drivers/char/random.c
 So it doesn't seem like it hangs in any of the sha_* funtions.

 The only other change is to SHA_WORKSPACE_WORDS - I wonder if some
 code depends on the old (much bigger) workspace for some reason?

 The git SHA1 routines are way smarter than the old SHA1, and will
 re-use the workspace area, so they need only a fraction of the old
 area.

 Try changing SHA_WORKSPACE_WORDS back to 80 (in
 include/linux/cryptohash.h). The git sha1 only needs 16 words, but ..

yup, setting it to 80 makes my kernel boot again :-)

 If that fixes it for you, then it's almost certainly some buggy user
 that uses the SHA1 workspace array for its own odd case, and
 incorrectly knows that it's that old wasteful 320 bytes. There's a
 few places in networking that uses SHA_WORKSPACE_WORDS.

Guess more architectures than ARM are affected by this then.

regards
Joachim Eastwood

                                     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: [PATCH] lib/sha1: use the git implementation of SHA-1

[Linus Torvalds]

On Sun, Aug 7, 2011 at 12:47 PM, Andreas Schwab sch...@linux-m68k.org wrote:

 ARM has its own implementation of sha_transform in arch/arm/lib/sha1.S,
 which assumes SHA_WORKSPACE_WORDS is 80.

Well, that certainly explains it.

I wonder if that thing is worth it. It seems to be based on the bad
slow version of sha1, so I suspect that the biggest advantage of it
may the byte-swapping being done more efficiently. The ARM version of
get_unaligned_be32() is potentially pretty bad.

Joachim, does it all work for you if you just remove 'sha1.o' from
lib-y in arch/arm/lib/Makefile?

Nico (now with corrected email address): is that ARM-optimized asm
really worth it? Compared to the git C implementation?

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: [PATCH] treewide: Remove direct uses of SHA_WORKSPACE_WORDS

[Linus Torvalds]

On Sun, Aug 7, 2011 at 2:29 PM, Joe Perches j...@perches.com wrote:
 While not connected to ARM's implementation of sha_transform,
 maybe this might make code a bit clearer.

 Remove need to know the size and type of SHA_WORKSPACE_WORDS.
 Introduce and use opaque struct sha_workspace instead.

The thing is, that workspace thing is likely just broken, and should be removed.

On machines with sufficient registers, we can keep the workspace in
the register set, and setting it up on stack and passing it in as a
buffer is just bad.

On x86 and arm it doesn't matter, but on ppc it likely pessimizes the
code by resulting in pointless writebacks to that stack buffer.

So the right thing to do is probably to just remove it entirely, and
just make the sha_transform() thing allocate it on the stack as
needed. The only issue then is whether you really want the stackspace
cleared afterwards. My personal guess is that it really doesn't
matter: if you leak stack space anywhere, you are seriously screwed
from a security standpoint anyway, so..

But in the meantime, I don't want to add this kind of abstraction for
something that I suspect is just fundamentally broken.

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: Crypto Update for 2.6.38

[Linus Torvalds]

On Wed, Jan 5, 2011 at 4:01 PM, Herbert Xu herb...@gondor.hengli.com.au wrote:

 * Crypto API interface for user-space (hash + skcipher)

Is there really any point to this? And can we get more explanation of
what the interface is, and who would use it?

If you need crypto in user space, it's almost invariably better done
in user space. If the CPU can do crypto on its own, and doesn't expose
those instructions to user space, it's just a stupid CPU - and the
user/kernel transfer is likely going to make it slower than a pure
software approach for any but the biggest transfers.

And if the crypto engine is off-chip, the sw version is going to be
faster anyway except for possible async versions that are hard to
interface to user space.

So I really need more convincing about the whole user-space interface.
Adding new interfaces willy-nilly isn't a good idea. They need damn
good reasons.

  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: Crypto Update for 2.6.38

[Linus Torvalds]

On Thu, Jan 6, 2011 at 1:16 PM, Herbert Xu herb...@gondor.hengli.com.au wrote:
 On Thu, Jan 06, 2011 at 10:05:46AM -0800, Linus Torvalds wrote:

 Is there really any point to this? And can we get more explanation of
 what the interface is, and who would use it?

 I think you've answered it yourself in the third paragraph :)

No I didn't.

What part of can we get more explanation of what the interface is is unclear?

Explanations of interface. Code. Who uses it? What are the actual
performance benefits on real code?

Quite frankly, asynchronous external devices using DMA or similar are
seldom real performance improvements. The bus and cache traffic tends
to overwhelm any other advantage, and commonly the result is (a) lower
performance with (b) better-looking profiles.

But better-looking profiles isn't actually a real advantage.

And I really do want to hear about new kernel interfaces. What _are_
the interfaces, and what are the advantages to them.

 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: Crypto Update for 2.6.38

[Linus Torvalds]

On Thu, Jan 6, 2011 at 1:39 PM, Herbert Xu herb...@gondor.hengli.com.au wrote:
 On Thu, Jan 06, 2011 at 01:23:19PM -0800, Linus Torvalds wrote:

 Explanations of interface. Code. Who uses it? What are the actual
 performance benefits on real code?

 You snipped out the bit in my reply where I expanded on it:

You didn't expand on it AT ALL.

You just mentioned the interface. I haven't seen WHAT THAT INTERFACE IS!

How hard is that to understand?

 Here is the original cover email for the patches:

Ok, this is more like it. This is roughly what I wanted to see:

 : Here is a sample hash program (note that these only illustrate
 : what the interface looks like and are not meant to be good examples
 : of coding :)

But I'm still missing the part where you show that there is any actual
use case that makes sense, and that actually improves performance.
Maybe it's been posted somewhere else, but the thing is, you're asking
_me_ to pull, and as a result you need to convince _me_ that this is a
good idea. So if it's been posted/discussed extensively elsewhere,
please point to those discussions.

I really don't like adding interfaces that don't have hard uses
associated with them. We've done it in the past, and it tends to be a
morass and a bad idea. That's been true even when the idea has been my
own, and thus obviously genius-level and clearly the RightThing(tm),
like splice(). And it's why I push back on new interfaces when I see
them.

Btw, it doesn't have to be about performance per se. Does this allow
people to use keys without actually _seeing_ those keys? Your example
implies that that is not the case, but that's actually one of the few
reasons to actually support a kernel crypto interface - the ability to
have private personal keys around, but not having to actually let
possibly untrusted programs see them.

For example of why something like that matters, I can well see myself
using some program to encrypt things. But maybe I don't trust that
program enough to give it my actual private keys. In that case, kernel
support is a real feature.

But in your example, it looks like you just give it the key. Which to
me means that you're totally missing one of the major reasons for
having a separate protection domain.

And that makes me think that the interface is bad. And that's why it's
a big change to go from internal kernel crypto interface to actual
user-space interface to the kernel crypto engine. The first one can
be fixed. The second one cannot.

So I'm not necessarily hung up on performance, but I am hung up on
there needs to be a point, and the interface needs to be
-correct-. Performance would be one such point. Not just 'the
hardware is there'. I know the hardware exists, but I'm not at all
convinced that DMA with all the cacheflushing will ever actully be
faster the the CPU. And if it can, I want to hear about the real-world
situation where it actually is used.

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: Crypto Update for 2.6.38

[Linus Torvalds]

On Thu, Jan 6, 2011 at 2:30 PM, Herbert Xu herb...@gondor.hengli.com.au wrote:

 The main use-case is bulk encryption/hashing in user-space.  For
 example, on Sparc Niagara2 you need to use SPU (Stream Processing
 Unit) in order to do crypto at 10Gb/s over the network.

Umm. But doesn't that require that the data then be sent to the network?

Why would a user-space - crypto engine - user space - network chip
thing ever be good enough? Niagara is so slow that the whole bounce
thing will totally negate all the SPU advantages.

Your interface doesn't seem to support the use case that you actually
want, which is to avoid the bouncing back and forth between user space
buffers.

And if you bounce back and forth, I bet you can't get that 10Gb/s anyway.

Can you do the bypass directly to the TCP stream with the interface
you added? It isn't at all obvious how it would work.

So let me repeat ONE MORE TIME:

 - I understand that your interface can use the hw that exists

 - but I still want real-world use cases to show that it actually
works and makes sense in practice.

Don't give me we could use the SPU crap. Give me this program
actually uses the SPU and gets better performance thanks to it, and
here are the numbers.

 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: Crypto Update for 2.6.38

[Linus Torvalds]

On Thu, Jan 6, 2011 at 2:53 PM, Herbert Xu herb...@gondor.hengli.com.au wrote:
 On Thu, Jan 06, 2011 at 02:43:35PM -0800, Linus Torvalds wrote:

 Can you do the bypass directly to the TCP stream with the interface
 you added? It isn't at all obvious how it would work.

 Yes it can.  The interface allows zero-copy in both directions
 using the splice interface.  Here is a sample program demonstrating
 zero-copy in-place encryption.  It doesn't send the result over TCP
 but I'm sure you can imagine what that would look like.

Ok. So can we actually get numbers for this?

Put another way: I really really REALLY don't want to merge new
user-space interfaces that don't actually work in reality. But if this
allows direct encryption to a network interface, and it actually is
able to saturate 10Gb on niagara (unlike a user-mode encryption thing,
I assume, since those things are dog slow), then that would certainly
be a good real-life test.

But I really don't want to merge it unless it has had at least
real-life testing of actually doing better than regular sw user-space
encryption.

I realize that on PC's, it's unlikely to ever help. So I'm not asking
for show me how this helps on my hardware. But I do want to get some
case on _some_ actual hardware where it works on a real load.

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: Crypto Fixes for 2.6.37

[Linus Torvalds]

On Wed, Dec 15, 2010 at 3:50 AM, Herbert Xu
herb...@gondor.hengli.com.au wrote:

 This push fixes a build problem under certain configurations due
 to a missing include.

 Please pull from

 git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git

You have a bad repo. Git says:

  fatal: loose object 52f6c5ad430e41736133acac179607b224eaaa11 (stored
in ./objects/52/f6c5ad430e41736133acac179607b224eaaa11) is corrupted

and it doesn't really seem to be corrupt as much as just unreadable
(ie you've made it readable only by yourself).

There's a few other objects like that too. How do you push to your
repo, and what changed?

  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

[Linus Torvalds]

On Fri, Aug 6, 2010 at 1:06 AM, Olivier Galibert galib...@pobox.com 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

[Linus Torvalds]

On Thu, Aug 5, 2010 at 6:40 PM, Herbert Xu herb...@gondor.hengli.com.au 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

[Linus Torvalds]

On Thu, Aug 5, 2010 at 7:23 PM, David Howells dhowe...@redhat.com 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

[Linus Torvalds]

On Thu, Aug 5, 2010 at 7:35 PM, Herbert Xu herb...@gondor.hengli.com.au 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: Crypto Fixes for 2.6.35

[Linus Torvalds]

On Thu, 3 Jun 2010, Herbert Xu wrote:
 
 Please pull from
 
 git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git

Already up-to-date. Forgot to push? (I also checked master, so it's not 
that mirroring is slow)

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: Crypto Update for 2.6.28

[Linus Torvalds]

On Thu, 25 Dec 2008, Herbert Xu wrote:
 
 A regression has been reported where the new algorithm testing
 infrastructure may cause the optimised versions of AES to fail
 when it's built into the kernel (as opposed to as a module).

Can't we just get rid of that stupid testing infrastructure, or make it a 
module-only thing? I refuse to pull a 1000+ line patch for something like 
this, especially this late. It's insane.

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: Crypto Update for 2.6.28

[Linus Torvalds]

On Fri, 10 Oct 2008, Herbert Xu wrote:
 
 Here is the crypto update for 2.6.28:

Umm. Here was empty.

Mind adding where it actually is, again?

Yeah, yeah, I can look at my old merges (you didn't think I _remember_ 
things, did you?), and I can see that it's going to be

git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6

but really, please do always say where things are supposed to come from!

Also, please us

git diff -M --stat --summary

for the diffstat. That way I would have seen the

 crypto/{cryptomgr.c = algboss.c}  |   92 +-

as a rename in the email, instead of having to look why my numbers don't 
match the numbers you claimed..

Linus
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/6] MODSIGN: Kernel module signing

[Linus Torvalds]

On Wed, 14 Feb 2007, David Howells wrote:

  (1) A cut-down MPI library derived from GPG with error handling added.

Do we really need to add this?

Wouldn't it be much nicer to just teach people to use one of the existing 
signature things that we need for _other_ cases anyway, and already have 
merged?

(Of course, it's possible that none of the current crypto supports any 
signature checking at all - I didn't actually look. In which case my 
argument is pointless).

Linus
-
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Creating diff from 2.6.16 from cryptodev-2.6 git tree

[Linus Torvalds]

On Fri, 30 Jun 2006, Michal Ludvig wrote:

 Linus Torvalds wrote:
 
  git log -p --full-diff v2.6.16.. crypto/
 
 Can I somehow get the result in a reverse order, i.e. oldest commits first?

Not that way, no. git log generates the data on-the-fly, so a simple 
git log will always give most recent first.

However, you can do this

git log --pretty=oneline v2.6.16.. crypto/

to generate a list of commits, one per line. Then, reverse that list 
(tac is your friend), and feed it back to git-diff-tree --stdin 
--pretty -p to get the diffs.

So

git log --pretty=oneline v2.6.16.. crypto/ |
tac |
git-diff-tree --stdin --pretty -p

should do what you want.

Of course, the patches (to other files than the crypt/ subdirectory) may 
still clash due to changes that are unrelated to the crypto changes. That 
is unavoidable, and you'll just have to fix that up by hand.

Linus
-
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Creating diff from 2.6.16 from cryptodev-2.6 git tree

[Linus Torvalds]

On Fri, 30 Jun 2006, Herbert Xu wrote:

 On Thu, Jun 29, 2006 at 07:25:01PM -0700, Linus Torvalds wrote:
  
  The easiest by far is if you only care about a certain sub-directory. 
  Then, assuming the branch crypto is the top-most commit of the cryptodev 
  repo, just do
  
  git diff v2.6.16..crypto -- crypto/
 
 Yes, you should always do something like this when backporting to make
 sure that you haven't missed patches merged in ways that you didn't
 anticipate.
 
 Although I'm not aware of any such patches in the time frame that Michal
 has in mind.

Note that this is why

git log -p --full-diff v2.6.16.. crypto/

is so great. It will show everything that touched that subdirectory, along 
with the incidentals. I think that in this case, that's exactly what 
Michal wants.

Linus
-
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html