[PATCH 0/5] arm: Early IRQ enabling in the Undef user handler

[PATCH 0/5] arm: Early IRQ enabling in the Undef user handler

[Catalin Marinas]

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 0/5] arm: Early IRQ enabling in the Undef user handler

[Catalin Marinas]

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

[PATCH 1/5] arm: Move asm macro get_thread_info to asm/assembler.h

[Catalin Marinas]

asm/assembler.h is a better place for this macro since it is used by
asm files outside arch/arm/kernel/

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 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(-)

diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
index 5c2285160575..7380a3ecffa2 100644
--- a/arch/arm/include/asm/assembler.h
+++ b/arch/arm/include/asm/assembler.h
@@ -174,6 +174,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";		\
diff --git a/arch/arm/kernel/entry-header.S b/arch/arm/kernel/entry-header.S
index 39f89fbd5111..1420725142ca 100644
--- a/arch/arm/kernel/entry-header.S
+++ b/arch/arm/kernel/entry-header.S
@@ -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"
 	@
diff --git a/arch/arm/vfp/entry.S b/arch/arm/vfp/entry.S
index 46e17492fd1f..9cc290ae4e3b 100644
--- a/arch/arm/vfp/entry.S
+++ b/arch/arm/vfp/entry.S
@@ -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.
 @
diff --git a/arch/arm/vfp/vfphw.S b/arch/arm/vfp/vfphw.S
index 3e5d3115a2a6..98f6246c9002 100644
--- a/arch/arm/vfp/vfphw.S
+++ b/arch/arm/vfp/vfphw.S
@@ -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

[Catalin Marinas]

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

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 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(-)

diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
index 7380a3ecffa2..a087318bbaff 100644
--- a/arch/arm/include/asm/assembler.h
+++ b/arch/arm/include/asm/assembler.h
@@ -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)
 
@@ -184,6 +185,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";		\
diff --git a/arch/arm/vfp/entry.S b/arch/arm/vfp/entry.S
index 9cc290ae4e3b..f0759e70fb86 100644
--- a/arch/arm/vfp/entry.S
+++ b/arch/arm/vfp/entry.S
@@ -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
diff --git a/arch/arm/vfp/vfphw.S b/arch/arm/vfp/vfphw.S
index 98f6246c9002..be807625ed8c 100644
--- a/arch/arm/vfp/vfphw.S
+++ b/arch/arm/vfp/vfphw.S
@@ -182,12 +182,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
 
 
@@ -206,12 +201,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()

[Catalin Marinas]

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

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm/kernel/iwmmxt.S | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/iwmmxt.S b/arch/arm/kernel/iwmmxt.S
index a08783823b32..c52f3e225aeb 100644
--- a/arch/arm/kernel/iwmmxt.S
+++ b/arch/arm/kernel/iwmmxt.S
@@ -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()

[Catalin Marinas]

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

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Cc: Hartley Sweeten <hsweeten@visionengravers.com>
Cc: Ryan Mallon <rmallon@gmail.com>
---
 arch/arm/mach-ep93xx/crunch-bits.S | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-ep93xx/crunch-bits.S b/arch/arm/mach-ep93xx/crunch-bits.S
index 0ec9bb48fab9..890c5df2b4fe 100644
--- a/arch/arm/mach-ep93xx/crunch-bits.S
+++ b/arch/arm/mach-ep93xx/crunch-bits.S
@@ -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

[Catalin Marinas]

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 <catalin.marinas@arm.com>
Cc: Hartley Sweeten <hsweeten@visionengravers.com>
Cc: Ryan Mallon <rmallon@gmail.com>
---
 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(-)

diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
index 1879e8dd2acc..5fc897cf409b 100644
--- a/arch/arm/kernel/entry-armv.S
+++ b/arch/arm/kernel/entry-armv.S
@@ -413,6 +413,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
@@ -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)
diff --git a/arch/arm/kernel/iwmmxt.S b/arch/arm/kernel/iwmmxt.S
index c52f3e225aeb..da3117054712 100644
--- a/arch/arm/kernel/iwmmxt.S
+++ b/arch/arm/kernel/iwmmxt.S
@@ -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)
diff --git a/arch/arm/mach-ep93xx/crunch-bits.S b/arch/arm/mach-ep93xx/crunch-bits.S
index 890c5df2b4fe..85e765534003 100644
--- a/arch/arm/mach-ep93xx/crunch-bits.S
+++ b/arch/arm/mach-ep93xx/crunch-bits.S
@@ -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
diff --git a/arch/arm/vfp/entry.S b/arch/arm/vfp/entry.S
index f0759e70fb86..fe6ca574d093 100644
--- a/arch/arm/vfp/entry.S
+++ b/arch/arm/vfp/entry.S
@@ -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 0/5] arm: Early IRQ enabling in the Undef user handler

