Skip to contentSkip to conversation

arm64: entry: always restore x0 from the stack on syscall return

PATCH New Will Deacon 3 participants 7 messages Aug 19, 2015 – Aug 20, 2015

Subsystems (2)
Stable branch arm64
lore
Share & tools
lore

Will Deacon

author
#0 lore raw

PATCH 1/1 arm64: entry: always restore x0 from the stack on syscall return

We have a micro-optimisation on the fast syscall return path where we take care to keep x0 live with the return value from the syscall so that we can avoid restoring it from the stack. The benefit of doing this is fairly suspect, since we will be restoring x1 from the stack anyway (which lives adjacent in the pt_regs structure) and the only additional cost is saving x0 back to pt_regs after the syscall handler, which could be seen as a poor man's prefetch.

More importantly, this causes issues with the context tracking code.

The ct_user_enter macro ends up branching into C code, which is free to use x0 as a scratch register and consequently leads to us returning junk back to userspace as the syscall return value. Rather than special case the context-tracking code, this patch removes the questionable optimisation entirely.

Cc <[email protected]>
Cc Larry Bassel <[email protected]>
Cc Kevin Hilman <[email protected]>
Cc Catalin Marinas <[email protected]>
Reported-by Hanjun Guo <[email protected]>
Signed-off-by Will Deacon <[email protected]>
---
 arch/arm64/kernel/entry.S | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)
arch/arm64/kernel/entry.S +5 -10
index 301c37207c64..2a5e64ccc991 100644
@@ -116,7 +116,7 @@
 	*/
 	.endm
 
