* RE: [PATCH] virtio-crypto: adjust priority of algorithm
From: Gonglei (Arei) @ 2017-01-13 9:24 UTC (permalink / raw)
To: Christian Borntraeger, virtualization@lists.linux-foundation.org,
linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: herbert@gondor.apana.org.au, mst@redhat.com
In-Reply-To: <874ee338-4dd6-8a2c-fb95-59bd784de026@de.ibm.com>
>
> From: Christian Borntraeger [mailto:borntraeger@de.ibm.com]
> Sent: Friday, January 13, 2017 4:28 PM
> To: Gonglei (Arei); virtualization@lists.linux-foundation.org;
> linux-crypto@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: mst@redhat.com; herbert@gondor.apana.org.au
> Subject: Re: [PATCH] virtio-crypto: adjust priority of algorithm
>
> ACK. Whoever takes this patch might want to fixup 3 typos.
>
Thanks, I'd better send v2 IMHO. :)
Regards,
-Gonglei
> On 01/13/2017 07:25 AM, Gonglei wrote:
>
> > Some hardware accelerators (like intel aseni or the s390
> aesni
> > cpacf functions) have lower priorities than virtio
> > crypto, and those drivers are faster than the same in
> > the host via virtio. So let's lower the priority of
> > virtio-crypto's algorithm, make it's higher than sofeware
> software
> > implimentations but lower than the hardware ones.
> implementations
> >
> > Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > ---
> > drivers/crypto/virtio/virtio_crypto_algs.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c
> b/drivers/crypto/virtio/virtio_crypto_algs.c
> > index 6f40a42..4de4740 100644
> > --- a/drivers/crypto/virtio/virtio_crypto_algs.c
> > +++ b/drivers/crypto/virtio/virtio_crypto_algs.c
> > @@ -498,7 +498,7 @@ void virtio_crypto_ablkcipher_finalize_req(
> > static struct crypto_alg virtio_crypto_algs[] = { {
> > .cra_name = "cbc(aes)",
> > .cra_driver_name = "virtio_crypto_aes_cbc",
> > - .cra_priority = 501,
> > + .cra_priority = 150,
> > .cra_flags = CRYPTO_ALG_TYPE_ABLKCIPHER | CRYPTO_ALG_ASYNC,
> > .cra_blocksize = AES_BLOCK_SIZE,
> > .cra_ctxsize = sizeof(struct virtio_crypto_ablkcipher_ctx),
> >
^ permalink raw reply
* Re: [PATCH] crypto: testmgr - don't DMA map IV from stack in test_skcipher()
From: Herbert Xu @ 2017-01-13 8:46 UTC (permalink / raw)
To: Horia Geantă; +Cc: David S. Miller, linux-crypto
In-Reply-To: <1484290756-20868-1-git-send-email-horia.geanta@nxp.com>
On Fri, Jan 13, 2017 at 08:59:16AM +0200, Horia Geantă wrote:
> Fix the "DMA-API: device driver maps memory from stack" warning
> generated when crypto accelerators map the IV.
>
> Signed-off-by: Horia Geantă <horia.geanta@nxp.com>
Hmm, the IV comes in as a pointer. So you should not assume that
it can be DMAed at all.
Perhaps we should change the API so that it gets passed in as an
SG list.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: x86-64: Maintain 16-byte stack alignment
From: Herbert Xu @ 2017-01-13 8:43 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Linus Torvalds, Linux Crypto Mailing List, Thomas Gleixner,
Andy Lutomirski, Ingo Molnar, Andy Lutomirski,
Linux Kernel Mailing List, Ard Biesheuvel
In-Reply-To: <20170113050709.yrdtfet5d4sebubi@treble>
On Thu, Jan 12, 2017 at 11:07:09PM -0600, Josh Poimboeuf wrote:
>
> Disabling frame pointers didn't seem to help, but I finally got it to
> misalign with a different test case. I think it had been aligning the
> array, so instead I made it push a register.
Right. If you start manipulating the stack directly then all bets
are off.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: x86-64: Maintain 16-byte stack alignment
From: Herbert Xu @ 2017-01-13 8:42 UTC (permalink / raw)
To: Linus Torvalds
Cc: Josh Poimboeuf, Linux Crypto Mailing List, Thomas Gleixner,
Andy Lutomirski, Ingo Molnar, Andy Lutomirski,
Linux Kernel Mailing List, Ard Biesheuvel
In-Reply-To: <CA+55aFzRrSwGxxfZk-RUEnsz=xhcSmOwE1CenfCPBWtsS9MwDw@mail.gmail.com>
On Thu, Jan 12, 2017 at 08:37:18PM -0800, Linus Torvalds wrote:
>
> So then the compiler actually needs to start adding useless instructions
> just to keep the stack 16-byte aligned.
Which it does. Of course most of the time no extra instructions
are required because there are stack variables, so it's just matter
of adding 8 to the value you're subtracting from rsp. But it is
probably why gcc assumes that the stack is 16-byte aligned which
triggered my original crash.
Here is an example from the function that was involved in the crash,
without frame pointers:
00000000000001b0 <chacha20_simd>:
1b0: 41 54 push %r12
1b2: 55 push %rbp
1b3: 48 81 ec f8 00 00 00 sub $0xf8,%rsp
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: x86-64: Maintain 16-byte stack alignment
From: Herbert Xu @ 2017-01-13 8:39 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Josh Poimboeuf, Linus Torvalds, Linux Kernel Mailing List,
Linux Crypto Mailing List, Ingo Molnar, Thomas Gleixner,
Andy Lutomirski, Ard Biesheuvel
In-Reply-To: <CALCETrXom8aY2XhpAyOtAwQQYF7wftBHJE_px1xr0iRmcYEJoA@mail.gmail.com>
On Thu, Jan 12, 2017 at 05:46:55PM -0800, Andy Lutomirski wrote:
>
> What I mean is: what guarantees that the stack is properly aligned for
> the subroutine call? gcc promises to set up a stack frame, but does
> it promise that rsp will be properly aligned to call a C function?
Yes, as long as you don't go behind its back and start directly
pushing things onto the stack with inline asm.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: x86-64: Maintain 16-byte stack alignment
From: Herbert Xu @ 2017-01-13 8:38 UTC (permalink / raw)
To: Linus Torvalds
Cc: Josh Poimboeuf, Andy Lutomirski, Linux Kernel Mailing List,
Linux Crypto Mailing List, Ingo Molnar, Thomas Gleixner,
Andy Lutomirski, Ard Biesheuvel
In-Reply-To: <CA+55aFwkodRf3QM+GLzq5G4F7fp+=ds-cHJRtCgJczMZgVx6Ug@mail.gmail.com>
On Thu, Jan 12, 2017 at 01:40:54PM -0800, Linus Torvalds wrote:
>
> 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).
The question is does it really make sense to save those 8 bytes
of padding on x86-64 when arm64 apparently also requires 16-byte
stack alignment.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: x86-64: Maintain 16-byte stack alignment
From: Herbert Xu @ 2017-01-13 8:36 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Linus Torvalds, Josh Poimboeuf, Linux Kernel Mailing List,
Linux Crypto Mailing List, Ingo Molnar, Thomas Gleixner,
Andy Lutomirski, Ard Biesheuvel
In-Reply-To: <CALCETrVz-wEFVUwrpS8-Ln9SWnsF5KxkqJC-Br6wJ+e0LGM9UA@mail.gmail.com>
On Thu, Jan 12, 2017 at 12:08:07PM -0800, Andy Lutomirski wrote:
>
> I think we have some inline functions that do asm volatile ("call
> ..."), and I don't see any credible way of forcing alignment short of
> generating an entirely new stack frame and aligning that. Ick. This
A straight asm call from C should always work because gcc keeps
the stack aligned in the prologue.
The only problem with inline assembly is when you start pushing
things onto the stack directly.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* [PATCH] crypto: arm/aes - avoid reserved 'tt' mnemonic in asm code
From: Ard Biesheuvel @ 2017-01-13 8:33 UTC (permalink / raw)
To: linux-crypto; +Cc: linux-arm-kernel, herbert, arnd, Ard Biesheuvel
The ARMv8-M architecture introduces 'tt' and 'ttt' instructions,
which means we can no longer use 'tt' as a register alias on recent
versions of binutils for ARM. So replace the alias with 'ttab'.
Fixes: 81edb4262975 ("crypto: arm/aes - replace scalar AES cipher")
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
arch/arm/crypto/aes-cipher-core.S | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/arm/crypto/aes-cipher-core.S b/arch/arm/crypto/aes-cipher-core.S
index b04261e1e068..c817a86c4ca8 100644
--- a/arch/arm/crypto/aes-cipher-core.S
+++ b/arch/arm/crypto/aes-cipher-core.S
@@ -18,7 +18,7 @@
rounds .req r1
in .req r2
out .req r3
- tt .req ip
+ ttab .req ip
t0 .req lr
t1 .req r2
@@ -34,9 +34,9 @@
.macro __load, out, in, idx
.if __LINUX_ARM_ARCH__ < 7 && \idx > 0
- ldr \out, [tt, \in, lsr #(8 * \idx) - 2]
+ ldr \out, [ttab, \in, lsr #(8 * \idx) - 2]
.else
- ldr \out, [tt, \in, lsl #2]
+ ldr \out, [ttab, \in, lsl #2]
.endif
.endm
@@ -136,7 +136,7 @@
eor r6, r6, r10
eor r7, r7, r11
- __adrl tt, \ttab
+ __adrl ttab, \ttab
tst rounds, #2
bne 1f
@@ -146,7 +146,7 @@
1: subs rounds, rounds, #4
\round r8, r9, r10, r11, r4, r5, r6, r7
- __adrl tt, \ltab, ls
+ __adrl ttab, \ltab, ls
\round r4, r5, r6, r7, r8, r9, r10, r11
bhi 0b
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] virtio-crypto: adjust priority of algorithm
From: Christian Borntraeger @ 2017-01-13 8:28 UTC (permalink / raw)
To: Gonglei, virtualization, linux-crypto, linux-kernel; +Cc: herbert, mst
In-Reply-To: <1484288741-31100-1-git-send-email-arei.gonglei@huawei.com>
ACK. Whoever takes this patch might want to fixup 3 typos.
On 01/13/2017 07:25 AM, Gonglei wrote:
> Some hardware accelerators (like intel aseni or the s390
aesni
> cpacf functions) have lower priorities than virtio
> crypto, and those drivers are faster than the same in
> the host via virtio. So let's lower the priority of
> virtio-crypto's algorithm, make it's higher than sofeware
software
> implimentations but lower than the hardware ones.
implementations
>
> Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
> drivers/crypto/virtio/virtio_crypto_algs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c b/drivers/crypto/virtio/virtio_crypto_algs.c
> index 6f40a42..4de4740 100644
> --- a/drivers/crypto/virtio/virtio_crypto_algs.c
> +++ b/drivers/crypto/virtio/virtio_crypto_algs.c
> @@ -498,7 +498,7 @@ void virtio_crypto_ablkcipher_finalize_req(
> static struct crypto_alg virtio_crypto_algs[] = { {
> .cra_name = "cbc(aes)",
> .cra_driver_name = "virtio_crypto_aes_cbc",
> - .cra_priority = 501,
> + .cra_priority = 150,
> .cra_flags = CRYPTO_ALG_TYPE_ABLKCIPHER | CRYPTO_ALG_ASYNC,
> .cra_blocksize = AES_BLOCK_SIZE,
> .cra_ctxsize = sizeof(struct virtio_crypto_ablkcipher_ctx),
>
^ permalink raw reply
* [PATCH] crypto: testmgr - don't DMA map IV from stack in test_skcipher()
From: Horia Geantă @ 2017-01-13 6:59 UTC (permalink / raw)
To: Herbert Xu; +Cc: David S. Miller, linux-crypto
Fix the "DMA-API: device driver maps memory from stack" warning
generated when crypto accelerators map the IV.
Signed-off-by: Horia Geantă <horia.geanta@nxp.com>
---
crypto/testmgr.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 500a5277cc22..64245aeef634 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -1081,12 +1081,16 @@ static int __test_skcipher(struct crypto_skcipher *tfm, int enc,
const char *e, *d;
struct tcrypt_result result;
void *data;
- char iv[MAX_IVLEN];
+ char *iv;
char *xbuf[XBUFSIZE];
char *xoutbuf[XBUFSIZE];
int ret = -ENOMEM;
unsigned int ivsize = crypto_skcipher_ivsize(tfm);
+ iv = kmalloc(MAX_IVLEN, GFP_KERNEL);
+ if (!iv)
+ return ret;
+
if (testmgr_alloc_buf(xbuf))
goto out_nobuf;
@@ -1328,6 +1332,7 @@ static int __test_skcipher(struct crypto_skcipher *tfm, int enc,
out_nooutbuf:
testmgr_free_buf(xbuf);
out_nobuf:
+ kfree(iv);
return ret;
}
--
2.4.4
^ permalink raw reply related
* [PATCH] virtio-crypto: adjust priority of algorithm
From: Gonglei @ 2017-01-13 6:25 UTC (permalink / raw)
To: virtualization, linux-crypto, linux-kernel
Cc: mst, herbert, borntraeger, Gonglei
Some hardware accelerators (like intel aseni or the s390
cpacf functions) have lower priorities than virtio
crypto, and those drivers are faster than the same in
the host via virtio. So let's lower the priority of
virtio-crypto's algorithm, make it's higher than sofeware
implimentations but lower than the hardware ones.
Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
drivers/crypto/virtio/virtio_crypto_algs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c b/drivers/crypto/virtio/virtio_crypto_algs.c
index 6f40a42..4de4740 100644
--- a/drivers/crypto/virtio/virtio_crypto_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_algs.c
@@ -498,7 +498,7 @@ void virtio_crypto_ablkcipher_finalize_req(
static struct crypto_alg virtio_crypto_algs[] = { {
.cra_name = "cbc(aes)",
.cra_driver_name = "virtio_crypto_aes_cbc",
- .cra_priority = 501,
+ .cra_priority = 150,
.cra_flags = CRYPTO_ALG_TYPE_ABLKCIPHER | CRYPTO_ALG_ASYNC,
.cra_blocksize = AES_BLOCK_SIZE,
.cra_ctxsize = sizeof(struct virtio_crypto_ablkcipher_ctx),
--
1.8.3.1
^ permalink raw reply related
* Re: x86-64: Maintain 16-byte stack alignment
From: Josh Poimboeuf @ 2017-01-13 5:07 UTC (permalink / raw)
To: Linus Torvalds
Cc: Linux Crypto Mailing List, Thomas Gleixner, Herbert Xu,
Andy Lutomirski, Ingo Molnar, Andy Lutomirski,
Linux Kernel Mailing List, Ard Biesheuvel
In-Reply-To: <CA+55aFzRrSwGxxfZk-RUEnsz=xhcSmOwE1CenfCPBWtsS9MwDw@mail.gmail.com>
On Thu, Jan 12, 2017 at 08:37:18PM -0800, Linus Torvalds wrote:
> On Jan 12, 2017 8:28 PM, "Josh Poimboeuf" <jpoimboe@redhat.com> wrote:
>
>
> The stack frame was always 16-byte aligned regardless of whether the
> buf array size was even or odd.
>
>
> Including with -fomit-frame-pointer?
>
> With frame pointers, stack frames really are naturally 16 bytes, and then
> keeping the frame 16-byte aligned is just a matter of making any extra
> frame allocations or push/pop sequences that you do also be a multiple of
> 16 bytes.
>
> But *without* frame pointers, the"native" frame size is just 8 bytes, and a
> function that doesn't need any other local storage and then calls another
> function (think various trivial wrapper functions that just add an argument
> and then munge the return value) would thus naturally cause the frame to
> become misaligned.
>
> So then the compiler actually needs to start adding useless instructions
> just to keep the stack 16-byte aligned.
Disabling frame pointers didn't seem to help, but I finally got it to
misalign with a different test case. I think it had been aligning the
array, so instead I made it push a register.
void otherfunc(void);
static inline void bar(int f)
{
register void *__sp asm(_ASM_SP);
asm volatile("call otherfunc" : "+r" (__sp) : "b"(f));
}
void foo(void)
{
bar(5);
}
00000000000020f0 <foo>:
20f0: 55 push %rbp
20f1: 48 89 e5 mov %rsp,%rbp
20f4: 53 push %rbx
20f5: bb 05 00 00 00 mov $0x5,%ebx
20fa: e8 00 00 00 00 callq 20ff <foo+0xf>
20fb: R_X86_64_PC32 otherfunc-0x4
20ff: 5b pop %rbx
2100: 5d pop %rbp
2101: c3 retq
2102: 0f 1f 40 00 nopl 0x0(%rax)
2106: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
210d: 00 00 00
--
Josh
^ permalink raw reply
* Re: x86-64: Maintain 16-byte stack alignment
From: Josh Poimboeuf @ 2017-01-13 4:27 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Linus Torvalds, Herbert Xu, Linux Kernel Mailing List,
Linux Crypto Mailing List, Ingo Molnar, Thomas Gleixner,
Andy Lutomirski, Ard Biesheuvel
In-Reply-To: <CALCETrUFCFn-rKnr+NG3SU7J78ree9siJC=Kz8f_Bk6eG2HyPA@mail.gmail.com>
On Thu, Jan 12, 2017 at 07:23:18PM -0800, Andy Lutomirski wrote:
> On Thu, Jan 12, 2017 at 7:11 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Thu, Jan 12, 2017 at 05:46:55PM -0800, Andy Lutomirski wrote:
> >> On Thu, Jan 12, 2017 at 12:15 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >> > On Thu, Jan 12, 2017 at 12:08:07PM -0800, Andy Lutomirski wrote:
> >> >> On Thu, Jan 12, 2017 at 11:51 AM, Linus Torvalds
> >> >> <torvalds@linux-foundation.org> wrote:
> >> >> > On Thu, Jan 12, 2017 at 6:02 AM, Josh Poimboeuf <jpoimboe@redhat.com> 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).
> >> >>
> >> >> What I meant was checking the entry code to see if it aligns stack
> >> >> frames, and good luck getting sparse to do that. Hmm, getting 16-byte
> >> >> alignment for real may actually be entirely a lost cause. After all,
> >> >> I think we have some inline functions that do asm volatile ("call
> >> >> ..."), and I don't see any credible way of forcing alignment short of
> >> >> generating an entirely new stack frame and aligning that.
> >> >
> >> > Actually we already found all such cases and fixed them by forcing a new
> >> > stack frame, thanks to objtool. For example, see 55a76b59b5fe.
> >>
> >> What I mean is: what guarantees that the stack is properly aligned for
> >> the subroutine call? gcc promises to set up a stack frame, but does
> >> it promise that rsp will be properly aligned to call a C function?
> >
> > Yes, I did an experiment and you're right. I had naively assumed that
> > all stack frames would be aligned.
>
> Just to check: did you do your experiment with -mpreferred-stack-boundary=4?
Yes, but it's too late for me to be doing hard stuff and I think my
first experiment was bogus. I didn't use all the other kernel-specific
gcc options.
I tried again with all the kernel gcc options, except with
-mpreferred-stack-boundary=4 instead of 3, and actually came up with the
opposite conclusion.
I used the following code:
void otherfunc(void);
static inline void bar(long *f)
{
asm volatile("call otherfunc" : : "m" (f) : );
}
void foo(void)
{
long buf[3] = {0, 0, 0};
bar(buf);
}
The stack frame was always 16-byte aligned regardless of whether the
buf array size was even or odd.
So my half-asleep brain is telling me that my original assumption was
right.
--
Josh
^ permalink raw reply
* Re: x86-64: Maintain 16-byte stack alignment
From: Andy Lutomirski @ 2017-01-13 3:23 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Linus Torvalds, Herbert Xu, Linux Kernel Mailing List,
Linux Crypto Mailing List, Ingo Molnar, Thomas Gleixner,
Andy Lutomirski, Ard Biesheuvel
In-Reply-To: <20170113031107.mgitq54fmjnrvi6f@treble>
On Thu, Jan 12, 2017 at 7:11 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Thu, Jan 12, 2017 at 05:46:55PM -0800, Andy Lutomirski wrote:
>> On Thu, Jan 12, 2017 at 12:15 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> > On Thu, Jan 12, 2017 at 12:08:07PM -0800, Andy Lutomirski wrote:
>> >> On Thu, Jan 12, 2017 at 11:51 AM, Linus Torvalds
>> >> <torvalds@linux-foundation.org> wrote:
>> >> > On Thu, Jan 12, 2017 at 6:02 AM, Josh Poimboeuf <jpoimboe@redhat.com> 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).
>> >>
>> >> What I meant was checking the entry code to see if it aligns stack
>> >> frames, and good luck getting sparse to do that. Hmm, getting 16-byte
>> >> alignment for real may actually be entirely a lost cause. After all,
>> >> I think we have some inline functions that do asm volatile ("call
>> >> ..."), and I don't see any credible way of forcing alignment short of
>> >> generating an entirely new stack frame and aligning that.
>> >
>> > Actually we already found all such cases and fixed them by forcing a new
>> > stack frame, thanks to objtool. For example, see 55a76b59b5fe.
>>
>> What I mean is: what guarantees that the stack is properly aligned for
>> the subroutine call? gcc promises to set up a stack frame, but does
>> it promise that rsp will be properly aligned to call a C function?
>
> Yes, I did an experiment and you're right. I had naively assumed that
> all stack frames would be aligned.
Just to check: did you do your experiment with -mpreferred-stack-boundary=4?
--Andy
^ permalink raw reply
* Re: [RFC PATCH 5/6] crypto: aesni-intel - Add bulk request support
From: Eric Biggers @ 2017-01-13 3:19 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: Herbert Xu, linux-crypto, dm-devel, Mike Snitzer, Milan Broz,
Mikulas Patocka, Binoy Jayan
In-Reply-To: <c32a28630157c619ac2a7c851be586e72f193c68.1484215956.git.omosnacek@gmail.com>
On Thu, Jan 12, 2017 at 01:59:57PM +0100, Ondrej Mosnacek wrote:
> This patch implements bulk request handling in the AES-NI crypto drivers.
> The major advantage of this is that with bulk requests, the kernel_fpu_*
> functions (which are usually quite slow) are now called only once for the whole
> request.
>
Hi Ondrej,
To what extent does the performance benefit of this patchset result from just
the reduced numbers of calls to kernel_fpu_begin() and kernel_fpu_end()?
If it's most of the benefit, would it make any sense to optimize
kernel_fpu_begin() and kernel_fpu_end() instead?
And if there are other examples besides kernel_fpu_begin/kernel_fpu_end where
the bulk API would provide a significant performance boost, can you mention
them?
Interestingly, the arm64 equivalent to kernel_fpu_begin()
(kernel_neon_begin_partial() in arch/arm64/kernel/fpsimd.c) appears to have an
optimization where the SIMD registers aren't saved if they were already saved.
I wonder why something similar isn't done on x86.
Eric
^ permalink raw reply
* Re: x86-64: Maintain 16-byte stack alignment
From: Josh Poimboeuf @ 2017-01-13 3:11 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Linus Torvalds, Herbert Xu, Linux Kernel Mailing List,
Linux Crypto Mailing List, Ingo Molnar, Thomas Gleixner,
Andy Lutomirski, Ard Biesheuvel
In-Reply-To: <CALCETrXom8aY2XhpAyOtAwQQYF7wftBHJE_px1xr0iRmcYEJoA@mail.gmail.com>
On Thu, Jan 12, 2017 at 05:46:55PM -0800, Andy Lutomirski wrote:
> On Thu, Jan 12, 2017 at 12:15 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Thu, Jan 12, 2017 at 12:08:07PM -0800, Andy Lutomirski wrote:
> >> On Thu, Jan 12, 2017 at 11:51 AM, Linus Torvalds
> >> <torvalds@linux-foundation.org> wrote:
> >> > On Thu, Jan 12, 2017 at 6:02 AM, Josh Poimboeuf <jpoimboe@redhat.com> 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).
> >>
> >> What I meant was checking the entry code to see if it aligns stack
> >> frames, and good luck getting sparse to do that. Hmm, getting 16-byte
> >> alignment for real may actually be entirely a lost cause. After all,
> >> I think we have some inline functions that do asm volatile ("call
> >> ..."), and I don't see any credible way of forcing alignment short of
> >> generating an entirely new stack frame and aligning that.
> >
> > Actually we already found all such cases and fixed them by forcing a new
> > stack frame, thanks to objtool. For example, see 55a76b59b5fe.
>
> What I mean is: what guarantees that the stack is properly aligned for
> the subroutine call? gcc promises to set up a stack frame, but does
> it promise that rsp will be properly aligned to call a C function?
Yes, I did an experiment and you're right. I had naively assumed that
all stack frames would be aligned.
--
Josh
^ permalink raw reply
* RE: [PATCH v8 1/1] crypto: add virtio-crypto driver
From: Gonglei (Arei) @ 2017-01-13 1:56 UTC (permalink / raw)
To: Michael S. Tsirkin, Christian Borntraeger
Cc: linux-kernel@vger.kernel.org, qemu-devel@nongnu.org,
virtio-dev@lists.oasis-open.org,
virtualization@lists.linux-foundation.org,
linux-crypto@vger.kernel.org, davem@davemloft.net,
herbert@gondor.apana.org.au, Huangweidong (C), Claudio Fontana,
Luonengjun, Hanweidong (Randy), Xuquan (Quan Xu),
Wanzongshun (Vincent), stefanha@redhat.com,
"Zhoujian (jay, Euler)" <
In-Reply-To: <20170112161729-mutt-send-email-mst@kernel.org>
>
> On Thu, Jan 12, 2017 at 03:10:25PM +0100, Christian Borntraeger wrote:
> > On 01/10/2017 01:56 PM, Christian Borntraeger wrote:
> > > On 01/10/2017 01:36 PM, Gonglei (Arei) wrote:
> > >> Hi,
> > >>
> > >>>
> > >>> On 12/15/2016 03:03 AM, Gonglei wrote:
> > >>> [...]
> > >>>> +
> > >>>> +static struct crypto_alg virtio_crypto_algs[] = { {
> > >>>> + .cra_name = "cbc(aes)",
> > >>>> + .cra_driver_name = "virtio_crypto_aes_cbc",
> > >>>> + .cra_priority = 501,
> > >>>
> > >>>
> > >>> This is still higher than the hardware-accelerators (like intel aesni or the
> > >>> s390 cpacf functions or the arm hw). aesni and s390/cpacf are supported
> by the
> > >>> hardware virtualization and available to the guests. I do not see a way
> how
> > >>> virtio
> > >>> crypto can be faster than that (in the end it might be cpacf/aesni +
> overhead)
> > >>> instead it will very likely be slower.
> > >>> So we should use a number that is higher than software implementations
> but
> > >>> lower than the hw ones.
> > >>>
> > >>> Just grepping around, the software ones seem be be around 100 and the
> > >>> hardware
> > >>> ones around 200-400. So why was 150 not enough?
> > >>>
> > >> I didn't find a documentation about how we use the priority, and I assumed
> > >> people use virtio-crypto will configure hardware accelerators in the
> > >> host. So I choosed the number which bigger than aesni's priority.
> > >
> > > Yes, but the aesni driver will only bind if there is HW support in the guest.
> > > And if aesni is available in the guest (or the s390 aes function from cpacf)
> > > it will always be faster than the same in the host via virtio.So your priority
> > > should be smaller.
> >
> >
> > any opinion on this?
>
> Going forward, we might add an emulated aesni device and that might
> become slower than virtio. OTOH if or when this happens, we can solve it
> by adding a priority or a feature flag to virtio to raise its priority.
>
> So I think I agree with Christian here, let's lower the priority.
> Gonglei, could you send a patch like this?
>
OK, will do.
Thanks,
-Gonglei
^ permalink raw reply
* Re: x86-64: Maintain 16-byte stack alignment
From: Andy Lutomirski @ 2017-01-13 1:46 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Linus Torvalds, Herbert Xu, Linux Kernel Mailing List,
Linux Crypto Mailing List, Ingo Molnar, Thomas Gleixner,
Andy Lutomirski, Ard Biesheuvel
In-Reply-To: <20170112201511.yj5ekqmj76r2yv6t@treble>
On Thu, Jan 12, 2017 at 12:15 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Thu, Jan 12, 2017 at 12:08:07PM -0800, Andy Lutomirski wrote:
>> On Thu, Jan 12, 2017 at 11:51 AM, Linus Torvalds
>> <torvalds@linux-foundation.org> wrote:
>> > On Thu, Jan 12, 2017 at 6:02 AM, Josh Poimboeuf <jpoimboe@redhat.com> 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).
>>
>> What I meant was checking the entry code to see if it aligns stack
>> frames, and good luck getting sparse to do that. Hmm, getting 16-byte
>> alignment for real may actually be entirely a lost cause. After all,
>> I think we have some inline functions that do asm volatile ("call
>> ..."), and I don't see any credible way of forcing alignment short of
>> generating an entirely new stack frame and aligning that.
>
> Actually we already found all such cases and fixed them by forcing a new
> stack frame, thanks to objtool. For example, see 55a76b59b5fe.
What I mean is: what guarantees that the stack is properly aligned for
the subroutine call? gcc promises to set up a stack frame, but does
it promise that rsp will be properly aligned to call a C function?
^ permalink raw reply
* [cryptodev:master 43/44] arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr tt,=crypto_ft_tab'
From: kbuild test robot @ 2017-01-12 23:28 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: kbuild-all, linux-crypto, Herbert Xu
[-- Attachment #1: Type: text/plain, Size: 8989 bytes --]
tree: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
head: 1abee99eafab67fb1c98f9ecfc43cd5735384a86
commit: 81edb42629758bacdf813dd5e4542ae26e3ad73a [43/44] crypto: arm/aes - replace scalar AES cipher
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 81edb42629758bacdf813dd5e4542ae26e3ad73a
# save the attached .config to linux build tree
make.cross ARCH=arm
All errors (new ones prefixed by >>):
arch/arm/crypto/aes-cipher-core.S: Assembler messages:
arch/arm/crypto/aes-cipher-core.S:21: Error: selected processor does not support `tt .req ip' in ARM mode
>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr tt,=crypto_ft_tab'
arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r8,[tt,r8,lsl#2]'
>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr t0,[tt,t0,lsr#(8*1)-2]'
arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r9,[tt,r9,lsl#2]'
>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr t1,[tt,t1,lsr#(8*1)-2]'
>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr t2,[tt,t2,lsr#(8*2)-2]'
>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r10,[tt,r10,lsr#(8*2)-2]'
arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr t0,[tt,t0,lsr#(8*3)-2]'
>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r11,[tt,r11,lsr#(8*3)-2]'
arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r10,[tt,r10,lsl#2]'
>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr t0,[tt,t0,lsr#(8*1)-2]'
arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r11,[tt,r11,lsl#2]'
>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr t1,[tt,t1,lsr#(8*1)-2]'
>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr t2,[tt,t2,lsr#(8*2)-2]'
>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r5,[tt,r5,lsr#(8*2)-2]'
arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr t0,[tt,t0,lsr#(8*3)-2]'
>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r6,[tt,r6,lsr#(8*3)-2]'
arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r4,[tt,r4,lsl#2]'
>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr t0,[tt,t0,lsr#(8*1)-2]'
arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r5,[tt,r5,lsl#2]'
>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr t1,[tt,t1,lsr#(8*1)-2]'
>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr t2,[tt,t2,lsr#(8*2)-2]'
arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r6,[tt,r6,lsr#(8*2)-2]'
arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr t0,[tt,t0,lsr#(8*3)-2]'
>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r7,[tt,r7,lsr#(8*3)-2]'
arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r6,[tt,r6,lsl#2]'
>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr t0,[tt,t0,lsr#(8*1)-2]'
arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r7,[tt,r7,lsl#2]'
>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr t1,[tt,t1,lsr#(8*1)-2]'
>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr t2,[tt,t2,lsr#(8*2)-2]'
>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r9,[tt,r9,lsr#(8*2)-2]'
arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr t0,[tt,t0,lsr#(8*3)-2]'
arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r10,[tt,r10,lsr#(8*3)-2]'
arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r8,[tt,r8,lsl#2]'
>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr t0,[tt,t0,lsr#(8*1)-2]'
arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r9,[tt,r9,lsl#2]'
vim +174 arch/arm/crypto/aes-cipher-core.S
15 .align 5
16
17 rk .req r0
18 rounds .req r1
19 in .req r2
20 out .req r3
> 21 tt .req ip
22
23 t0 .req lr
24 t1 .req r2
25 t2 .req r3
26
27 .macro __select, out, in, idx
28 .if __LINUX_ARM_ARCH__ < 7
29 and \out, \in, #0xff << (8 * \idx)
30 .else
31 ubfx \out, \in, #(8 * \idx), #8
32 .endif
33 .endm
34
35 .macro __load, out, in, idx
36 .if __LINUX_ARM_ARCH__ < 7 && \idx > 0
37 ldr \out, [tt, \in, lsr #(8 * \idx) - 2]
38 .else
39 ldr \out, [tt, \in, lsl #2]
40 .endif
41 .endm
42
43 .macro __hround, out0, out1, in0, in1, in2, in3, t3, t4, enc
44 __select \out0, \in0, 0
45 __select t0, \in1, 1
46 __load \out0, \out0, 0
47 __load t0, t0, 1
48
49 .if \enc
50 __select \out1, \in1, 0
51 __select t1, \in2, 1
52 .else
53 __select \out1, \in3, 0
54 __select t1, \in0, 1
55 .endif
56 __load \out1, \out1, 0
57 __select t2, \in2, 2
58 __load t1, t1, 1
59 __load t2, t2, 2
60
61 eor \out0, \out0, t0, ror #24
62
63 __select t0, \in3, 3
64 .if \enc
65 __select \t3, \in3, 2
66 __select \t4, \in0, 3
67 .else
68 __select \t3, \in1, 2
69 __select \t4, \in2, 3
70 .endif
71 __load \t3, \t3, 2
72 __load t0, t0, 3
73 __load \t4, \t4, 3
74
75 eor \out1, \out1, t1, ror #24
76 eor \out0, \out0, t2, ror #16
77 ldm rk!, {t1, t2}
78 eor \out1, \out1, \t3, ror #16
79 eor \out0, \out0, t0, ror #8
80 eor \out1, \out1, \t4, ror #8
81 eor \out0, \out0, t1
82 eor \out1, \out1, t2
83 .endm
84
85 .macro fround, out0, out1, out2, out3, in0, in1, in2, in3
86 __hround \out0, \out1, \in0, \in1, \in2, \in3, \out2, \out3, 1
87 __hround \out2, \out3, \in2, \in3, \in0, \in1, \in1, \in2, 1
88 .endm
89
90 .macro iround, out0, out1, out2, out3, in0, in1, in2, in3
91 __hround \out0, \out1, \in0, \in3, \in2, \in1, \out2, \out3, 0
92 __hround \out2, \out3, \in2, \in1, \in0, \in3, \in1, \in0, 0
93 .endm
94
95 .macro __rev, out, in
96 .if __LINUX_ARM_ARCH__ < 6
97 lsl t0, \in, #24
98 and t1, \in, #0xff00
99 and t2, \in, #0xff0000
100 orr \out, t0, \in, lsr #24
101 orr \out, \out, t1, lsl #8
102 orr \out, \out, t2, lsr #8
103 .else
104 rev \out, \in
105 .endif
106 .endm
107
108 .macro __adrl, out, sym, c
109 .if __LINUX_ARM_ARCH__ < 7
110 ldr\c \out, =\sym
111 .else
112 movw\c \out, #:lower16:\sym
113 movt\c \out, #:upper16:\sym
114 .endif
115 .endm
116
117 .macro do_crypt, round, ttab, ltab
118 push {r3-r11, lr}
119
120 ldr r4, [in]
121 ldr r5, [in, #4]
122 ldr r6, [in, #8]
123 ldr r7, [in, #12]
124
125 ldm rk!, {r8-r11}
126
127 #ifdef CONFIG_CPU_BIG_ENDIAN
128 __rev r4, r4
129 __rev r5, r5
130 __rev r6, r6
131 __rev r7, r7
132 #endif
133
134 eor r4, r4, r8
135 eor r5, r5, r9
136 eor r6, r6, r10
137 eor r7, r7, r11
138
139 __adrl tt, \ttab
140
141 tst rounds, #2
142 bne 1f
143
144 0: \round r8, r9, r10, r11, r4, r5, r6, r7
145 \round r4, r5, r6, r7, r8, r9, r10, r11
146
147 1: subs rounds, rounds, #4
148 \round r8, r9, r10, r11, r4, r5, r6, r7
149 __adrl tt, \ltab, ls
150 \round r4, r5, r6, r7, r8, r9, r10, r11
151 bhi 0b
152
153 #ifdef CONFIG_CPU_BIG_ENDIAN
154 __rev r4, r4
155 __rev r5, r5
156 __rev r6, r6
157 __rev r7, r7
158 #endif
159
160 ldr out, [sp]
161
162 str r4, [out]
163 str r5, [out, #4]
164 str r6, [out, #8]
165 str r7, [out, #12]
166
167 pop {r3-r11, pc}
168
169 .align 3
170 .ltorg
171 .endm
172
173 ENTRY(__aes_arm_encrypt)
> 174 do_crypt fround, crypto_ft_tab, crypto_fl_tab
175 ENDPROC(__aes_arm_encrypt)
176
177 ENTRY(__aes_arm_decrypt)
> 178 do_crypt iround, crypto_it_tab, crypto_il_tab
179 ENDPROC(__aes_arm_decrypt)
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 60405 bytes --]
^ permalink raw reply
* Re: x86-64: Maintain 16-byte stack alignment
From: Linus Torvalds @ 2017-01-12 21:40 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Andy Lutomirski, Herbert Xu, Linux Kernel Mailing List,
Linux Crypto Mailing List, Ingo Molnar, Thomas Gleixner,
Andy Lutomirski, Ard Biesheuvel
In-Reply-To: <20170112205504.gb6z2w52mektyc73@treble>
On Thu, Jan 12, 2017 at 12:55 PM, Josh Poimboeuf <jpoimboe@redhat.com> 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
^ permalink raw reply
* Re: x86-64: Maintain 16-byte stack alignment
From: Josh Poimboeuf @ 2017-01-12 20:55 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Linus Torvalds, Herbert Xu, Linux Kernel Mailing List,
Linux Crypto Mailing List, Ingo Molnar, Thomas Gleixner,
Andy Lutomirski, Ard Biesheuvel
In-Reply-To: <20170112201511.yj5ekqmj76r2yv6t@treble>
On Thu, Jan 12, 2017 at 02:15:11PM -0600, Josh Poimboeuf wrote:
> On Thu, Jan 12, 2017 at 12:08:07PM -0800, Andy Lutomirski wrote:
> > On Thu, Jan 12, 2017 at 11:51 AM, Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > > On Thu, Jan 12, 2017 at 6:02 AM, Josh Poimboeuf <jpoimboe@redhat.com> 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).
> >
> > What I meant was checking the entry code to see if it aligns stack
> > frames, and good luck getting sparse to do that. Hmm, getting 16-byte
> > alignment for real may actually be entirely a lost cause. After all,
> > I think we have some inline functions that do asm volatile ("call
> > ..."), and I don't see any credible way of forcing alignment short of
> > generating an entirely new stack frame and aligning that.
>
> Actually we already found all such cases and fixed them by forcing a new
> stack frame, thanks to objtool. For example, see 55a76b59b5fe.
>
> > Ick. This
> > whole situation stinks, and I wish that the gcc developers had been
> > less daft here in the first place or that we'd noticed and gotten it
> > fixed much longer ago.
> >
> > Can we come up with a macro like STACK_ALIGN_16 that turns into
> > __aligned__(32) on bad gcc versions and combine that with your sparse
> > patch?
This could work. Only concerns I'd have are:
- Are there (or will there be in the future) any asm functions which
assume a 16-byte aligned stack? (Seems unlikely. Stack alignment is
common in the crypto code but they do the alignment manually.)
- Who's going to run sparse all the time to catch unauthorized users of
__aligned__(16)?
--
Josh
^ permalink raw reply
* Re: [cryptodev:master 43/44] arch/arm/crypto/aes-cipher-core.S:21: Error: selected processor does not support `tt .req ip' in ARM mode
From: Ard Biesheuvel @ 2017-01-12 20:44 UTC (permalink / raw)
To: kbuild test robot; +Cc: kbuild-all, linux-crypto@vger.kernel.org, Herbert Xu
In-Reply-To: <201701130326.19FpToYy%fengguang.wu@intel.com>
Hi Arnd,
On 12 January 2017 at 19:04, kbuild test robot <fengguang.wu@intel.com> wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
> head: 1abee99eafab67fb1c98f9ecfc43cd5735384a86
> commit: 81edb42629758bacdf813dd5e4542ae26e3ad73a [43/44] crypto: arm/aes - replace scalar AES cipher
> config: arm-multi_v7_defconfig (attached as .config)
> compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
> wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> git checkout 81edb42629758bacdf813dd5e4542ae26e3ad73a
> # save the attached .config to linux build tree
> make.cross ARCH=arm
>
> All errors (new ones prefixed by >>):
>
> arch/arm/crypto/aes-cipher-core.S: Assembler messages:
>>> arch/arm/crypto/aes-cipher-core.S:21: Error: selected processor does not support `tt .req ip' in ARM mode
Did you ever see this error? This is very odd: .req simply declares an
alias for a register name, and this works fine locally
>>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `movw tt,#:lower16:crypto_ft_tab'
>>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `movt tt,#:upper16:crypto_ft_tab'
>>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r8,[tt,r8,lsl#2]'
>>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr t0,[tt,t0,lsl#2]'
>>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r9,[tt,r9,lsl#2]'
>>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr t1,[tt,t1,lsl#2]'
>>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr t2,[tt,t2,lsl#2]'
>>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r10,[tt,r10,lsl#2]'
>>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr t0,[tt,t0,lsl#2]'
>>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r11,[tt,r11,lsl#2]'
>>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r10,[tt,r10,lsl#2]'
>>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr t0,[tt,t0,lsl#2]'
>>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r11,[tt,r11,lsl#2]'
>>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr t1,[tt,t1,lsl#2]'
>>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr t2,[tt,t2,lsl#2]'
>>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r5,[tt,r5,lsl#2]'
>>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr t0,[tt,t0,lsl#2]'
>>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r6,[tt,r6,lsl#2]'
>>> arch/arm/crypto/aes-cipher-core.S:174: Error: ARM register expected -- `ldr r4,[tt,r4,lsl#2]'
>
> vim +21 arch/arm/crypto/aes-cipher-core.S
>
> 15 .align 5
> 16
> 17 rk .req r0
> 18 rounds .req r1
> 19 in .req r2
> 20 out .req r3
> > 21 tt .req ip
> 22
> 23 t0 .req lr
> 24 t1 .req r2
> 25 t2 .req r3
> 26
> 27 .macro __select, out, in, idx
> 28 .if __LINUX_ARM_ARCH__ < 7
> 29 and \out, \in, #0xff << (8 * \idx)
> 30 .else
> 31 ubfx \out, \in, #(8 * \idx), #8
> 32 .endif
> 33 .endm
> 34
> 35 .macro __load, out, in, idx
> 36 .if __LINUX_ARM_ARCH__ < 7 && \idx > 0
> 37 ldr \out, [tt, \in, lsr #(8 * \idx) - 2]
> 38 .else
> 39 ldr \out, [tt, \in, lsl #2]
> 40 .endif
> 41 .endm
> 42
> 43 .macro __hround, out0, out1, in0, in1, in2, in3, t3, t4, enc
> 44 __select \out0, \in0, 0
> 45 __select t0, \in1, 1
> 46 __load \out0, \out0, 0
> 47 __load t0, t0, 1
> 48
> 49 .if \enc
> 50 __select \out1, \in1, 0
> 51 __select t1, \in2, 1
> 52 .else
> 53 __select \out1, \in3, 0
> 54 __select t1, \in0, 1
> 55 .endif
> 56 __load \out1, \out1, 0
> 57 __select t2, \in2, 2
> 58 __load t1, t1, 1
> 59 __load t2, t2, 2
> 60
> 61 eor \out0, \out0, t0, ror #24
> 62
> 63 __select t0, \in3, 3
> 64 .if \enc
> 65 __select \t3, \in3, 2
> 66 __select \t4, \in0, 3
> 67 .else
> 68 __select \t3, \in1, 2
> 69 __select \t4, \in2, 3
> 70 .endif
> 71 __load \t3, \t3, 2
> 72 __load t0, t0, 3
> 73 __load \t4, \t4, 3
> 74
> 75 eor \out1, \out1, t1, ror #24
> 76 eor \out0, \out0, t2, ror #16
> 77 ldm rk!, {t1, t2}
> 78 eor \out1, \out1, \t3, ror #16
> 79 eor \out0, \out0, t0, ror #8
> 80 eor \out1, \out1, \t4, ror #8
> 81 eor \out0, \out0, t1
> 82 eor \out1, \out1, t2
> 83 .endm
> 84
> 85 .macro fround, out0, out1, out2, out3, in0, in1, in2, in3
> 86 __hround \out0, \out1, \in0, \in1, \in2, \in3, \out2, \out3, 1
> 87 __hround \out2, \out3, \in2, \in3, \in0, \in1, \in1, \in2, 1
> 88 .endm
> 89
> 90 .macro iround, out0, out1, out2, out3, in0, in1, in2, in3
> 91 __hround \out0, \out1, \in0, \in3, \in2, \in1, \out2, \out3, 0
> 92 __hround \out2, \out3, \in2, \in1, \in0, \in3, \in1, \in0, 0
> 93 .endm
> 94
> 95 .macro __rev, out, in
> 96 .if __LINUX_ARM_ARCH__ < 6
> 97 lsl t0, \in, #24
> 98 and t1, \in, #0xff00
> 99 and t2, \in, #0xff0000
> 100 orr \out, t0, \in, lsr #24
> 101 orr \out, \out, t1, lsl #8
> 102 orr \out, \out, t2, lsr #8
> 103 .else
> 104 rev \out, \in
> 105 .endif
> 106 .endm
> 107
> 108 .macro __adrl, out, sym, c
> 109 .if __LINUX_ARM_ARCH__ < 7
> 110 ldr\c \out, =\sym
> 111 .else
> 112 movw\c \out, #:lower16:\sym
> 113 movt\c \out, #:upper16:\sym
> 114 .endif
> 115 .endm
> 116
> 117 .macro do_crypt, round, ttab, ltab
> 118 push {r3-r11, lr}
> 119
> 120 ldr r4, [in]
> 121 ldr r5, [in, #4]
> 122 ldr r6, [in, #8]
> 123 ldr r7, [in, #12]
> 124
> 125 ldm rk!, {r8-r11}
> 126
> 127 #ifdef CONFIG_CPU_BIG_ENDIAN
> 128 __rev r4, r4
> 129 __rev r5, r5
> 130 __rev r6, r6
> 131 __rev r7, r7
> 132 #endif
> 133
> 134 eor r4, r4, r8
> 135 eor r5, r5, r9
> 136 eor r6, r6, r10
> 137 eor r7, r7, r11
> 138
> 139 __adrl tt, \ttab
> 140
> 141 tst rounds, #2
> 142 bne 1f
> 143
> 144 0: \round r8, r9, r10, r11, r4, r5, r6, r7
> 145 \round r4, r5, r6, r7, r8, r9, r10, r11
> 146
> 147 1: subs rounds, rounds, #4
> 148 \round r8, r9, r10, r11, r4, r5, r6, r7
> 149 __adrl tt, \ltab, ls
> 150 \round r4, r5, r6, r7, r8, r9, r10, r11
> 151 bhi 0b
> 152
> 153 #ifdef CONFIG_CPU_BIG_ENDIAN
> 154 __rev r4, r4
> 155 __rev r5, r5
> 156 __rev r6, r6
> 157 __rev r7, r7
> 158 #endif
> 159
> 160 ldr out, [sp]
> 161
> 162 str r4, [out]
> 163 str r5, [out, #4]
> 164 str r6, [out, #8]
> 165 str r7, [out, #12]
> 166
> 167 pop {r3-r11, pc}
> 168
> 169 .align 3
> 170 .ltorg
> 171 .endm
> 172
> 173 ENTRY(__aes_arm_encrypt)
> > 174 do_crypt fround, crypto_ft_tab, crypto_fl_tab
> 175 ENDPROC(__aes_arm_encrypt)
> 176
> 177 ENTRY(__aes_arm_decrypt)
> > 178 do_crypt iround, crypto_it_tab, crypto_il_tab
> 179 ENDPROC(__aes_arm_decrypt)
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply
* Re: x86-64: Maintain 16-byte stack alignment
From: Josh Poimboeuf @ 2017-01-12 20:15 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Linus Torvalds, Herbert Xu, Linux Kernel Mailing List,
Linux Crypto Mailing List, Ingo Molnar, Thomas Gleixner,
Andy Lutomirski, Ard Biesheuvel
In-Reply-To: <CALCETrVz-wEFVUwrpS8-Ln9SWnsF5KxkqJC-Br6wJ+e0LGM9UA@mail.gmail.com>
On Thu, Jan 12, 2017 at 12:08:07PM -0800, Andy Lutomirski wrote:
> On Thu, Jan 12, 2017 at 11:51 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > On Thu, Jan 12, 2017 at 6:02 AM, Josh Poimboeuf <jpoimboe@redhat.com> 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).
>
> What I meant was checking the entry code to see if it aligns stack
> frames, and good luck getting sparse to do that. Hmm, getting 16-byte
> alignment for real may actually be entirely a lost cause. After all,
> I think we have some inline functions that do asm volatile ("call
> ..."), and I don't see any credible way of forcing alignment short of
> generating an entirely new stack frame and aligning that.
Actually we already found all such cases and fixed them by forcing a new
stack frame, thanks to objtool. For example, see 55a76b59b5fe.
> Ick. This
> whole situation stinks, and I wish that the gcc developers had been
> less daft here in the first place or that we'd noticed and gotten it
> fixed much longer ago.
>
> Can we come up with a macro like STACK_ALIGN_16 that turns into
> __aligned__(32) on bad gcc versions and combine that with your sparse
> patch?
--
Josh
^ permalink raw reply
* Re: x86-64: Maintain 16-byte stack alignment
From: Andy Lutomirski @ 2017-01-12 20:08 UTC (permalink / raw)
To: Linus Torvalds
Cc: Josh Poimboeuf, Herbert Xu, Linux Kernel Mailing List,
Linux Crypto Mailing List, Ingo Molnar, Thomas Gleixner,
Andy Lutomirski, Ard Biesheuvel
In-Reply-To: <CA+55aFxP+V6Wbq5Xw_NOksiWouEMg4gjBJgeGa-qFyxDMnTmcA@mail.gmail.com>
On Thu, Jan 12, 2017 at 11:51 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Jan 12, 2017 at 6:02 AM, Josh Poimboeuf <jpoimboe@redhat.com> 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).
What I meant was checking the entry code to see if it aligns stack
frames, and good luck getting sparse to do that. Hmm, getting 16-byte
alignment for real may actually be entirely a lost cause. After all,
I think we have some inline functions that do asm volatile ("call
..."), and I don't see any credible way of forcing alignment short of
generating an entirely new stack frame and aligning that. Ick. This
whole situation stinks, and I wish that the gcc developers had been
less daft here in the first place or that we'd noticed and gotten it
fixed much longer ago.
Can we come up with a macro like STACK_ALIGN_16 that turns into
__aligned__(32) on bad gcc versions and combine that with your sparse
patch?
^ permalink raw reply
* Re: x86-64: Maintain 16-byte stack alignment
From: Linus Torvalds @ 2017-01-12 19:51 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Andy Lutomirski, Herbert Xu, Linux Kernel Mailing List,
Linux Crypto Mailing List, Ingo Molnar, Thomas Gleixner,
Andy Lutomirski, Ard Biesheuvel
In-Reply-To: <20170112140215.rh247gwk55fjzmg7@treble>
[-- Attachment #1: Type: text/plain, Size: 3363 bytes --]
On Thu, Jan 12, 2017 at 6:02 AM, Josh Poimboeuf <jpoimboe@redhat.com> 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
[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 887 bytes --]
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)
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox