Re: WARNING: CPU: 0 PID: 3031 at ./arch/x86/include/asm/fpu/internal.h:530 fpu__restore+0x90/0x130()

[Borislav Petkov]

On Thu, Feb 11, 2016 at 05:16:00PM -0800, Andy Lutomirski wrote:
> Are you running 32-bit userspace by any chance?

Sure, that's a 32-bit kernel testing partition. :)

> I'm guessing you're hitting this in __fpu_restore_sig:

Yeah, I was looking at that too.

>         fpu__drop(fpu);
>         if (__copy_from_user(&fpu->state.xsave, buf_fx, state_size) ||
>             __copy_from_user(&env, buf, sizeof(env))) {
>             fpstate_init(&fpu->state);
>             err = -1;
>         } else {
>             sanitize_restored_xstate(tsk, &env, xfeatures, fx_only);
>         }
> 
>         fpu->fpstate_active = 1;
> 
> <-- preempted right here

Yeah, that could explain why I'm seeing it.

>         if (use_eager_fpu()) {
>             preempt_disable();
>             fpu__restore(fpu);
>             preempt_enable();
>         }
> 
> I don't see why this code deserves to work.  If I'm right, it can be
> fixed by pulling the preempt_disable out of the if (use_eager_fpu())
> to right above the fpstate_active = 1 line.  Don't bother trying to
> optimize the !use_eager_fpu() case.

Right.

> Once someone gets around to eagerly *allocating* the FPU context and
> dropping CR0.TS usage entirely, then even that won't be enough unless
> we do my suggesting of deferring FPU restore to
> prepare_exit_to_usermode.  (Doing that will make all of this much,
> much more sane.)

Sounds good to me.

So the thing with this issue is, is that I don't have a reproducer yet.
It happened randomly.

So let me ask it this way: can anything go wrong if we pull up the
preemption disabled region? I mean, do we even care about the !eager FPU
case? I'd much prefer that vs FPU state corruption...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.