[Yang Zhang]

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_usr and disable them
> afterwards, so I would rather do it as in this series).

Tested-by: Yang Zhang<email:tim.zhang@spreadtrum.com>

[PATCH 0/5] arm: Early IRQ enabling in the Undef user handler

[Arun KS]

On Thu, Mar 13, 2014 at 11:45 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> 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(-)

Tested-by: Arun KS<getarunks@gmail.com>

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

[PATCH 5/5] arm: Enable IRQs before attempting to read user space in __und_usr

[Arun KS]

Hi Catalin,

On Thu, Mar 13, 2014 at 11:45 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> 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 <catalin.marinas@arm.com>
> Cc: Hartley Sweeten <hsweeten@visionengravers.com>
> Cc: Ryan Mallon <rmallon@gmail.com>
> ---
>  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(-)
>
> diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
> index 1879e8dd2acc..5fc897cf409b 100644
> --- a/arch/arm/kernel/entry-armv.S
> +++ b/arch/arm/kernel/entry-armv.S
> @@ -413,6 +413,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

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

/*
 * The out of line fixup for the ldrt instructions above.
 */
        .pushsection .fixup, "ax"
        .align  2
4:      mov     pc, r9
        .popsection
        .pushsection __ex_table,"a"
        .long   1b, 4b
#if CONFIG_ARM_THUMB && __LINUX_ARM_ARCH__ >= 6 && CONFIG_CPU_V7
        .long   2b, 4b
        .long   3b, 4b
#endif
        .popsection


Thanks,
Arun

> @@ -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)
> diff --git a/arch/arm/kernel/iwmmxt.S b/arch/arm/kernel/iwmmxt.S
> index c52f3e225aeb..da3117054712 100644
> --- a/arch/arm/kernel/iwmmxt.S
> +++ b/arch/arm/kernel/iwmmxt.S
> @@ -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)
> diff --git a/arch/arm/mach-ep93xx/crunch-bits.S b/arch/arm/mach-ep93xx/crunch-bits.S
> index 890c5df2b4fe..85e765534003 100644
> --- a/arch/arm/mach-ep93xx/crunch-bits.S
> +++ b/arch/arm/mach-ep93xx/crunch-bits.S
> @@ -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
> diff --git a/arch/arm/vfp/entry.S b/arch/arm/vfp/entry.S
> index f0759e70fb86..fe6ca574d093 100644
> --- a/arch/arm/vfp/entry.S
> +++ b/arch/arm/vfp/entry.S
> @@ -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

[PATCH 5/5] arm: Enable IRQs before attempting to read user space in __und_usr

[Russell King - ARM Linux]

On Tue, Apr 01, 2014 at 05:54:15PM +0530, Arun KS wrote:
> 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.

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

[PATCH 0/5] arm: Early IRQ enabling in the Undef user handler

[Catalin Marinas]

On Tue, Apr 01, 2014 at 01:03:04PM +0100, Arun KS wrote:
> On Thu, Mar 13, 2014 at 11:45 PM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > 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(-)
> 
> Tested-by: Arun KS<getarunks@gmail.com>

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

-- 
Catalin

[PATCH 0/5] arm: Early IRQ enabling in the Undef user handler

[Arun KS]

On Tue, Apr 1, 2014 at 9:11 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Tue, Apr 01, 2014 at 01:03:04PM +0100, Arun KS wrote:
>> On Thu, Mar 13, 2014 at 11:45 PM, Catalin Marinas
>> <catalin.marinas@arm.com> wrote:
>> > 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(-)
>>
>> Tested-by: Arun KS<getarunks@gmail.com>
>
> Thanks. I assume it's tested only with VFP (not crunch nor iwmmxt).
Yes.

Thanks,
Arun
>
> --
> Catalin

[PATCH 5/5] arm: Enable IRQs before attempting to read user space in __und_usr

[Arun KS]

On Tue, Apr 1, 2014 at 8:18 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Apr 01, 2014 at 05:54:15PM +0530, Arun KS wrote:
>> 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.

