* [PATCH 0/2] MIPS: Fix FPU preemption issues @ 2016-02-01 13:50 ` James Hogan 0 siblings, 0 replies; 7+ messages in thread From: James Hogan @ 2016-02-01 13:50 UTC (permalink / raw) To: Ralf Baechle; +Cc: linux-mips, Paul Burton, James Hogan These patches aim to prevent the FPU being left enabled across a task context switch due to full kernel preemption. This could result in returning to a KVM guest with FPU enabled. Patch 1 fixes a more theoretical case that I spotted where TIF_USEDFPU is cleared without disabling the FPU in the execve path. Patch 2 fixes a much easier to hit case (multiple WARNs before reaching login prompt if only the WARN part of patch is applied) due to saved Status in interrupt context not being updated when FPU is disabled. In doing so it also allows an orphaned enabled FPU to remain enabled through to user mode, hence the new WARN on context switch to catch future cases of it. James Hogan (2): MIPS: Properly disable FPU in start_thread() MIPS: Fix FPU disable with preemption arch/mips/include/asm/fpu.h | 4 ++++ arch/mips/include/asm/stackframe.h | 4 ++-- arch/mips/kernel/process.c | 6 ++---- 3 files changed, 8 insertions(+), 6 deletions(-) -- 2.4.10 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 0/2] MIPS: Fix FPU preemption issues @ 2016-02-01 13:50 ` James Hogan 0 siblings, 0 replies; 7+ messages in thread From: James Hogan @ 2016-02-01 13:50 UTC (permalink / raw) To: Ralf Baechle; +Cc: linux-mips, Paul Burton, James Hogan These patches aim to prevent the FPU being left enabled across a task context switch due to full kernel preemption. This could result in returning to a KVM guest with FPU enabled. Patch 1 fixes a more theoretical case that I spotted where TIF_USEDFPU is cleared without disabling the FPU in the execve path. Patch 2 fixes a much easier to hit case (multiple WARNs before reaching login prompt if only the WARN part of patch is applied) due to saved Status in interrupt context not being updated when FPU is disabled. In doing so it also allows an orphaned enabled FPU to remain enabled through to user mode, hence the new WARN on context switch to catch future cases of it. James Hogan (2): MIPS: Properly disable FPU in start_thread() MIPS: Fix FPU disable with preemption arch/mips/include/asm/fpu.h | 4 ++++ arch/mips/include/asm/stackframe.h | 4 ++-- arch/mips/kernel/process.c | 6 ++---- 3 files changed, 8 insertions(+), 6 deletions(-) -- 2.4.10 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] MIPS: Properly disable FPU in start_thread() @ 2016-02-01 13:50 ` James Hogan 0 siblings, 0 replies; 7+ messages in thread From: James Hogan @ 2016-02-01 13:50 UTC (permalink / raw) To: Ralf Baechle; +Cc: linux-mips, Paul Burton, James Hogan start_thread() (called for execve(2)) clears the TIF_USEDFPU flag without atomically disabling the FPU. With a preemptive kernel, an unfortunately timed preemption after this could result in another task (or KVM guest) being scheduled in with the FPU still enabled, since lose_fpu_inatomic() only turns it off if TIF_USEDFPU is set. Use lose_fpu(0) instead of the separate FPU / MSA management, which should do the right thing (drop FPU properly and atomically without saving state) and will be more future proof. Signed-off-by: James Hogan <james.hogan@imgtec.com> Reviewed-by: Paul Burton <paul.burton@imgtec.com> Cc: Ralf Baechle <ralf@linux-mips.org> Cc: linux-mips@linux-mips.org --- arch/mips/kernel/process.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c index f2975d4d1e44..eddd5fd6fdfa 100644 --- a/arch/mips/kernel/process.c +++ b/arch/mips/kernel/process.c @@ -65,12 +65,10 @@ void start_thread(struct pt_regs * regs, unsigned long pc, unsigned long sp) status = regs->cp0_status & ~(ST0_CU0|ST0_CU1|ST0_FR|KU_MASK); status |= KU_USER; regs->cp0_status = status; + lose_fpu(0); + clear_thread_flag(TIF_MSA_CTX_LIVE); clear_used_math(); - clear_fpu_owner(); init_dsp(); - clear_thread_flag(TIF_USEDMSA); - clear_thread_flag(TIF_MSA_CTX_LIVE); - disable_msa(); regs->cp0_epc = pc; regs->regs[29] = sp; } -- 2.4.10 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 1/2] MIPS: Properly disable FPU in start_thread() @ 2016-02-01 13:50 ` James Hogan 0 siblings, 0 replies; 7+ messages in thread From: James Hogan @ 2016-02-01 13:50 UTC (permalink / raw) To: Ralf Baechle; +Cc: linux-mips, Paul Burton, James Hogan start_thread() (called for execve(2)) clears the TIF_USEDFPU flag without atomically disabling the FPU. With a preemptive kernel, an unfortunately timed preemption after this could result in another task (or KVM guest) being scheduled in with the FPU still enabled, since lose_fpu_inatomic() only turns it off if TIF_USEDFPU is set. Use lose_fpu(0) instead of the separate FPU / MSA management, which should do the right thing (drop FPU properly and atomically without saving state) and will be more future proof. Signed-off-by: James Hogan <james.hogan@imgtec.com> Reviewed-by: Paul Burton <paul.burton@imgtec.com> Cc: Ralf Baechle <ralf@linux-mips.org> Cc: linux-mips@linux-mips.org --- arch/mips/kernel/process.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c index f2975d4d1e44..eddd5fd6fdfa 100644 --- a/arch/mips/kernel/process.c +++ b/arch/mips/kernel/process.c @@ -65,12 +65,10 @@ void start_thread(struct pt_regs * regs, unsigned long pc, unsigned long sp) status = regs->cp0_status & ~(ST0_CU0|ST0_CU1|ST0_FR|KU_MASK); status |= KU_USER; regs->cp0_status = status; + lose_fpu(0); + clear_thread_flag(TIF_MSA_CTX_LIVE); clear_used_math(); - clear_fpu_owner(); init_dsp(); - clear_thread_flag(TIF_USEDMSA); - clear_thread_flag(TIF_MSA_CTX_LIVE); - disable_msa(); regs->cp0_epc = pc; regs->regs[29] = sp; } -- 2.4.10 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] MIPS: Fix FPU disable with preemption @ 2016-02-01 13:50 ` James Hogan 0 siblings, 0 replies; 7+ messages in thread From: James Hogan @ 2016-02-01 13:50 UTC (permalink / raw) To: Ralf Baechle; +Cc: linux-mips, Paul Burton, James Hogan The FPU should not be left enabled after a task context switch. This isn't usually a problem as the FPU enable bit is updated before returning to userland, however it can potentially mask kernel bugs, and in fact KVM assumes it won't happen and won't clear the FPU enable bit before returning to the guest, which allows the guest to use stale FPU context. Interrupts and exceptions save and restore most bits of the CP0 Status register which contains the FPU enable bit (CU1). When the kernel needs to enable or disable the FPU (for example due to attempted FPU use by userland, or the scheduler being invoked) both the actual Status register and the saved value in the userland context are updated. However this doesn't work correctly with full kernel preemption enabled, since the FPU enable bit can be cleared from within an interrupt when the scheduler is invoked, and only the userland context is updated, not the interrupt context. For example: 1) Enter kernel with FPU already enabled, TIF_USEDFPU=1, Status.CU1=1 saved. 2) Take a timer interrupt while in kernel mode, Status.CU1=1 saved. 3) Timer interrupt invokes scheduler to preempt the task, which clears TIF_USEDFPU, disables the FPU in Status register (Status.CU1=0), and the value stored in user context from step (1), but not the interrupt context from step (2). 4) When the process is scheduled back in again Status.CU1=0. 5) The interrupt context from step (2) is restored, which sets Status.CU1=1. So from user context point of view, preemption has re-enabled FPU! 6) If the scheduler is invoked again (via preemption or voluntarily) before returning to userland, TIF_USEDFPU=0 so the FPU is not disabled before the task context switch. 7) The next task resumes from the context switch with FPU enabled! The restoring of the Status register on return from interrupt/exception is already selective about which bits to restore, leaving the interrupt mask bits alone so enabling/disabling of CPU interrupt lines can persist. Extend this to also leave both the CU1 bit (FPU enable) and the FR bit (which specifies the FPU mode and gets changed with CU1). This prevents a stale Status value being restored in step (5) above and persisting through subsequent context switches. Also switch to the use of definitions from asm/mipsregs.h while we're at it. Since this change also affects the restoration of Status register on the path back to userland, it increases the sensitivity of the kernel to the problem of the FPU being left enabled, allowing it to propagate to userland, therefore a warning is also added to lose_fpu_inatomic() to point out any future reoccurances before they do any damage. Signed-off-by: James Hogan <james.hogan@imgtec.com> Reviewed-by: Paul Burton <paul.burton@imgtec.com> Cc: Ralf Baechle <ralf@linux-mips.org> Cc: linux-mips@linux-mips.org --- arch/mips/include/asm/fpu.h | 4 ++++ arch/mips/include/asm/stackframe.h | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/arch/mips/include/asm/fpu.h b/arch/mips/include/asm/fpu.h index 9cbf383b8834..f06f97bd62df 100644 --- a/arch/mips/include/asm/fpu.h +++ b/arch/mips/include/asm/fpu.h @@ -179,6 +179,10 @@ static inline void lose_fpu_inatomic(int save, struct task_struct *tsk) if (save) _save_fp(tsk); __disable_fpu(); + } else { + /* FPU should not have been left enabled with no owner */ + WARN(read_c0_status() & ST0_CU1, + "Orphaned FPU left enabled"); } KSTK_STATUS(tsk) &= ~ST0_CU1; clear_tsk_thread_flag(tsk, TIF_USEDFPU); diff --git a/arch/mips/include/asm/stackframe.h b/arch/mips/include/asm/stackframe.h index a71da576883c..eebf39549606 100644 --- a/arch/mips/include/asm/stackframe.h +++ b/arch/mips/include/asm/stackframe.h @@ -289,7 +289,7 @@ .set reorder .set noat mfc0 a0, CP0_STATUS - li v1, 0xff00 + li v1, ST0_CU1 | ST0_IM ori a0, STATMASK xori a0, STATMASK mtc0 a0, CP0_STATUS @@ -330,7 +330,7 @@ ori a0, STATMASK xori a0, STATMASK mtc0 a0, CP0_STATUS - li v1, 0xff00 + li v1, ST0_CU1 | ST0_FR | ST0_IM and a0, v1 LONG_L v0, PT_STATUS(sp) nor v1, $0, v1 -- 2.4.10 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] MIPS: Fix FPU disable with preemption @ 2016-02-01 13:50 ` James Hogan 0 siblings, 0 replies; 7+ messages in thread From: James Hogan @ 2016-02-01 13:50 UTC (permalink / raw) To: Ralf Baechle; +Cc: linux-mips, Paul Burton, James Hogan The FPU should not be left enabled after a task context switch. This isn't usually a problem as the FPU enable bit is updated before returning to userland, however it can potentially mask kernel bugs, and in fact KVM assumes it won't happen and won't clear the FPU enable bit before returning to the guest, which allows the guest to use stale FPU context. Interrupts and exceptions save and restore most bits of the CP0 Status register which contains the FPU enable bit (CU1). When the kernel needs to enable or disable the FPU (for example due to attempted FPU use by userland, or the scheduler being invoked) both the actual Status register and the saved value in the userland context are updated. However this doesn't work correctly with full kernel preemption enabled, since the FPU enable bit can be cleared from within an interrupt when the scheduler is invoked, and only the userland context is updated, not the interrupt context. For example: 1) Enter kernel with FPU already enabled, TIF_USEDFPU=1, Status.CU1=1 saved. 2) Take a timer interrupt while in kernel mode, Status.CU1=1 saved. 3) Timer interrupt invokes scheduler to preempt the task, which clears TIF_USEDFPU, disables the FPU in Status register (Status.CU1=0), and the value stored in user context from step (1), but not the interrupt context from step (2). 4) When the process is scheduled back in again Status.CU1=0. 5) The interrupt context from step (2) is restored, which sets Status.CU1=1. So from user context point of view, preemption has re-enabled FPU! 6) If the scheduler is invoked again (via preemption or voluntarily) before returning to userland, TIF_USEDFPU=0 so the FPU is not disabled before the task context switch. 7) The next task resumes from the context switch with FPU enabled! The restoring of the Status register on return from interrupt/exception is already selective about which bits to restore, leaving the interrupt mask bits alone so enabling/disabling of CPU interrupt lines can persist. Extend this to also leave both the CU1 bit (FPU enable) and the FR bit (which specifies the FPU mode and gets changed with CU1). This prevents a stale Status value being restored in step (5) above and persisting through subsequent context switches. Also switch to the use of definitions from asm/mipsregs.h while we're at it. Since this change also affects the restoration of Status register on the path back to userland, it increases the sensitivity of the kernel to the problem of the FPU being left enabled, allowing it to propagate to userland, therefore a warning is also added to lose_fpu_inatomic() to point out any future reoccurances before they do any damage. Signed-off-by: James Hogan <james.hogan@imgtec.com> Reviewed-by: Paul Burton <paul.burton@imgtec.com> Cc: Ralf Baechle <ralf@linux-mips.org> Cc: linux-mips@linux-mips.org --- arch/mips/include/asm/fpu.h | 4 ++++ arch/mips/include/asm/stackframe.h | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/arch/mips/include/asm/fpu.h b/arch/mips/include/asm/fpu.h index 9cbf383b8834..f06f97bd62df 100644 --- a/arch/mips/include/asm/fpu.h +++ b/arch/mips/include/asm/fpu.h @@ -179,6 +179,10 @@ static inline void lose_fpu_inatomic(int save, struct task_struct *tsk) if (save) _save_fp(tsk); __disable_fpu(); + } else { + /* FPU should not have been left enabled with no owner */ + WARN(read_c0_status() & ST0_CU1, + "Orphaned FPU left enabled"); } KSTK_STATUS(tsk) &= ~ST0_CU1; clear_tsk_thread_flag(tsk, TIF_USEDFPU); diff --git a/arch/mips/include/asm/stackframe.h b/arch/mips/include/asm/stackframe.h index a71da576883c..eebf39549606 100644 --- a/arch/mips/include/asm/stackframe.h +++ b/arch/mips/include/asm/stackframe.h @@ -289,7 +289,7 @@ .set reorder .set noat mfc0 a0, CP0_STATUS - li v1, 0xff00 + li v1, ST0_CU1 | ST0_IM ori a0, STATMASK xori a0, STATMASK mtc0 a0, CP0_STATUS @@ -330,7 +330,7 @@ ori a0, STATMASK xori a0, STATMASK mtc0 a0, CP0_STATUS - li v1, 0xff00 + li v1, ST0_CU1 | ST0_FR | ST0_IM and a0, v1 LONG_L v0, PT_STATUS(sp) nor v1, $0, v1 -- 2.4.10 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] MIPS: Fix FPU preemption issues 2016-02-01 13:50 ` James Hogan ` (2 preceding siblings ...) (?) @ 2016-02-01 22:54 ` Ralf Baechle -1 siblings, 0 replies; 7+ messages in thread From: Ralf Baechle @ 2016-02-01 22:54 UTC (permalink / raw) To: James Hogan; +Cc: linux-mips, Paul Burton Both applied. Thanks, Ralf ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-02-01 22:54 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-02-01 13:50 [PATCH 0/2] MIPS: Fix FPU preemption issues James Hogan 2016-02-01 13:50 ` James Hogan 2016-02-01 13:50 ` [PATCH 1/2] MIPS: Properly disable FPU in start_thread() James Hogan 2016-02-01 13:50 ` James Hogan 2016-02-01 13:50 ` [PATCH 2/2] MIPS: Fix FPU disable with preemption James Hogan 2016-02-01 13:50 ` James Hogan 2016-02-01 22:54 ` [PATCH 0/2] MIPS: Fix FPU preemption issues Ralf Baechle
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.