arm: Early IRQ enabling in the Undef user handler
Subsystems (4)
cover arm: Early IRQ enabling in the Undef user handler
The __und_usr handler accesses the user space to read the opcode of the faulting instruction. This is currently done with interrupts disabled but it could potentially cause a data abort of the page table was modified from another CPU. The data abort with interrupts disabled triggers a might_sleep() warning in do_page_fault() or, worse, deadlock on the pte lock when TLB ops broadcasting is enabled (ARM11MPCore or A15 together with workaround for erratum 798181).
To enable interrupts earlier, the preemption needs to be disabled in the crunch and iwmmxt handlers. The series needs testing on such hardware.
(an alternative simpler patch would be to enable the interrupts only around the user access instructions in __und_usr and disable them afterwards, so I would rather do it as in this series).
Catalin Marinas (5):
arm: Move asm macro get_thread_info to asm/assembler.h
arm: Add {inc,dec}_preempt_count asm macros
arm: Disable preemption in iwmmxt_task_enable()
arm: Disable preemption in crunch_task_enable()
arm: Enable IRQs before attempting to read user space in __und_usr
arch/arm/include/asm/assembler.h | 42 ++++++++++++++++++++++++++++++++++++++ arch/arm/kernel/entry-armv.S | 11 ++++++---- arch/arm/kernel/entry-header.S | 11 ---------- arch/arm/kernel/iwmmxt.S | 15 ++++++++++---- arch/arm/mach-ep93xx/crunch-bits.S | 13 +++++++++--- arch/arm/vfp/entry.S | 28 ++++++++----------------- arch/arm/vfp/vfphw.S | 19 ++++++----------- 7 files changed, 84 insertions(+), 55 deletions(-)
PATCH 1/5 arm: Move asm macro get_thread_info to asm/assembler.h
asm/assembler.h is a better place for this macro since it is used by asm files outside arch/arm/kernel/
--- arch/arm/include/asm/assembler.h | 10 ++++++++++ arch/arm/kernel/entry-header.S | 11 ----------- arch/arm/vfp/entry.S | 5 ++++- arch/arm/vfp/vfphw.S | 5 ++++- 4 files changed, 18 insertions(+), 13 deletions(-)
arch/arm/include/asm/assembler.h +10 -0
@@ -167,6 +167,16 @@ restore_irqs_notrace \oldcpsr .endm +/* + * Get current thread_info. + */ + .macro get_thread_info, rd + ARM( mov \rd, sp, lsr #13 ) + THUMB( mov \rd, sp ) + THUMB( lsr \rd, \rd, #13 ) + mov \rd, \rd, lsl #13 + .endm + #define USER(x...) \ 9999: x; \ .pushsection __ex_table,"a"; \
arch/arm/kernel/entry-header.S +0 -11
@@ -236,11 +236,6 @@ movs pc, lr @ return & move spsr_svc into cpsr .endm - .macro get_thread_info, rd - mov \rd, sp, lsr #13 - mov \rd, \rd, lsl #13 - .endm - @ @ 32-bit wide "mov pc, reg" @
@@ -306,12 +301,6 @@ .endm #endif /* ifdef CONFIG_CPU_V7M / else */ - .macro get_thread_info, rd - mov \rd, sp - lsr \rd, \rd, #13 - mov \rd, \rd, lsl #13 - .endm - @ @ 32-bit wide "mov pc, reg" @
arch/arm/vfp/entry.S +4 -1
@@ -8,9 +8,12 @@ * it under the terms of the GNU General Public License version 2 as * published by the Free Software Foundation. */ +#include <linux/init.h> +#include <linux/linkage.h> #include <asm/thread_info.h> #include <asm/vfpmacros.h> -#include "../kernel/entry-header.S" +#include <asm/assembler.h> +#include <asm/asm-offsets.h> @ VFP entry point. @
arch/arm/vfp/vfphw.S +4 -1
@@ -14,10 +14,13 @@ * r10 points at the start of the private FP workspace in the thread structure * sp points to a struct pt_regs (as defined in include/asm/proc/ptrace.h) */ +#include <linux/init.h> +#include <linux/linkage.h> #include <asm/thread_info.h> #include <asm/vfpmacros.h> #include <linux/kern_levels.h> -#include "../kernel/entry-header.S" +#include <asm/assembler.h> +#include <asm/asm-offsets.h> .macro DBGSTR, str #ifdef DEBUG
PATCH 2/5 arm: Add {inc,dec}_preempt_count asm macros
In reply to Catalin MarinasThe patch adds asm macros for inc_preempt_count and dec_preempt_count_ti (which also gets the current thread_info) instead of open-coding them in arch/arm/vfp/*.S files.
--- arch/arm/include/asm/assembler.h | 32 ++++++++++++++++++++++++++++++++ arch/arm/vfp/entry.S | 20 +++----------------- arch/arm/vfp/vfphw.S | 14 ++------------ 3 files changed, 37 insertions(+), 29 deletions(-)
arch/arm/include/asm/assembler.h +32 -0
@@ -23,6 +23,7 @@#include <asm/ptrace.h> #include <asm/domain.h> #include <asm/opcodes-virt.h> +#include <asm/asm-offsets.h> #define IOMEM (x) (x)
@@ -177,6 +178,37 @@ mov \rd, \rd, lsl #13 .endm +/* + * Increment/decrement the preempt count. + */ +#ifdef CONFIG_PREEMPT_COUNT + .macro inc_preempt_count, ti, tmp + ldr \tmp, [\ti, #TI_PREEMPT] @ get preempt count + add \tmp, \tmp, #1 @ increment it + str \tmp, [\ti, #TI_PREEMPT] + .endm + + .macro dec_preempt_count, ti, tmp + ldr \tmp, [\ti, #TI_PREEMPT] @ get preempt count + sub \tmp, \tmp, #1 @ decrement it + str \tmp, [\ti, #TI_PREEMPT] + .endm + + .macro dec_preempt_count_ti, ti, tmp + get_thread_info \ti + dec_preempt_count \ti, \tmp + .endm +#else + .macro inc_preempt_count, ti, tmp + .endm + + .macro dec_preempt_count, ti, tmp + .endm + + .macro dec_preempt_count_ti, ti, tmp + .endm +#endif + #define USER(x...) \ 9999: x; \ .pushsection __ex_table,"a"; \
arch/arm/vfp/entry.S +3 -17
@@ -25,11 +25,7 @@ @ IRQs disabled. @ ENTRY(do_vfp) -#ifdef CONFIG_PREEMPT_COUNT - ldr r4, [r10, #TI_PREEMPT] @ get preempt count - add r11, r4, #1 @ increment it - str r11, [r10, #TI_PREEMPT] -#endif + inc_preempt_count r10, r4 enable_irq ldr r4, .LCvfp ldr r11, [r10, #TI_CPU] @ CPU number
@@ -38,12 +34,7 @@ ENTRY(do_vfp) ENDPROC(do_vfp) ENTRY(vfp_null_entry) -#ifdef CONFIG_PREEMPT_COUNT - get_thread_info r10 - ldr r4, [r10, #TI_PREEMPT] @ get preempt count - sub r11, r4, #1 @ decrement it - str r11, [r10, #TI_PREEMPT] -#endif + dec_preempt_count_ti r10, r4 mov pc, lr ENDPROC(vfp_null_entry)
@@ -56,12 +47,7 @@ ENDPROC(vfp_null_entry) __INIT ENTRY(vfp_testing_entry) -#ifdef CONFIG_PREEMPT_COUNT - get_thread_info r10 - ldr r4, [r10, #TI_PREEMPT] @ get preempt count - sub r11, r4, #1 @ decrement it - str r11, [r10, #TI_PREEMPT] -#endif + dec_preempt_count_ti r10, r4 ldr r0, VFP_arch_address str r0, [r0] @ set to non-zero value mov pc, r9 @ we have handled the fault
arch/arm/vfp/vfphw.S +2 -12
@@ -177,12 +177,7 @@ vfp_hw_state_valid: @ else it's one 32-bit instruction, so @ always subtract 4 from the following @ instruction address. -#ifdef CONFIG_PREEMPT_COUNT - get_thread_info r10 - ldr r4, [r10, #TI_PREEMPT] @ get preempt count - sub r11, r4, #1 @ decrement it - str r11, [r10, #TI_PREEMPT] -#endif + dec_preempt_count_ti r10, r4 mov pc, r9 @ we think we have handled things
@@ -201,12 +196,7 @@ look_for_VFP_exceptions: @ not recognised by VFP DBGSTR "not VFP" -#ifdef CONFIG_PREEMPT_COUNT - get_thread_info r10 - ldr r4, [r10, #TI_PREEMPT] @ get preempt count - sub r11, r4, #1 @ decrement it - str r11, [r10, #TI_PREEMPT] -#endif + dec_preempt_count_ti r10, r4 mov pc, lr process_exception:
PATCH 3/5 arm: Disable preemption in iwmmxt_task_enable()
In reply to Catalin MarinasThis patch is in preparation for calling the iwmmxt_task_enable() function with interrupts enabled.
--- arch/arm/kernel/iwmmxt.S | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
arch/arm/kernel/iwmmxt.S +10 -3
@@ -65,13 +65,14 @@ */ ENTRY(iwmmxt_task_enable) + inc_preempt_count r10, r3 XSC(mrc p15, 0, r2, c15, c1, 0) PJ4(mrc p15, 0, r2, c1, c0, 2) @ CP0 and CP1 accessible? XSC(tst r2, #0x3) PJ4(tst r2, #0xf) - movne pc, lr @ if so no business here + bne 4f @ if so no business here @ enable access to CP0 and CP1 XSC(orr r2, r2, #0x3) XSC(mcr p15, 0, r2, c15, c1, 0)
@@ -132,7 +133,7 @@ concan_dump: wstrd wR15, [r1, #MMX_WR15] 2: teq r0, #0 @ anything to load? - moveq pc, lr + beq 3f concan_load:
@@ -165,8 +166,14 @@ concan_load: @ clear CUP/MUP (only if r1 != 0) teq r1, #0 mov r2, #0 - moveq pc, lr + beq 3f tmcr wCon, r2 + +3: +#ifdef CONFIG_PREEMPT_COUNT + get_thread_info r10 +#endif +4: dec_preempt_count r10, r3 mov pc, lr /*
PATCH 4/5 arm: Disable preemption in crunch_task_enable()
In reply to Catalin MarinasThis patch is in preparation for calling the crunch_task_enable() function with interrupts enabled.
--- arch/arm/mach-ep93xx/crunch-bits.S | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
arch/arm/mach-ep93xx/crunch-bits.S +9 -2
@@ -65,11 +65,13 @@ * called from prefetch exception handler with interrupts disabled */ ENTRY(crunch_task_enable) + inc_preempt_count r10, r3 + ldr r8, =(EP93XX_APB_VIRT_BASE + 0x00130000) @ syscon addr ldr r1, [r8, #0x80] tst r1, #0x00800000 @ access to crunch enabled? - movne pc, lr @ if so no business here + bne 2f @ if so no business here mov r3, #0xaa @ unlock syscon swlock str r3, [r8, #0xc0] orr r1, r1, #0x00800000 @ enable access to crunch
@@ -142,7 +144,7 @@ crunch_save: teq r0, #0 @ anything to load? cfldr64eq mvdx0, [r1, #CRUNCH_MVDX0] @ mvdx0 was clobbered - moveq pc, lr + beq 1f crunch_load: cfldr64 mvdx0, [r0, #CRUNCH_DSPSC] @ load status word
@@ -190,6 +192,11 @@ crunch_load: cfldr64 mvdx14, [r0, #CRUNCH_MVDX14] cfldr64 mvdx15, [r0, #CRUNCH_MVDX15] +1: +#ifdef CONFIG_PREEMPT_COUNT + get_thread_info r10 +#endif +2: dec_preempt_count r10, r3 mov pc, lr /*
PATCH 5/5 arm: Enable IRQs before attempting to read user space in __und_usr
In reply to Catalin MarinasThe Undef abort handler in the kernel reads the undefined instruction from user space. If the page table was modified from another CPU, the user access could fail and do_page_fault() will be executed with interrupts disabled. This can potentially deadlock on ARM11MPCore or on Cortex-A15 with erratum 798181 workaround enabled (both implying IPI for TLB maintenance with page table lock held).
This patch enables the IRQs in __und_usr before attempting to read the instruction from user space.
--- arch/arm/kernel/entry-armv.S | 11 +++++++---- arch/arm/kernel/iwmmxt.S | 2 +- arch/arm/mach-ep93xx/crunch-bits.S | 2 +- arch/arm/vfp/entry.S | 3 +-- 4 files changed, 10 insertions(+), 8 deletions(-)
arch/arm/kernel/entry-armv.S +7 -4
@@ -411,6 +411,11 @@ __und_usr: @ adr r9, BSYM(ret_from_exception) + @ IRQs must be enabled before attempting to read the instruction from + @ user space since that could cause a page/translation fault if the + @ page table was modified by another CPU. + enable_irq + tst r3, #PSR_T_BIT @ Thumb mode? bne __und_usr_thumb sub r4, r2, #4 @ ARM instr at LR - 4
@@ -514,7 +519,7 @@ ENDPROC(__und_usr) * r9 = normal "successful" return address * r10 = this threads thread_info structure * lr = unrecognised instruction return address - * IRQs disabled, FIQs enabled. + * IRQs enabled, FIQs enabled. */ @ @ Fall-through from Thumb-2 __und_usr
@@ -621,7 +626,6 @@ call_fpe: #endif do_fpe: - enable_irq ldr r4, .LCfp add r10, r10, #TI_FPSTATE @ r10 = workspace ldr pc, [r4] @ Call FP module USR entry point
@@ -649,8 +653,7 @@ __und_usr_fault_32: b 1f __und_usr_fault_16: mov r1, #2 -1: enable_irq - mov r0, sp +1: mov r0, sp adr lr, BSYM(ret_from_exception) b __und_fault ENDPROC(__und_usr_fault_32)
arch/arm/kernel/iwmmxt.S +1 -1
@@ -61,7 +61,7 @@ * r9 = ret_from_exception * lr = undefined instr exit * - * called from prefetch exception handler with interrupts disabled + * called from prefetch exception handler with interrupts enabled */ ENTRY(iwmmxt_task_enable)
arch/arm/mach-ep93xx/crunch-bits.S +1 -1
@@ -62,7 +62,7 @@ * r9 = ret_from_exception * lr = undefined instr exit * - * called from prefetch exception handler with interrupts disabled + * called from prefetch exception handler with interrupts enabled */ ENTRY(crunch_task_enable) inc_preempt_count r10, r3
arch/arm/vfp/entry.S +1 -2
@@ -22,11 +22,10 @@ @ r9 = normal "successful" return address @ r10 = this threads thread_info structure @ lr = unrecognised instruction return address -@ IRQs disabled. +@ IRQs enabled. @ ENTRY(do_vfp) inc_preempt_count r10, r4 - enable_irq ldr r4, .LCvfp ldr r11, [r10, #TI_CPU] @ CPU number add r10, r10, #TI_VFPSTATE @ r10 = workspace
PATCH 5/5 arm: Enable IRQs before attempting to read user space in __und_usr
Quoted 9 lines · Catalin Marinas <catalin.marinas <at> arm.com> writes:
The Undef abort handler in the kernel reads the undefined instruction from user space. If the page table was modified from another CPU, the user access could fail and
do_page_fault()will be executed with interrupts disabled. This can potentially deadlock on ARM11MPCore or on Cortex-A15 with erratum 798181 workaround enabled (both implying IPI for TLB maintenance with page table lock held).This patch enables the IRQs in
__und_usrbefore attempting to read the instruction from user space.
This patch moves enable_irq call from do_fpe directly to __und_usr handler, but __und_svc handler also calls do_fpe (via call_fpe), so now this codepath runs with disabled irqs. This behavior change doesn't look good for me.
PATCH 5/5 arm: Enable IRQs before attempting to read user space in __und_usr
Quoted 9 lines · Alexey Ignatov
Quoted 7 lines · Catalin Marinas <catalin.marinas <at> arm.com> writes:
The Undef abort handler in the kernel reads the undefined instruction from user space. If the page table was modified from another CPU, the user access could fail and
do_page_fault()will be executed with interrupts disabled. This can potentially deadlock on ARM11MPCore or on Cortex-A15 with erratum 798181 workaround enabled (both implying IPI for TLB maintenance with page table lock held).
Quoted 6 lines
Quoted 2 lines
This patch enables the IRQs in
__und_usrbefore attempting to read the instruction from user space.This patch moves
enable_irqcall fromdo_fpedirectly to__und_usrhandler, but__und_svchandler also callsdo_fpe(viacall_fpe), so now this codepath runs with disabled irqs. This behavior change doesn't look good for me.
However, you're not executing FPA instructions in the kernel as a general rule, so it doesn't matter.
PATCH 5/5 arm: Enable IRQs before attempting to read user space in __und_usr
Quoted 13 lines · Russell King - ARM Linux
Quoted 12 lines · Alexey Ignatov
Quoted 9 lines · Catalin Marinas <catalin.marinas <at> arm.com> writes:
The Undef abort handler in the kernel reads the undefined instruction from user space. If the page table was modified from another CPU, the user access could fail and
do_page_fault()will be executed with interrupts disabled. This can potentially deadlock on ARM11MPCore or on Cortex-A15 with erratum 798181 workaround enabled (both implying IPI for TLB maintenance with page table lock held).This patch enables the IRQs in
__und_usrbefore attempting to read the instruction from user space.
Quoted 6 lines
Quoted 3 lines
This patch moves
enable_irqcall fromdo_fpedirectly to__und_usrhandler, but__und_svchandler also callsdo_fpe(viacall_fpe), so now this codepath runs with disabled irqs. This behavior change doesn't look good for me.However, you're not executing FPA instructions in the kernel as a general rule, so it doesn't matter.
Theoretically, ok.
It seems that we cought this deadlock on Cortex-A15 and this patch fixes things (testing in progress). Is there any plans to mainline?
cover arm: Early IRQ enabling in the Undef user handler
In reply to Catalin MarinasQuoted 3 lines · Catalin Marinas <catalin.marinas <at> arm.com> writes:
(an alternative simpler patch would be to enable the interrupts only around the user access instructions in
__und_usrand disable them afterwards, so I would rather do it as in this series).