Hi Catalin/Russell,

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.

File: arch/arm/kernel/entry-armv.S + 482
/*
 * The out of line fixup for the ldrt instructions above.
 */
        .pushsection .fixup, "ax"
        .align  2
4:      mov     pc, r9
        .popsection

Below patch fixes this,

>From d72e15d92c1016ce3b1c7c7da01ca60cf21340d5 Mon Sep 17 00:00:00 2001
From: Arun KS <getarunks@gmail.com>
Date: Fri, 4 Apr 2014 16:42:58 +0530
Subject: Modify fixup handler to re-execute the original instruction

Signed-off-by: Danesh Petigara <dpetigara@broadcom.com>
Signed-off-by: Vinayak Menon <vinayakm.list@gmail.com>
---
 arch/arm/kernel/entry-armv.S |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
index 1879e8d..e801c7d 100644
--- a/arch/arm/kernel/entry-armv.S
+++ b/arch/arm/kernel/entry-armv.S
@@ -484,7 +484,8 @@ ENDPROC(__und_usr)
  */
        .pushsection .fixup, "ax"
        .align  2
-4:     mov     pc, r9
+4:     str     r4, [sp, #S_PC]
+       mov     pc, r9
        .popsection
        .pushsection __ex_table,"a"
        .long   1b, 4b
--
1.7.6

Thanks,
Arun


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

[PATCH 5/5] arm: Enable IRQs before attempting to read user space in __und_usr

[Catalin Marinas]

Hi Arun,

On Fri, Apr 04, 2014 at 12:29:04PM +0100, Arun KS wrote:
> On Tue, Apr 1, 2014 at 8:18 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Tue, Apr 01, 2014 at 05:54:15PM +0530, Arun KS wrote:
> >> 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.

If ldrt results in an abort, with my patches for enabling the IRQ we
should not execute the fixup handler. The in-kernel page fault would
handled and the ldrt instruction re-executed.

I think we could still get to the fixup handler if one thread is
triggering the undef while another thread (different CPU) is munmap'ing
the page (very unlikely case though). The fault caused by ldrt in
__und_usr wouldn't be fully handled, falling back to the fixup.

> From d72e15d92c1016ce3b1c7c7da01ca60cf21340d5 Mon Sep 17 00:00:00 2001
> From: Arun KS <getarunks@gmail.com>
> Date: Fri, 4 Apr 2014 16:42:58 +0530
> Subject: Modify fixup handler to re-execute the original instruction
> 
> Signed-off-by: Danesh Petigara <dpetigara@broadcom.com>
> Signed-off-by: Vinayak Menon <vinayakm.list@gmail.com>

Some more text here is useful, like what you explained above, just add
it to the commit log.

> ---
>  arch/arm/kernel/entry-armv.S |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
> index 1879e8d..e801c7d 100644
> --- a/arch/arm/kernel/entry-armv.S
> +++ b/arch/arm/kernel/entry-armv.S
> @@ -484,7 +484,8 @@ ENDPROC(__und_usr)
>   */
>         .pushsection .fixup, "ax"
>         .align  2
> -4:     mov     pc, r9
> +4:     str     r4, [sp, #S_PC]
> +       mov     pc, r9
>         .popsection
>         .pushsection __ex_table,"a"
>         .long   1b, 4b

I think this patch should cover the above case and a subsequent
re-execution of the address in user space would trigger the prefetch
abort.

On its own, I think the above patch would also work without my other
patches. But the effect is that on ldrt fault we re-execute the user
instruction hoping that it will trigger the prefetch abort, fix it up in
the kernel and return to that instruction again. It may be worth as
cc: stable (assuming that anyone tests it properly).

Otherwise:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

[PATCH 3/5] arm: Disable preemption in iwmmxt_task_enable()

[Russell King - ARM Linux]

On Thu, Mar 13, 2014 at 06:15:50PM +0000, Catalin Marinas wrote:
> This patch is in preparation for calling the iwmmxt_task_enable()
> function with interrupts enabled.

This patch isn't even build-tested and causes build regressions due to
lack of an include.  Therefore, I'm dropping the whole series.

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

[PATCH 4/5] arm: Disable preemption in crunch_task_enable()

[Russell King - ARM Linux]

On Thu, Mar 13, 2014 at 06:15:51PM +0000, Catalin Marinas wrote:
> This patch is in preparation for calling the crunch_task_enable()
> function with interrupts enabled.

This also looks like it's missing an include.

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