Re: Kernel oops with 2.6.26, padlock and ipsec: probably problem with fpu state changes

Suresh Siddha
2008-08-08T18:54:18Z

On Fri, Aug 08, 2008 at 03:36:54AM -0700, Wolfgang Walter wrote:
> Am Donnerstag, 7. August 2008 18:23 schrieb Wolfgang Walter:
> > But yesterday I tried the following patch on top of a vanilla 2.6.26:
> >
> > =======================================================
> > diff -ur ../linux-2.6.26/drivers/crypto/padlock-aes.c
> > ./drivers/crypto/padlock-aes.c ---
> > ../linux-2.6.26/drivers/crypto/padlock-aes.c 2008-07-15 11:29:32.000000000
> > +0200 +++ ./drivers/crypto/padlock-aes.c 2008-08-07 17:46:55.000000000
> > +0200 @@ -16,6 +16,7 @@
> > #include <linux/interrupt.h>
> > #include <linux/kernel.h>
> > #include <asm/byteorder.h>
> > +#include <asm/i387.h>
> > #include "padlock.h"
> >
> > /* Control word. */
> > @@ -144,9 +145,11 @@
> > static inline void padlock_xcrypt(const u8 *input, u8 *output, void *key,
> > void *control_word)
> > {
> > + kernel_fpu_begin();
> > asm volatile (".byte 0xf3,0x0f,0xa7,0xc8" /* rep xcryptecb */
> >
> > : "+S"(input), "+D"(output)
> > : "d"(control_word), "b"(key), "c"(1));
> >
> > + kernel_fpu_end();
> > }
> >
> > static void aes_crypt_copy(const u8 *in, u8 *out, u32 *key, struct cword
> > *cword) @@ -179,6 +182,7 @@
> > return;
> > }
> >
> > + kernel_fpu_begin();
> > asm volatile ("test $1, %%cl;"
> > "je 1f;"
> > "lea -1(%%ecx), %%eax;"
> > @@ -190,15 +194,18 @@
> >
> > : "+S"(input), "+D"(output)
> > : "d"(control_word), "b"(key), "c"(count)
> > : "ax");
> >
> > + kernel_fpu_end();
> > }
> >
> > static inline u8 *padlock_xcrypt_cbc(const u8 *input, u8 *output, void
> > *key, u8 *iv, void *control_word, u32 count)
> > {
> > /* rep xcryptcbc */
> > + kernel_fpu_begin();
> > asm volatile (".byte 0xf3,0x0f,0xa7,0xd0"
> >
> > : "+S" (input), "+D" (output), "+a" (iv)
> > : "d" (control_word), "b" (key), "c" (count));
> >
> > + kernel_fpu_end();
> > return iv;
> > }
> >
> > =============================================================
> >
> > I found that kernel_fpu_begin(); kernel_fpu_begin(); is used with
> > MMX and/or SSE:
> >
> > include/asm/xor_32.h
> > drivers/md/raid6mmx.c
> > drivers/md/raid6sse1.c
> > drivers/md/raid6sse2.c
> >
> >
> > With this change I its a little bit more stable, I needed more then 5
> > minutes to crash the kernel (repeated it several times). If I read
> > the code correctly this disables preemption for the time the padlock cmd
> > is executing.
>
> Forget that - I booted the wrong kernel.
>
> I now really test this modification and it seems to be stable. The router now
> runs for 45 minutes without oops.

Ok. Thanks. I think I can explain the failure:

These padlock instructions though don't use/touch SSE registers, but it behaves
similar to other SSE instructions. For example, it might cause DNA faults
when cr0.ts is set. While this is a spurious DNA trap, it might cause
problems with the recent fpu code changes.

This is the code sequence that is probably causing this problem:

a) new app is getting exec'd and it is somewhere in between
start_thread() and flush_old_exec() in the load_xyz_binary()

b) At pont "a", task's fpu state (like TS_USEDFPU, used_math() etc) is
cleared.

c) Now we get an interrupt/softirq which starts using these encrypt/decrypt
routines in the network stack. This generates a math fault (as
cr0.ts is '1') which sets TS_USEDFPU and restores the math that is
in the task's xstate.

d) Return to exec code path, which does start_thread() which does
free_thread_xstate() and sets xstate pointer to NULL while
the TS_USEDFPU is still set.

e) At the next context switch from the new exec'd task to another task,
we have a scenarios where TS_USEDFPU is set but xstate pointer is null.
This can cause an oops during unlazy_fpu() in __switch_to()

Now:

1) This should happen with or with out pre-emption. Viro also encountered
similar problem with out CONFIG_PREEMPT.

2) kernel_fpu_begin() and kernel_fpu_end() will fix this problem, because
kernel_fpu_begin() will manually do a clts() and won't run in to the
situation of setting TS_USEDFPU in step "c" above.

3) This was working before the fpu changes, because its a spurious
math fault which doesn't corrupt any fpu/sse registers and the task's
math state was always in an allocated state.

I propose to go with wolf's patch which add's kernel_fpu_begin() and
kernel_fpu_end() around these instructions. This is the correct fix
(unless there is something wrong in my above understanding).

thanks,
suresh