arm: Early IRQ enabling in the Undef user handler

This patch is in preparation for calling the iwmmxt_task_enable() function with interrupts enabled.

This patch is in preparation for calling the crunch_task_enable() function with interrupts enabled.

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_usr before attempting to read the instruction from user space.

Signed-off-by Catalin Marinas <[email protected]>

Cc Hartley Sweeten <[email protected]>

Cc Ryan Mallon <[email protected]>

---
 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 +0 -0

index 1879e8dd2acc..5fc897cf409b 100644
@@ -413,6 +413,11 @@ __und_usr:
        @

arch/arm/kernel/entry-armv.S +2 -4

@@ -517,7 +522,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

@@ -624,7 +629,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

@@ -652,8 +656,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

index c52f3e225aeb..da3117054712 100644
@@ -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

index 890c5df2b4fe..85e765534003 100644
@@ -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

index f0759e70fb86..fe6ca574d093 100644
@@ -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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

As the data abort is now handled in normal way, can we remove the fixup handler for ldrt?

Quoted 2 lines · Arun KS

As the data abort is now handled in normal way, can we remove the fixup handler for ldrt?

No, because all instructions which access userspace, and therefore may fault, must be given a fixup handler so the kernel knows what to do should it not be able to supply the page.

Omitting that marking means the only other option in that case is for the kernel to oops.

From d72e15d92c1016ce3b1c7c7da01ca60cf21340d5 Mon Sep 17 00:00:00 2001

--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it.

Quoted 10 lines · Russell King - ARM Linux

Quoted 2 lines · Arun KS

As the data abort is now handled in normal way, can we remove the fixup handler for ldrt?

No, because all instructions which access userspace, and therefore may fault, must be given a fixup handler so the kernel knows what to do should it not be able to supply the page.

Omitting that marking means the only other option in that case is for the kernel to oops.

I have a concern regarding the fixup handler.
Fixup handler returns to the next instruction which has caused the undef execption, rather than going to the same instruction.

ARM ARM says that after undefined exception, the pc will be pointing to the next instruction.
ie +4 offset in case of ARM and +2 in case of Thumb.

And there is no correction offset passed to vector_stub in case of undef exception.

File: arch/arm/kernel/entry-armv.S +1085
vector_stub und, UND_MODE

During an undefined exception, in normal scenario(ie when ldrt instruction does not cause an abort) after resorting the context in VFP hardware, the PC is modified as show below before jumping to ret_from_exception which is in r9.

File: arch/arm/vfp/vfphw.S +169
@ The context stored in the VFP hardware is up to date with this thread vfp_hw_state_valid:
tst r1, #FPEXC_EX
bne process_exception @ might as well handle the pending
@ exception before retrying branch @ out before setting an FPEXC that @ stops us reading stuff
VFPFMXR FPEXC, r1 @ Restore FPEXC last
sub r2, r2, #4 @ Retry current instruction - if Thumb
str r2, [sp, #S_PC] @ mode it's two 16-bit instructions,
@ else it's one 32-bit instruction, so @ always subtract 4 from the following @ instruction address.

But if ldrt results in an abort, we reach the fixup handler and return to ret_from_execption without correcting the pc.

From d72e15d92c1016ce3b1c7c7da01ca60cf21340d5 Mon Sep 17 00:00:00 2001
From: Arun KS <[email protected]>
Date: Fri, 4 Apr 2014 16:42:58 +0530
Subject: Modify fixup handler to re-execute the original instruction

Signed-off-by Danesh Petigara <[email protected]>

Signed-off-by Vinayak Menon <[email protected]>

---
 arch/arm/kernel/entry-armv.S |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

arch/arm/kernel/entry-armv.S +0 -0

index 1879e8d..e801c7d 100644

Hi Russell,

I've had this series in my repo for a long time with some follow-ups on the list but without any conclusive ACK or NAK. If there are no objections, I plan to send it to the patch system.

The only change is rebasing to 3.14-rc6 and adding some Cc for the crunch bits.

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).

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(-)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 22 lines · Catalin Marinas

I've had this series in my repo for a long time with some follow-ups on the list but without any conclusive ACK or NAK. If there are no objections, I plan to send it to the patch system.

The only change is rebasing to 3.14-rc6 and adding some Cc for the crunch bits.

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).

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

Quoted 8 lines

 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(-)

Tested-by Arun KS<[email protected]>

Quoted 33 lines · Arun KS

Quoted 30 lines · Catalin Marinas

I've had this series in my repo for a long time with some follow-ups on the list but without any conclusive ACK or NAK. If there are no objections, I plan to send it to the patch system.

The only change is rebasing to 3.14-rc6 and adding some Cc for the crunch bits.

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).

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(-)

Quoted 1 line

Tested-by Arun KS<[email protected]>

Thanks. I assume it's tested only with VFP (not crunch nor iwmmxt).

--
Catalin