-	.macro	kernel_exit, el, ret = 0
+	.macro	kernel_exit, el
 	ldp	x21, x22, [sp, #S_PC]		// load ELR, SPSR
 	.if	\el == 0
 	ct_user_enter

@@ -146,11 +146,7 @@
 	.endif
 	msr	elr_el1, x21			// set up the return data
 	msr	spsr_el1, x22
-	.if	\ret
-	ldr	x1, [sp, #S_X1]			// preserve x0 (syscall return)
-	.else
 	ldp	x0, x1, [sp, #16 * 0]
-	.endif
 	ldp	x2, x3, [sp, #16 * 1]
 	ldp	x4, x5, [sp, #16 * 2]
 	ldp	x6, x7, [sp, #16 * 3]

@@ -613,13 +609,14 @@ ENDPROC(cpu_switch_to)
  */
 ret_fast_syscall:
 	disable_irq				// disable interrupts
+	str	x0, [sp, #S_X0]			// returned x0
 	ldr	x1, [tsk, #TI_FLAGS]		// re-check for syscall tracing
 	and	x2, x1, #_TIF_SYSCALL_WORK
 	cbnz	x2, ret_fast_syscall_trace
 	and	x2, x1, #_TIF_WORK_MASK
-	cbnz	x2, fast_work_pending
+	cbnz	x2, work_pending
 	enable_step_tsk x1, x2
-	kernel_exit 0, ret = 1
+	kernel_exit 0
 ret_fast_syscall_trace:
 	enable_irq				// enable interrupts
 	b	__sys_trace_return

@@ -627,8 +624,6 @@ ret_fast_syscall_trace:
 /*
  * Ok, we need to do extra processing, enter the slow path.
  */
-fast_work_pending:
-	str	x0, [sp, #S_X0]			// returned x0
 work_pending:
 	tbnz	x1, #TIF_NEED_RESCHED, work_resched
 	/* TIF_SIGPENDING, TIF_NOTIFY_RESUME or TIF_FOREIGN_FPSTATE case */

@@ -652,7 +647,7 @@ ret_to_user:
 	cbnz	x2, work_pending
 	enable_step_tsk x1, x2
 no_work_pending:
-	kernel_exit 0, ret = 0
+	kernel_exit 0
 ENDPROC(ret_to_user)
 
 /*
Signature · 1 line
2.1.4
maintainer
#1 lore raw

PATCH 1/1 arm64: entry: always restore x0 from the stack on syscall return

Quoted 7 lines · Will Deacon
arch/arm64/kernel/entry.S +1 -0
@@ -613,13 +609,14 @@ ENDPROC(cpu_switch_to)
  */
 ret_fast_syscall:
 	disable_irq				// disable interrupts
+	str	x0, [sp, #S_X0]			// returned x0
 	ldr	x1, [tsk, #TI_FLAGS]		// re-check for syscall tracing
 	and	x2, x1, #_TIF_SYSCALL_WORK

arch/arm64/kernel/entry.S ENDPROC(cpu_switch_to) · line 622

Quoted 10 lines
arch/arm64/kernel/entry.S +2 -2
 	cbnz	x2, ret_fast_syscall_trace
 	and	x2, x1, #_TIF_WORK_MASK
-	cbnz	x2, fast_work_pending
+	cbnz	x2, work_pending
 	enable_step_tsk x1, x2
-	kernel_exit 0, ret = 1
+	kernel_exit 0
 ret_fast_syscall_trace:
 	enable_irq				// enable interrupts
 	b	__sys_trace_return

There is another str x0 in __sys_trace_return which I think we could remove.

Signature · 1 line
Catalin
author
#2 lore raw

PATCH 1/1 arm64: entry: always restore x0 from the stack on syscall return

Quoted 13 lines · Catalin Marinas
Quoted 12 lines · Will Deacon
(quoted diff) +2 -1
@@ -613,13 +609,14 @@ ENDPROC(cpu_switch_to)
  */
 ret_fast_syscall:
 	disable_irq				// disable interrupts
+	str	x0, [sp, #S_X0]			// returned x0
 	ldr	x1, [tsk, #TI_FLAGS]		// re-check for syscall tracing
 	and	x2, x1, #_TIF_SYSCALL_WORK
 	cbnz	x2, ret_fast_syscall_trace
 	and	x2, x1, #_TIF_WORK_MASK
-	cbnz	x2, fast_work_pending
+	cbnz	x2, work_pending
 	enable_step_tsk x1, x2
Quoted 8 lines

Review ENDPROC(cpu_switch_to) · line 622

Quoted 5 lines
(quoted diff) +1 -1
-	kernel_exit 0, ret = 1
+	kernel_exit 0
 ret_fast_syscall_trace:
 	enable_irq				// enable interrupts
 	b	__sys_trace_return

There is another str x0 in __sys_trace_return which I think we could remove.

Hmm, I don't think we can remove that. It's needed on the slowpath to update the pt_regs with either -ENOSYS (for __ni_sys_trace) or the syscall return value from the blr in __sys_trace.

What we can do instead is change the branch above to branch to __sys_trace_return_skipped. Patch below.

Will

--->8

arch/arm64/kernel/entry.S +1 -1
index 2a5e64ccc991..088322ff1ba0 100644
@@ -619,7 +619,7 @@ ret_fast_syscall:
        kernel_exit 0
 ret_fast_syscall_trace:
        enable_irq                              // enable interrupts
-       b       __sys_trace_return
+       b       __sys_trace_return_skipped      // we already saved x0
 
 /*
  * Ok, we need to do extra processing, enter the slow path.
maintainer
#3 lore raw

PATCH 1/1 arm64: entry: always restore x0 from the stack on syscall return

Quoted 39 lines · Will Deacon
Quoted 13 lines · Catalin Marinas
Quoted 12 lines · Will Deacon
(quoted diff) +2 -1
@@ -613,13 +609,14 @@ ENDPROC(cpu_switch_to)
  */
 ret_fast_syscall:
 	disable_irq				// disable interrupts
+	str	x0, [sp, #S_X0]			// returned x0
 	ldr	x1, [tsk, #TI_FLAGS]		// re-check for syscall tracing
 	and	x2, x1, #_TIF_SYSCALL_WORK
 	cbnz	x2, ret_fast_syscall_trace
 	and	x2, x1, #_TIF_WORK_MASK
-	cbnz	x2, fast_work_pending
+	cbnz	x2, work_pending
 	enable_step_tsk x1, x2
Quoted 8 lines

Review ENDPROC(cpu_switch_to) · line 622

Quoted 5 lines
(quoted diff) +1 -1
-	kernel_exit 0, ret = 1
+	kernel_exit 0
 ret_fast_syscall_trace:
 	enable_irq				// enable interrupts
 	b	__sys_trace_return

There is another str x0 in __sys_trace_return which I think we could remove.

Hmm, I don't think we can remove that. It's needed on the slowpath to update the pt_regs with either -ENOSYS (for __ni_sys_trace) or the syscall return value from the blr in __sys_trace.

What we can do instead is change the branch above to branch to __sys_trace_return_skipped. Patch below.

Will

--->8

arch/arm64/kernel/entry.S +0 -0
index 2a5e64ccc991..088322ff1ba0 100644

arch/arm64/kernel/entry.S ret_fast_syscall: · line 622

Quoted 5 lines
arch/arm64/kernel/entry.S +1 -1
@@ -619,7 +619,7 @@ ret_fast_syscall:
        kernel_exit 0
 ret_fast_syscall_trace:
        enable_irq                              // enable interrupts
-       b       __sys_trace_return
+       b       __sys_trace_return_skipped      // we already saved x0

That would do. With this added:

Reviewed-by Catalin Marinas <[email protected]>

(or I can take it for 4.2 but I'd like more testing like LTP)

author
#4 lore raw

PATCH 1/1 arm64: entry: always restore x0 from the stack on syscall return

Quoted 29 lines · Catalin Marinas
Quoted 20 lines · Will Deacon
Quoted 2 lines · Catalin Marinas

There is another str x0 in __sys_trace_return which I think we could remove.

Hmm, I don't think we can remove that. It's needed on the slowpath to update the pt_regs with either -ENOSYS (for __ni_sys_trace) or the syscall return value from the blr in __sys_trace.

What we can do instead is change the branch above to branch to __sys_trace_return_skipped. Patch below.

Will

--->8

arch/arm64/kernel/entry.S +0 -0
index 2a5e64ccc991..088322ff1ba0 100644

arch/arm64/kernel/entry.S ret_fast_syscall: · line 622

Quoted 5 lines
arch/arm64/kernel/entry.S +1 -1
@@ -619,7 +619,7 @@ ret_fast_syscall:
        kernel_exit 0
 ret_fast_syscall_trace:
        enable_irq                              // enable interrupts
-       b       __sys_trace_return
+       b       __sys_trace_return_skipped      // we already saved x0

That would do. With this added:

Quoted 3 lines
Reviewed-by Catalin Marinas <[email protected]>

(or I can take it for 4.2 but I'd like more testing like LTP)

Yeah, I'll run some tests overnight and see how it holds up.

Will

#5 lore raw

PATCH 1/1 arm64: entry: always restore x0 from the stack on syscall return

Quoted 30 lines · Will Deacon
Quoted 29 lines · Catalin Marinas
Quoted 20 lines · Will Deacon
Quoted 2 lines · Catalin Marinas

There is another str x0 in __sys_trace_return which I think we could remove.

Hmm, I don't think we can remove that. It's needed on the slowpath to update the pt_regs with either -ENOSYS (for __ni_sys_trace) or the syscall return value from the blr in __sys_trace.

What we can do instead is change the branch above to branch to __sys_trace_return_skipped. Patch below.

Will

--->8

arch/arm64/kernel/entry.S +0 -0
index 2a5e64ccc991..088322ff1ba0 100644

arch/arm64/kernel/entry.S ret_fast_syscall: · line 622

Quoted 5 lines
arch/arm64/kernel/entry.S +1 -1
@@ -619,7 +619,7 @@ ret_fast_syscall:
         kernel_exit 0
  ret_fast_syscall_trace:
         enable_irq                              // enable interrupts
-       b       __sys_trace_return
+       b       __sys_trace_return_skipped      // we already saved x0

That would do. With this added:

Quoted 5 lines
Quoted 3 lines
Reviewed-by Catalin Marinas <[email protected]>

(or I can take it for 4.2 but I'd like more testing like LTP)

Yeah, I'll run some tests overnight and see how it holds up.

How is the stress test going on? I didn't do some stress test but when I applied this patch (along with above additions), the problem I reported is gone,

Tested-by Hanjun Guo <[email protected]>

Thanks
Hanjun

author
#6 lore raw

PATCH 1/1 arm64: entry: always restore x0 from the stack on syscall return

Quoted 35 lines · Hanjun Guo
Quoted 34 lines · Will Deacon
Quoted 32 lines · Catalin Marinas
Quoted 20 lines · Will Deacon
Quoted 2 lines · Catalin Marinas

There is another str x0 in __sys_trace_return which I think we could remove.

Hmm, I don't think we can remove that. It's needed on the slowpath to update the pt_regs with either -ENOSYS (for __ni_sys_trace) or the syscall return value from the blr in __sys_trace.

What we can do instead is change the branch above to branch to __sys_trace_return_skipped. Patch below.

Will

--->8

arch/arm64/kernel/entry.S +0 -0
index 2a5e64ccc991..088322ff1ba0 100644

arch/arm64/kernel/entry.S ret_fast_syscall: · line 622

Quoted 5 lines
arch/arm64/kernel/entry.S +1 -1
@@ -619,7 +619,7 @@ ret_fast_syscall:
         kernel_exit 0
  ret_fast_syscall_trace:
         enable_irq                              // enable interrupts
-       b       __sys_trace_return
+       b       __sys_trace_return_skipped      // we already saved x0

That would do. With this added:

Reviewed-by Catalin Marinas <[email protected]>

(or I can take it for 4.2 but I'd like more testing like LTP)

Quoted 7 lines
Quoted 1 line

Yeah, I'll run some tests overnight and see how it holds up.

How is the stress test going on? I didn't do some stress test but when I applied this patch (along with above additions), the problem I reported is gone,

Tested-by Hanjun Guo <[email protected]>

Thanks, Hanjun. LTP passed successfully, so I think the patch is ok but there's no need to rush it in for 4.2.

Will