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

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

[Will Deacon]

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: <stable@vger.kernel.org>
Cc: Larry Bassel <larry.bassel@linaro.org>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Reported-by: Hanjun Guo <hanjun.guo@linaro.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/entry.S | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 301c37207c64..2a5e64ccc991 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -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)
 
 /*
-- 
2.1.4

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

[Catalin Marinas]

On Wed, Aug 19, 2015 at 04:09:49PM +0100, Will Deacon wrote:
> @@ -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

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

-- 
Catalin

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

[Will Deacon]

On Wed, Aug 19, 2015 at 05:03:20PM +0100, Catalin Marinas wrote:
> On Wed, Aug 19, 2015 at 04:09:49PM +0100, Will Deacon wrote:
> > @@ -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
> 
> 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

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 2a5e64ccc991..088322ff1ba0 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -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.

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

[Catalin Marinas]

On Wed, Aug 19, 2015 at 05:23:59PM +0100, Will Deacon wrote:
> On Wed, Aug 19, 2015 at 05:03:20PM +0100, Catalin Marinas wrote:
> > On Wed, Aug 19, 2015 at 04:09:49PM +0100, Will Deacon wrote:
> > > @@ -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
> > 
> > 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
> 
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 2a5e64ccc991..088322ff1ba0 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -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 <catalin.marinas@arm.com>

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

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

[Will Deacon]

On Wed, Aug 19, 2015 at 05:35:05PM +0100, Catalin Marinas wrote:
> On Wed, Aug 19, 2015 at 05:23:59PM +0100, Will Deacon wrote:
> > On Wed, Aug 19, 2015 at 05:03:20PM +0100, Catalin Marinas wrote:
> > > 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
> > 
> > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> > index 2a5e64ccc991..088322ff1ba0 100644
> > --- a/arch/arm64/kernel/entry.S
> > +++ b/arch/arm64/kernel/entry.S
> > @@ -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 <catalin.marinas@arm.com>
> 
> (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

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

[Hanjun Guo]

On 08/20/2015 12:56 AM, Will Deacon wrote:
> On Wed, Aug 19, 2015 at 05:35:05PM +0100, Catalin Marinas wrote:
>> On Wed, Aug 19, 2015 at 05:23:59PM +0100, Will Deacon wrote:
>>> On Wed, Aug 19, 2015 at 05:03:20PM +0100, Catalin Marinas wrote:
>>>> 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
>>>
>>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>>> index 2a5e64ccc991..088322ff1ba0 100644
>>> --- a/arch/arm64/kernel/entry.S
>>> +++ b/arch/arm64/kernel/entry.S
>>> @@ -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 <catalin.marinas@arm.com>
>>
>> (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 <hanjun.guo@linaro.org>

Thanks
Hanjun

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

[Will Deacon]

On Thu, Aug 20, 2015 at 09:42:12AM +0100, Hanjun Guo wrote:
> On 08/20/2015 12:56 AM, Will Deacon wrote:
> > On Wed, Aug 19, 2015 at 05:35:05PM +0100, Catalin Marinas wrote:
> >> On Wed, Aug 19, 2015 at 05:23:59PM +0100, Will Deacon wrote:
> >>> On Wed, Aug 19, 2015 at 05:03:20PM +0100, Catalin Marinas wrote:
> >>>> 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
> >>>
> >>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> >>> index 2a5e64ccc991..088322ff1ba0 100644
> >>> --- a/arch/arm64/kernel/entry.S
> >>> +++ b/arch/arm64/kernel/entry.S
> >>> @@ -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 <catalin.marinas@arm.com>
> >>
> >> (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 <hanjun.guo@linaro.org>

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