From 47f01e8cc23f3d041f6b9fb97627369eaf75ba7f Mon Sep 17 00:00:00 2001 From: Ingo Molnar Date: Wed, 27 May 2015 12:22:29 +0200 Subject: [PATCH] x86/fpu: Fix FPU register read access to the current task Bobby Powers reported the following FPU warning during ELF coredumping: WARNING: CPU: 0 PID: 27452 at arch/x86/kernel/fpu/core.c:324 fpu__activate_stopped+0x8a/0xa0() This warning unearthed an invalid assumption about fpu__activate_stopped() that I added in: 67e97fc2ec57 ("x86/fpu: Rename init_fpu() to fpu__unlazy_stopped() and add debugging check") the old init_fpu() function had an (intentional but obscure) side effect: when FPU registers are accessed for the current task, for reading, then it synchronized live in-register FPU state with the fpstate by saving it. So fix this bug by saving the FPU if we are the current task. We'll still warn in fpu__save() if this is called for not yet stopped child tasks, so the debugging check is still preserved. Also rename the function to fpu__activate_fpstate(), because it's not exclusively used for stopped tasks, but for the current task as well. ( Note that this bug calls for a cleaner separation of access-for-read and access-for-modification FPU methods, but we'll do that in separate patches. ) Reported-by: Bobby Powers Cc: Andy Lutomirski Cc: Borislav Petkov Cc: Dave Hansen Cc: Fenghua Yu Cc: H. Peter Anvin Cc: Linus Torvalds Cc: Oleg Nesterov Cc: Peter Zijlstra Cc: Thomas Gleixner Signed-off-by: Ingo Molnar --- arch/x86/include/asm/fpu/internal.h | 2 +- arch/x86/kernel/fpu/core.c | 43 +++++++++++++++++++++---------------- arch/x86/kernel/fpu/regset.c | 12 +++++------ 3 files changed, 32 insertions(+), 25 deletions(-) diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h index 99690be..62d13d5 100644 --- a/arch/x86/include/asm/fpu/internal.h +++ b/arch/x86/include/asm/fpu/internal.h @@ -22,7 +22,7 @@ * High level FPU state handling functions: */ extern void fpu__activate_curr(struct fpu *fpu); -extern void fpu__activate_stopped(struct fpu *fpu); +extern void fpu__activate_fpstate(struct fpu *fpu); extern void fpu__save(struct fpu *fpu); extern void fpu__restore(struct fpu *fpu); extern int fpu__restore_sig(void __user *buf, int ia32_frame); diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c index 01a1550..b410492 100644 --- a/arch/x86/kernel/fpu/core.c +++ b/arch/x86/kernel/fpu/core.c @@ -296,40 +296,47 @@ void fpu__activate_curr(struct fpu *fpu) EXPORT_SYMBOL_GPL(fpu__activate_curr); /* - * This function must be called before we modify a stopped child's - * fpstate. + * This function must be called before we read or write a task's fpstate. * - * If the child has not used the FPU before then initialize its + * If the task has not used the FPU before then initialize its * fpstate. * - * If the child has used the FPU before then unlazy it. + * If the task has used the FPU before then save and unlazy it. * - * [ After this function call, after registers in the fpstate are + * [ If this function is used for non-current child tasks, then + * after this function call, after registers in the fpstate are * modified and the child task has woken up, the child task will * restore the modified FPU state from the modified context. If we * didn't clear its lazy status here then the lazy in-registers * state pending on its former CPU could be restored, corrupting - * the modifications. ] + * the modifications. * - * This function is also called before we read a stopped child's - * FPU state - to make sure it's initialized if the child has - * no active FPU state. + * This function can be used for the current task as well, but + * only for reading the fpstate. Modifications to the fpstate + * will be lost on eagerfpu systems. ] * * TODO: A future optimization would be to skip the unlazying in * the read-only case, it's not strictly necessary for * read-only access to the context. */ -void fpu__activate_stopped(struct fpu *child_fpu) +void fpu__activate_fpstate(struct fpu *fpu) { - WARN_ON_FPU(child_fpu == ¤t->thread.fpu); - - if (child_fpu->fpstate_active) { - child_fpu->last_cpu = -1; + /* + * If fpregs are active (in the current CPU), then + * copy them to the fpstate: + */ + if (fpu->fpregs_active) { + fpu__save(fpu); } else { - fpstate_init(&child_fpu->state); - - /* Safe to do for stopped child tasks: */ - child_fpu->fpstate_active = 1; + if (fpu->fpstate_active) { + /* Invalidate any lazy state: */ + fpu->last_cpu = -1; + } else { + fpstate_init(&fpu->state); + + /* Safe to do for current and for stopped child tasks: */ + fpu->fpstate_active = 1; + } } } diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c index 297b3da..a1f97d9 100644 --- a/arch/x86/kernel/fpu/regset.c +++ b/arch/x86/kernel/fpu/regset.c @@ -33,7 +33,7 @@ int xfpregs_get(struct task_struct *target, const struct user_regset *regset, if (!cpu_has_fxsr) return -ENODEV; - fpu__activate_stopped(fpu); + fpu__activate_fpstate(fpu); fpstate_sanitize_xstate(fpu); return user_regset_copyout(&pos, &count, &kbuf, &ubuf, @@ -50,7 +50,7 @@ int xfpregs_set(struct task_struct *target, const struct user_regset *regset, if (!cpu_has_fxsr) return -ENODEV; - fpu__activate_stopped(fpu); + fpu__activate_fpstate(fpu); fpstate_sanitize_xstate(fpu); ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, @@ -82,7 +82,7 @@ int xstateregs_get(struct task_struct *target, const struct user_regset *regset, if (!cpu_has_xsave) return -ENODEV; - fpu__activate_stopped(fpu); + fpu__activate_fpstate(fpu); xsave = &fpu->state.xsave; @@ -111,7 +111,7 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset, if (!cpu_has_xsave) return -ENODEV; - fpu__activate_stopped(fpu); + fpu__activate_fpstate(fpu); xsave = &fpu->state.xsave; @@ -273,7 +273,7 @@ int fpregs_get(struct task_struct *target, const struct user_regset *regset, struct fpu *fpu = &target->thread.fpu; struct user_i387_ia32_struct env; - fpu__activate_stopped(fpu); + fpu__activate_fpstate(fpu); if (!static_cpu_has(X86_FEATURE_FPU)) return fpregs_soft_get(target, regset, pos, count, kbuf, ubuf); @@ -303,7 +303,7 @@ int fpregs_set(struct task_struct *target, const struct user_regset *regset, struct user_i387_ia32_struct env; int ret; - fpu__activate_stopped(fpu); + fpu__activate_fpstate(fpu); fpstate_sanitize_xstate(fpu); if (!static_cpu_has(X86_FEATURE_FPU)) -- 2.7.4