arm syscall fast path can miss a ptrace syscall-exit

arm syscall fast path can miss a ptrace syscall-exit

[Josh Stone]

Hi,

I've discovered a case where both arm and arm64 will miss a ptrace
syscall-exit that they should report.  If the syscall is entered without
TIF_SYSCALL_TRACE set, then it goes on the fast path.  It's then
possible to have TIF_SYSCALL_TRACE added in the middle of the syscall,
but ret_fast_syscall doesn't check this flag again.

For instance, with PTRACE_O_TRACEFORK set, we could enter a fork() and
report PTRACE_EVENT_FORK to the tracer from do_fork(), in the middle of
the syscall.  That tracer may resume with PTRACE_SYSCALL, which sets
TIF_SYSCALL_TRACE; then do_fork() returns, and we *should* then get a
ptrace syscall-exit-stop.  But with arm and arm64, the syscall fast path
doesn't notice the added flag and just returns.

The attached program demonstrates the bug.  Note that it's important not
to have any other slow-path flags either, like TIF_SYSCALL_AUDIT.  On
x86_64, this program outputs:

  event-syscall-exit: stopped 18
  event-syscall-exit: ptrace event 1
  event-syscall-exit: syscall
  event-syscall-exit: stopped 11
  event-syscall-exit: signaled 11

But I confirmed that if you get arm64 on the fast path, that syscall
event will be missing.

Does my diagnosis sound reasonable?  I'm no arm expert, so I hesitate to
attempt patching entry.S myself, but I'd be happy to test patches.

Thanks,
Josh Stone
-------------- next part --------------
A non-text attachment was scrubbed...
Name: event-syscall-exit.c
Type: text/x-csrc
Size: 2048 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150514/7c1f6fb2/attachment.bin>

arm syscall fast path can miss a ptrace syscall-exit

[Russell King - ARM Linux]

On Thu, May 14, 2015 at 12:13:40PM -0700, Josh Stone wrote:
> I've discovered a case where both arm and arm64 will miss a ptrace
> syscall-exit that they should report.  If the syscall is entered without
> TIF_SYSCALL_TRACE set, then it goes on the fast path.  It's then
> possible to have TIF_SYSCALL_TRACE added in the middle of the syscall,
> but ret_fast_syscall doesn't check this flag again.

Yes, we assume that if TIF_SYSCALL_TRACE was not set before the call, it
isn't set after.  That appears to be an invalid assumption.

Here's a patch for ARM - untested atm.

There's still a possible hole - if we exit the syscall, then do "work"
before returning (such as reschedling to another process), and _then_
have syscall tracing enabled, we won't trace the exit.  I think that's
acceptable as I see no difference between that and having restored
state for userspace, and then immediately processing an interrupt and
scheduling on the IRQ exit path.

 arch/arm/kernel/entry-common.S | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index f8ccc21fa032..4e7f40c577e6 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -33,7 +33,9 @@ ret_fast_syscall:
  UNWIND(.fnstart	)
  UNWIND(.cantunwind	)
 	disable_irq				@ disable interrupts
-	ldr	r1, [tsk, #TI_FLAGS]
+	ldr	r1, [tsk, #TI_FLAGS]		@ re-check for syscall tracing
+	tst	r1, #_TIF_SYSCALL_WORK
+	bne	__sys_trace_return
 	tst	r1, #_TIF_WORK_MASK
 	bne	fast_work_pending
 	asm_trace_hardirqs_on


-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

arm syscall fast path can miss a ptrace syscall-exit

[Josh Stone]

On 05/14/2015 12:35 PM, Russell King - ARM Linux wrote:
> On Thu, May 14, 2015 at 12:13:40PM -0700, Josh Stone wrote:
>> I've discovered a case where both arm and arm64 will miss a ptrace
>> syscall-exit that they should report.  If the syscall is entered without
>> TIF_SYSCALL_TRACE set, then it goes on the fast path.  It's then
>> possible to have TIF_SYSCALL_TRACE added in the middle of the syscall,
>> but ret_fast_syscall doesn't check this flag again.
> 
> Yes, we assume that if TIF_SYSCALL_TRACE was not set before the call, it
> isn't set after.  That appears to be an invalid assumption.
> 
> Here's a patch for ARM - untested atm.

Thanks!  The system I have at hand is arm64, so I made the similar
change in arch/arm64/kernel/entry.S, and this passes my test.

> There's still a possible hole - if we exit the syscall, then do "work"
> before returning (such as reschedling to another process), and _then_
> have syscall tracing enabled, we won't trace the exit.  I think that's
> acceptable as I see no difference between that and having restored
> state for userspace, and then immediately processing an interrupt and
> scheduling on the IRQ exit path.

Yeah, I think that's fine.  I don't think that hole is visible to
ptrace, at least, and other tracers already have to accept this
possibility anyway.

> 
>  arch/arm/kernel/entry-common.S | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
> index f8ccc21fa032..4e7f40c577e6 100644
> --- a/arch/arm/kernel/entry-common.S
> +++ b/arch/arm/kernel/entry-common.S
> @@ -33,7 +33,9 @@ ret_fast_syscall:
>   UNWIND(.fnstart	)
>   UNWIND(.cantunwind	)
>  	disable_irq				@ disable interrupts
> -	ldr	r1, [tsk, #TI_FLAGS]
> +	ldr	r1, [tsk, #TI_FLAGS]		@ re-check for syscall tracing
> +	tst	r1, #_TIF_SYSCALL_WORK
> +	bne	__sys_trace_return
>  	tst	r1, #_TIF_WORK_MASK
>  	bne	fast_work_pending
>  	asm_trace_hardirqs_on
> 
> 

arm syscall fast path can miss a ptrace syscall-exit

[Josh Stone]

Hi Russell,

Do you plan to commit this check for syscall flags?

On 05/14/2015 02:08 PM, Josh Stone wrote:
> On 05/14/2015 12:35 PM, Russell King - ARM Linux wrote:
>> On Thu, May 14, 2015 at 12:13:40PM -0700, Josh Stone wrote:
>>> I've discovered a case where both arm and arm64 will miss a ptrace
>>> syscall-exit that they should report.  If the syscall is entered without
>>> TIF_SYSCALL_TRACE set, then it goes on the fast path.  It's then
>>> possible to have TIF_SYSCALL_TRACE added in the middle of the syscall,
>>> but ret_fast_syscall doesn't check this flag again.
>>
>> Yes, we assume that if TIF_SYSCALL_TRACE was not set before the call, it
>> isn't set after.  That appears to be an invalid assumption.
>>
>> Here's a patch for ARM - untested atm.
> 
> Thanks!  The system I have at hand is arm64, so I made the similar
> change in arch/arm64/kernel/entry.S, and this passes my test.

FWIW, here's the arm64 change I tested following your example:

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 959fe8733560..a547a3e8a198 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -608,7 +608,9 @@ ENDPROC(cpu_switch_to)
  */
 ret_fast_syscall:
 	disable_irq				// disable interrupts
-	ldr	x1, [tsk, #TI_FLAGS]
+	ldr	x1, [tsk, #TI_FLAGS]		// re-check for syscall tracing
+	and	x2, x1, #_TIF_SYSCALL_WORK
+	cbnz	x2, __sys_trace_return
 	and	x2, x1, #_TIF_WORK_MASK
 	cbnz	x2, fast_work_pending
 	enable_step_tsk x1, x2


>> There's still a possible hole - if we exit the syscall, then do "work"
>> before returning (such as reschedling to another process), and _then_
>> have syscall tracing enabled, we won't trace the exit.  I think that's
>> acceptable as I see no difference between that and having restored
>> state for userspace, and then immediately processing an interrupt and
>> scheduling on the IRQ exit path.
> 
> Yeah, I think that's fine.  I don't think that hole is visible to
> ptrace, at least, and other tracers already have to accept this
> possibility anyway.
> 
>>
>>  arch/arm/kernel/entry-common.S | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
>> index f8ccc21fa032..4e7f40c577e6 100644
>> --- a/arch/arm/kernel/entry-common.S
>> +++ b/arch/arm/kernel/entry-common.S
>> @@ -33,7 +33,9 @@ ret_fast_syscall:
>>   UNWIND(.fnstart	)
>>   UNWIND(.cantunwind	)
>>  	disable_irq				@ disable interrupts
>> -	ldr	r1, [tsk, #TI_FLAGS]
>> +	ldr	r1, [tsk, #TI_FLAGS]		@ re-check for syscall tracing
>> +	tst	r1, #_TIF_SYSCALL_WORK
>> +	bne	__sys_trace_return
>>  	tst	r1, #_TIF_WORK_MASK
>>  	bne	fast_work_pending
>>  	asm_trace_hardirqs_on
>>
>>
> 

arm syscall fast path can miss a ptrace syscall-exit

[Russell King - ARM Linux]

On Tue, May 26, 2015 at 03:38:24PM -0700, Josh Stone wrote:
> Hi Russell,
> 
> Do you plan to commit this check for syscall flags?

It's been committed since 15th May, but as it's the only fix I have
queued up (and my focus has been elsewhere) it hasn't been sent yet.
It's been in linux-next for a while now.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

arm syscall fast path can miss a ptrace syscall-exit

[Josh Stone]

On 05/28/2015 03:37 AM, Russell King - ARM Linux wrote:
> On Tue, May 26, 2015 at 03:38:24PM -0700, Josh Stone wrote:
>> Hi Russell,
>>
>> Do you plan to commit this check for syscall flags?
> 
> It's been committed since 15th May, but as it's the only fix I have
> queued up (and my focus has been elsewhere) it hasn't been sent yet.
> It's been in linux-next for a while now.

Ok, thanks, I see it there.

How about the same fix for arm64?  I see you don't work much on that, so
should I post that patch myself?

arm syscall fast path can miss a ptrace syscall-exit

[Will Deacon]

On Fri, May 29, 2015 at 09:13:18PM +0100, Josh Stone wrote:
> On 05/28/2015 03:37 AM, Russell King - ARM Linux wrote:
> > On Tue, May 26, 2015 at 03:38:24PM -0700, Josh Stone wrote:
> >> Do you plan to commit this check for syscall flags?
> > 
> > It's been committed since 15th May, but as it's the only fix I have
> > queued up (and my focus has been elsewhere) it hasn't been sent yet.
> > It's been in linux-next for a while now.
> 
> Ok, thanks, I see it there.
> 
> How about the same fix for arm64?  I see you don't work much on that, so
> should I post that patch myself?

Yes, please. If you post your arm64 patch with me and Catalin on Cc, then
we'll make sure it gets queued up.

Thanks,

Will

[PATCH] arm64: fix missing syscall trace exit

[Josh Stone]

If a syscall is entered without TIF_SYSCALL_TRACE set, then it goes on
the fast path.  It's then possible to have TIF_SYSCALL_TRACE added in
the middle of the syscall, but ret_fast_syscall doesn't check this flag
again.  This causes a ptrace syscall-exit-stop to be missed.

For instance, from a PTRACE_EVENT_FORK reported during do_fork, the
tracer might resume with PTRACE_SYSCALL, setting TIF_SYSCALL_TRACE.
Now the completion of the fork should have a syscall-exit-stop.

Russell King fixed this on arm by re-checking _TIF_SYSCALL_WORK in the
fast exit path.  Do the same on arm64.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Russell King <rmk@arm.linux.org.uk>
Signed-off-by: Josh Stone <jistone@redhat.com>
---
 arch/arm64/kernel/entry.S | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 959fe8733560..a547a3e8a198 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -608,7 +608,9 @@ ENDPROC(cpu_switch_to)
  */
 ret_fast_syscall:
 	disable_irq				// disable interrupts
-	ldr	x1, [tsk, #TI_FLAGS]
+	ldr	x1, [tsk, #TI_FLAGS]		// re-check for syscall tracing
+	and	x2, x1, #_TIF_SYSCALL_WORK
+	cbnz	x2, __sys_trace_return
 	and	x2, x1, #_TIF_WORK_MASK
 	cbnz	x2, fast_work_pending
 	enable_step_tsk x1, x2
-- 
2.4.1

[PATCH] arm64: fix missing syscall trace exit

[Josh Stone]

On 06/02/2015 06:01 PM, Josh Stone wrote:
> If a syscall is entered without TIF_SYSCALL_TRACE set, then it goes on
> the fast path.  It's then possible to have TIF_SYSCALL_TRACE added in
> the middle of the syscall, but ret_fast_syscall doesn't check this flag
> again.  This causes a ptrace syscall-exit-stop to be missed.
> 
> For instance, from a PTRACE_EVENT_FORK reported during do_fork, the
> tracer might resume with PTRACE_SYSCALL, setting TIF_SYSCALL_TRACE.
> Now the completion of the fork should have a syscall-exit-stop.
> 
> Russell King fixed this on arm by re-checking _TIF_SYSCALL_WORK in the
> fast exit path.  Do the same on arm64.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Russell King <rmk@arm.linux.org.uk>
> Signed-off-by: Josh Stone <jistone@redhat.com>
> ---
>  arch/arm64/kernel/entry.S | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 959fe8733560..a547a3e8a198 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -608,7 +608,9 @@ ENDPROC(cpu_switch_to)
>   */
>  ret_fast_syscall:
>  	disable_irq				// disable interrupts
> -	ldr	x1, [tsk, #TI_FLAGS]
> +	ldr	x1, [tsk, #TI_FLAGS]		// re-check for syscall tracing
> +	and	x2, x1, #_TIF_SYSCALL_WORK
> +	cbnz	x2, __sys_trace_return
>  	and	x2, x1, #_TIF_WORK_MASK
>  	cbnz	x2, fast_work_pending
>  	enable_step_tsk x1, x2

I do have one concern about this, also in Russell's ARM patch.  Is it
really ok to branch to __sys_trace_return with interrupts disabled?

I didn't hit any issue from that, but my testcase only exercises this
path once each run.  So that might have just been lucky not to hit any
gross scenario...

[PATCH] arm64: fix missing syscall trace exit

[Will Deacon]

On Wed, Jun 03, 2015 at 02:11:48AM +0100, Josh Stone wrote:
> On 06/02/2015 06:01 PM, Josh Stone wrote:
> > If a syscall is entered without TIF_SYSCALL_TRACE set, then it goes on
> > the fast path.  It's then possible to have TIF_SYSCALL_TRACE added in
> > the middle of the syscall, but ret_fast_syscall doesn't check this flag
> > again.  This causes a ptrace syscall-exit-stop to be missed.
> > 
> > For instance, from a PTRACE_EVENT_FORK reported during do_fork, the
> > tracer might resume with PTRACE_SYSCALL, setting TIF_SYSCALL_TRACE.
> > Now the completion of the fork should have a syscall-exit-stop.
> > 
> > Russell King fixed this on arm by re-checking _TIF_SYSCALL_WORK in the
> > fast exit path.  Do the same on arm64.
> > 
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Russell King <rmk@arm.linux.org.uk>
> > Signed-off-by: Josh Stone <jistone@redhat.com>
> > ---
> >  arch/arm64/kernel/entry.S | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> > index 959fe8733560..a547a3e8a198 100644
> > --- a/arch/arm64/kernel/entry.S
> > +++ b/arch/arm64/kernel/entry.S
> > @@ -608,7 +608,9 @@ ENDPROC(cpu_switch_to)
> >   */
> >  ret_fast_syscall:
> >  	disable_irq				// disable interrupts
> > -	ldr	x1, [tsk, #TI_FLAGS]
> > +	ldr	x1, [tsk, #TI_FLAGS]		// re-check for syscall tracing
> > +	and	x2, x1, #_TIF_SYSCALL_WORK
> > +	cbnz	x2, __sys_trace_return
> >  	and	x2, x1, #_TIF_WORK_MASK
> >  	cbnz	x2, fast_work_pending
> >  	enable_step_tsk x1, x2
> 
> I do have one concern about this, also in Russell's ARM patch.  Is it
> really ok to branch to __sys_trace_return with interrupts disabled?

I think you're right to be concerned!

> I didn't hit any issue from that, but my testcase only exercises this
> path once each run.  So that might have just been lucky not to hit any
> gross scenario...

Did you try enabling all the audit stuff? It looks like that can call
into the scheduler, so I think we should be running the tracing callbacks
with interrupts enabled (and it looks like x86 do this on the exit path).

Will

[PATCH] arm64: fix missing syscall trace exit

[Josh Stone]

On 06/03/2015 02:52 AM, Will Deacon wrote:
> On Wed, Jun 03, 2015 at 02:11:48AM +0100, Josh Stone wrote:
>> On 06/02/2015 06:01 PM, Josh Stone wrote:
>>>  ret_fast_syscall:
>>>  	disable_irq				// disable interrupts
>>> -	ldr	x1, [tsk, #TI_FLAGS]
>>> +	ldr	x1, [tsk, #TI_FLAGS]		// re-check for syscall tracing
>>> +	and	x2, x1, #_TIF_SYSCALL_WORK
>>> +	cbnz	x2, __sys_trace_return
>>>  	and	x2, x1, #_TIF_WORK_MASK
>>>  	cbnz	x2, fast_work_pending
>>>  	enable_step_tsk x1, x2
>>
>> I do have one concern about this, also in Russell's ARM patch.  Is it
>> really ok to branch to __sys_trace_return with interrupts disabled?
> 
> I think you're right to be concerned!
> 
>> I didn't hit any issue from that, but my testcase only exercises this
>> path once each run.  So that might have just been lucky not to hit any
>> gross scenario...
> 
> Did you try enabling all the audit stuff? It looks like that can call
> into the scheduler, so I think we should be running the tracing callbacks
> with interrupts enabled (and it looks like x86 do this on the exit path).

This particular path only applies if you entered the syscall *without*
any tracing, which is what makes it pretty much a oneshot.  You'd have
to arrange for audit enabling in the middle of a syscall to see it.  My
ptrace test is easier because working from PTRACE_EVENT_FORK is always
in the middle of the fork syscall.

But anyway, I agree interrupts should be enabled -- I'll work on this.
Then after __sys_trace_return jumps to ret_from_user, they'll be
disabled again.  Likewise for arm32 jumping to ret_slow_syscall.

[PATCH] arm64: fix missing syscall trace exit

[Russell King - ARM Linux]

On Tue, Jun 02, 2015 at 06:11:48PM -0700, Josh Stone wrote:
> On 06/02/2015 06:01 PM, Josh Stone wrote:
> > If a syscall is entered without TIF_SYSCALL_TRACE set, then it goes on
> > the fast path.  It's then possible to have TIF_SYSCALL_TRACE added in
> > the middle of the syscall, but ret_fast_syscall doesn't check this flag
> > again.  This causes a ptrace syscall-exit-stop to be missed.
> > 
> > For instance, from a PTRACE_EVENT_FORK reported during do_fork, the
> > tracer might resume with PTRACE_SYSCALL, setting TIF_SYSCALL_TRACE.
> > Now the completion of the fork should have a syscall-exit-stop.
> > 
> > Russell King fixed this on arm by re-checking _TIF_SYSCALL_WORK in the
> > fast exit path.  Do the same on arm64.
> > 
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Russell King <rmk@arm.linux.org.uk>
> > Signed-off-by: Josh Stone <jistone@redhat.com>
> > ---
> >  arch/arm64/kernel/entry.S | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> > index 959fe8733560..a547a3e8a198 100644
> > --- a/arch/arm64/kernel/entry.S
> > +++ b/arch/arm64/kernel/entry.S
> > @@ -608,7 +608,9 @@ ENDPROC(cpu_switch_to)
> >   */
> >  ret_fast_syscall:
> >  	disable_irq				// disable interrupts
> > -	ldr	x1, [tsk, #TI_FLAGS]
> > +	ldr	x1, [tsk, #TI_FLAGS]		// re-check for syscall tracing
> > +	and	x2, x1, #_TIF_SYSCALL_WORK
> > +	cbnz	x2, __sys_trace_return
> >  	and	x2, x1, #_TIF_WORK_MASK
> >  	cbnz	x2, fast_work_pending
> >  	enable_step_tsk x1, x2
> 
> I do have one concern about this, also in Russell's ARM patch.  Is it
> really ok to branch to __sys_trace_return with interrupts disabled?

I'm not that happy to hear that you have concerns over the patch after
hurrying its submission into the -rc kernels.

> I didn't hit any issue from that, but my testcase only exercises this
> path once each run.  So that might have just been lucky not to hit any
> gross scenario...

It would've been good to have tested that _prior_ to me pushing the patch
into mainline and having the stable trees pick it up.  This kind of thing
can potentially de-stabilise the kernel.

I had thought you'd have tested with audit and other stuff enabled (I
don't use that stuff, and I'm clueless about how to use it.)

Surely, if you're tracing a child, and you start tracing on the exit
path of a syscall, the child should sleep - and as sleeping with IRQs
disabled is not allowed, there should've been a warning if this path
was hit.  I think this brings into question whether that path was
actually hit during testing.  I hope you tried running a kernel with
the usual suite of debugging options enabled?

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH] arm64: fix missing syscall trace exit

[Josh Stone]

On 06/04/2015 03:06 AM, Russell King - ARM Linux wrote:
> On Tue, Jun 02, 2015 at 06:11:48PM -0700, Josh Stone wrote:
>> On 06/02/2015 06:01 PM, Josh Stone wrote:
>>> If a syscall is entered without TIF_SYSCALL_TRACE set, then it goes on
>>> the fast path.  It's then possible to have TIF_SYSCALL_TRACE added in
>>> the middle of the syscall, but ret_fast_syscall doesn't check this flag
>>> again.  This causes a ptrace syscall-exit-stop to be missed.
>>>
>>> For instance, from a PTRACE_EVENT_FORK reported during do_fork, the
>>> tracer might resume with PTRACE_SYSCALL, setting TIF_SYSCALL_TRACE.
>>> Now the completion of the fork should have a syscall-exit-stop.
>>>
>>> Russell King fixed this on arm by re-checking _TIF_SYSCALL_WORK in the
>>> fast exit path.  Do the same on arm64.
>>>
>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>> Cc: Will Deacon <will.deacon@arm.com>
>>> Cc: Russell King <rmk@arm.linux.org.uk>
>>> Signed-off-by: Josh Stone <jistone@redhat.com>
>>> ---
>>>  arch/arm64/kernel/entry.S | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>>> index 959fe8733560..a547a3e8a198 100644
>>> --- a/arch/arm64/kernel/entry.S
>>> +++ b/arch/arm64/kernel/entry.S
>>> @@ -608,7 +608,9 @@ ENDPROC(cpu_switch_to)
>>>   */
>>>  ret_fast_syscall:
>>>  	disable_irq				// disable interrupts
>>> -	ldr	x1, [tsk, #TI_FLAGS]
>>> +	ldr	x1, [tsk, #TI_FLAGS]		// re-check for syscall tracing
>>> +	and	x2, x1, #_TIF_SYSCALL_WORK
>>> +	cbnz	x2, __sys_trace_return
>>>  	and	x2, x1, #_TIF_WORK_MASK
>>>  	cbnz	x2, fast_work_pending
>>>  	enable_step_tsk x1, x2
>>
>> I do have one concern about this, also in Russell's ARM patch.  Is it
>> really ok to branch to __sys_trace_return with interrupts disabled?
> 
> I'm not that happy to hear that you have concerns over the patch after
> hurrying its submission into the -rc kernels.

I simply didn't notice before that disable_irq might be an issue.
Sorry.  I haven't actually encountered any problem, just in theory.

>> I didn't hit any issue from that, but my testcase only exercises this
>> path once each run.  So that might have just been lucky not to hit any
>> gross scenario...
> 
> It would've been good to have tested that _prior_ to me pushing the patch
> into mainline and having the stable trees pick it up.  This kind of thing
> can potentially de-stabilise the kernel.

I never said I tested ARM.  I did test ARM64 with my version of the
patch, and it had no issue that I could see at runtime.

But of course I agree destabilizing is bad -- this is why I spoke up
when I did notice this as a potential problem.

> I had thought you'd have tested with audit and other stuff enabled (I
> don't use that stuff, and I'm clueless about how to use it.)

If you have audit enabled, you'll *never* reach ret_fast_syscall, you'll
get to sys_trace on entry.  If you *ever* had audit enabled since boot,
audit_alloc() sets TIF_SYSCALL_AUDIT on every task that's not explicitly
filtered.  AFAICS, audit_alloc() is the only way to set that flag,
during copy_process(), so it'll never be mid-syscall anyway.

But TIF_SYSCALL_TRACE via PTRACE_SYSCALL is more dynamic, and that's
where I noticed the original problem and how I wrote my test.  See my
original mail attachment for that test if you want to try it.

> Surely, if you're tracing a child, and you start tracing on the exit
> path of a syscall, the child should sleep - and as sleeping with IRQs
> disabled is not allowed, there should've been a warning if this path
> was hit.  I think this brings into question whether that path was
> actually hit during testing.  I hope you tried running a kernel with
> the usual suite of debugging options enabled?

Surely it should sleep, yes -- in my test it hits a ptrace stop.

Whether that exact path is reached -- I think so.  I ran my test on a
distro kernel to see the failure, then applied only this fix and ran
again, could no longer see failure.

I can try a systemtap or ftrace kprobe on ret_fast_syscall to be sure
that path is reached.

Because I was working from a distro kernel, it didn't have debugging
options, no.  I'll go run that now, including both arm and arm64 if I
can find available systems...

[PATCH] arm64: fix missing syscall trace exit

[Josh Stone]

On 06/04/2015 10:14 AM, Josh Stone wrote:
> Whether that exact path is reached -- I think so.  I ran my test on a
> distro kernel to see the failure, then applied only this fix and ran
> again, could no longer see failure.
> 
> I can try a systemtap or ftrace kprobe on ret_fast_syscall to be sure
> that path is reached.

I haven't gotten an ARM system yet.  ARM64 doesn't have kprobes, so I
can't directly probe ret_fast_syscall, but I did use the sched_switch
tracepoint to confirm that the thread_info flags didn't contain any
tracing flags until the moment I set PTRACE_SYSCALL, so it ought to be
on that fast path.

Plus I confirmed again that without those two lines in ret_fast_syscall
it still fails my test, and succeeds with them, so I can't see how it
could be anything but this path.

> Because I was working from a distro kernel, it didn't have debugging
> options, no.  I'll go run that now, including both arm and arm64 if I
> can find available systems...

Now I booted an arm64 machine into kernel v4.1-rc6-52-gff25ea8 plus my
__sys_trace_return patch, and I enabled debug options.  I get a lot of
might_sleep errors from xgbe_tx_timer (below), but this happens even
without my patch.

I get no kernel debug errors or warnings from my test, except that it
records the SIGSEGV termination (which is by design).  I even threw a
loop inside the test to hammer it more -- all clean.

Still, I don't think it's right to trace with interrupts disabled, so
here's a version which enables them again first.  How does this look?

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 959fe87..988bea4 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -608,11 +608,16 @@ ENDPROC(cpu_switch_to)
  */
 ret_fast_syscall:
        disable_irq                             // disable interrupts
-       ldr     x1, [tsk, #TI_FLAGS]
+       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
        enable_step_tsk x1, x2
        kernel_exit 0, ret = 1
+ret_fast_syscall_trace:
+       enable_irq                              // enable interrupts
+       b       __sys_trace_return
 
 /*
  * Ok, we need to do extra processing, enter the slow path.


===

Off topic, here's the xgbe_tx_timer BUG.  As I mentioned, this happens
even on a vanilla kernel, before I've run any of my tests.  I think this
is a legitimate bug to call disable_irq() from a softirq timer.

  disable_irq -> synchronize_irq -> wait_event -> might_sleep

[   19.841911] BUG: sleeping function called from invalid context at kernel/irq/manage.c:110
[   19.850081] in_atomic(): 1, irqs_disabled(): 0, pid: 1133, name: sed
[   19.856434] INFO: lockdep is turned off.
[   19.860349] CPU: 3 PID: 1133 Comm: sed Tainted: G        W       4.1.0-rc6-orig+ #5
[   19.867999] Hardware name: Default string Default string/Default string, BIOS ROD0080B 03/18/2015
[   19.876865] Call trace:
[   19.879306] [<fffffe0000096e08>] dump_backtrace+0x0/0x16c
[   19.884702] [<fffffe0000096f84>] show_stack+0x10/0x1c
[   19.889746] [<fffffe000071408c>] dump_stack+0x84/0xbc
[   19.894796] [<fffffe00000eab28>] ___might_sleep+0x15c/0x248
[   19.900360] [<fffffe00000ead7c>] __might_sleep+0x54/0x94
[   19.905685] [<fffffe0000121ee4>] synchronize_irq+0x2c/0x88
[   19.911167] [<fffffe00001221ec>] disable_irq+0x1c/0x2c
[   19.916304] [<fffffe00004d8fec>] xgbe_tx_timer+0x64/0x90
[   19.921606] [<fffffe00001344b4>] call_timer_fn+0xd4/0x48c
[   19.927002] [<fffffe00001351fc>] run_timer_softirq+0x240/0x440
[   19.932833] [<fffffe00000c5014>] __do_softirq+0x118/0x608
[   19.938221] [<fffffe00000c584c>] irq_exit+0x9c/0xdc
[   19.943094] [<fffffe0000121200>] __handle_domain_irq+0x68/0xb4
[   19.948916] [<fffffe0000090398>] gic_handle_irq+0x30/0x7c
[   19.954310] Exception stack(0xfffffe001ef4bb20 to 0xfffffe001ef4bc40)
[   19.960742] bb20: d6b401c0 fffffe03 01ea7600 fffffe00 1ef4bc60 fffffe00 00117a80 fffffe00
[   19.968913] bb40: d6b401f0 fffffe03 0095f450 fffffe00 01ea7610 fffffe00 00000000 00000000
[   19.977088] bb60: 1ef4bc10 fffffe00 1ef4bbe0 fffffe00 0109d468 fffffe00 00000000 00000000
[   19.985262] bb80: d4a96880 fffffe03 00000000 00000000 1ef4bc80 fffffe00 00000000 756e694c
[   19.993436] bba0: 00000006 00000004 00010000 00000000 98343c1c 000003ff 98343b44 000003ff
[   20.001603] bbc0: 00226d00 fffffe00 983440dc 000003ff 00000000 00000000 d6b401c0 fffffe03
[   20.009776] bbe0: 01ea7600 fffffe00 00fc0000 fffffe00 00fc2000 fffffe00 00fff3a0 fffffe00
[   20.017950] bc00: 00000015 00000000 0000011a 00000000 00000038 00000000 00742000 fffffe00
[   20.026125] bc20: 1ef48000 fffffe00 1ef4bc60 fffffe00 00228dd4 fffffe00 1ef4bc60 fffffe00
[   20.034299] [<fffffe0000092ce8>] el1_irq+0x68/0x100
[   20.039173] [<fffffe0000228dd0>] get_empty_filp+0xf8/0x1ec
[   20.044658] [<fffffe00002363ec>] path_openat+0x34/0x644
[   20.049874] [<fffffe0000237b18>] do_filp_open+0x2c/0x88
[   20.055095] [<fffffe0000226bf8>] do_sys_open+0x13c/0x21c
[   20.060397] [<fffffe0000226d0c>] SyS_openat+0xc/0x18

[PATCH] arm64: fix missing syscall trace exit

[Will Deacon]

On Fri, Jun 05, 2015 at 12:17:56AM +0100, Josh Stone wrote:
> Still, I don't think it's right to trace with interrupts disabled, so
> here's a version which enables them again first.  How does this look?
> 
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 959fe87..988bea4 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -608,11 +608,16 @@ ENDPROC(cpu_switch_to)
>   */
>  ret_fast_syscall:
>         disable_irq                             // disable interrupts
> -       ldr     x1, [tsk, #TI_FLAGS]
> +       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
>         enable_step_tsk x1, x2
>         kernel_exit 0, ret = 1
> +ret_fast_syscall_trace:
> +       enable_irq                              // enable interrupts
> +       b       __sys_trace_return
>  
>  /*
>   * Ok, we need to do extra processing, enter the slow path.

Looks good to me. Can you post as a stand-alone patch, with commit message
etc please?

> Off topic, here's the xgbe_tx_timer BUG.  As I mentioned, this happens
> even on a vanilla kernel, before I've run any of my tests.  I think this
> is a legitimate bug to call disable_irq() from a softirq timer.

Adding Tom, as he maintains the AMD xgbe driver (assuming that's what is
exploding here). I'm guessing you need s/disable_irq/disable_irq_nosync/.

Will

>   disable_irq -> synchronize_irq -> wait_event -> might_sleep
> 
> [   19.841911] BUG: sleeping function called from invalid context at kernel/irq/manage.c:110
> [   19.850081] in_atomic(): 1, irqs_disabled(): 0, pid: 1133, name: sed
> [   19.856434] INFO: lockdep is turned off.
> [   19.860349] CPU: 3 PID: 1133 Comm: sed Tainted: G        W       4.1.0-rc6-orig+ #5
> [   19.867999] Hardware name: Default string Default string/Default string, BIOS ROD0080B 03/18/2015
> [   19.876865] Call trace:
> [   19.879306] [<fffffe0000096e08>] dump_backtrace+0x0/0x16c
> [   19.884702] [<fffffe0000096f84>] show_stack+0x10/0x1c
> [   19.889746] [<fffffe000071408c>] dump_stack+0x84/0xbc
> [   19.894796] [<fffffe00000eab28>] ___might_sleep+0x15c/0x248
> [   19.900360] [<fffffe00000ead7c>] __might_sleep+0x54/0x94
> [   19.905685] [<fffffe0000121ee4>] synchronize_irq+0x2c/0x88
> [   19.911167] [<fffffe00001221ec>] disable_irq+0x1c/0x2c
> [   19.916304] [<fffffe00004d8fec>] xgbe_tx_timer+0x64/0x90
> [   19.921606] [<fffffe00001344b4>] call_timer_fn+0xd4/0x48c
> [   19.927002] [<fffffe00001351fc>] run_timer_softirq+0x240/0x440
> [   19.932833] [<fffffe00000c5014>] __do_softirq+0x118/0x608
> [   19.938221] [<fffffe00000c584c>] irq_exit+0x9c/0xdc
> [   19.943094] [<fffffe0000121200>] __handle_domain_irq+0x68/0xb4
> [   19.948916] [<fffffe0000090398>] gic_handle_irq+0x30/0x7c
> [   19.954310] Exception stack(0xfffffe001ef4bb20 to 0xfffffe001ef4bc40)
> [   19.960742] bb20: d6b401c0 fffffe03 01ea7600 fffffe00 1ef4bc60 fffffe00 00117a80 fffffe00
> [   19.968913] bb40: d6b401f0 fffffe03 0095f450 fffffe00 01ea7610 fffffe00 00000000 00000000
> [   19.977088] bb60: 1ef4bc10 fffffe00 1ef4bbe0 fffffe00 0109d468 fffffe00 00000000 00000000
> [   19.985262] bb80: d4a96880 fffffe03 00000000 00000000 1ef4bc80 fffffe00 00000000 756e694c
> [   19.993436] bba0: 00000006 00000004 00010000 00000000 98343c1c 000003ff 98343b44 000003ff
> [   20.001603] bbc0: 00226d00 fffffe00 983440dc 000003ff 00000000 00000000 d6b401c0 fffffe03
> [   20.009776] bbe0: 01ea7600 fffffe00 00fc0000 fffffe00 00fc2000 fffffe00 00fff3a0 fffffe00
> [   20.017950] bc00: 00000015 00000000 0000011a 00000000 00000038 00000000 00742000 fffffe00
> [   20.026125] bc20: 1ef48000 fffffe00 1ef4bc60 fffffe00 00228dd4 fffffe00 1ef4bc60 fffffe00
> [   20.034299] [<fffffe0000092ce8>] el1_irq+0x68/0x100
> [   20.039173] [<fffffe0000228dd0>] get_empty_filp+0xf8/0x1ec
> [   20.044658] [<fffffe00002363ec>] path_openat+0x34/0x644
> [   20.049874] [<fffffe0000237b18>] do_filp_open+0x2c/0x88
> [   20.055095] [<fffffe0000226bf8>] do_sys_open+0x13c/0x21c
> [   20.060397] [<fffffe0000226d0c>] SyS_openat+0xc/0x18
> 
> 
> 

[PATCH] arm64: fix missing syscall trace exit

[Tom Lendacky]

On 06/05/2015 10:38 AM, Will Deacon wrote:
> On Fri, Jun 05, 2015 at 12:17:56AM +0100, Josh Stone wrote:
>> Still, I don't think it's right to trace with interrupts disabled, so
>> here's a version which enables them again first.  How does this look?
>>
>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>> index 959fe87..988bea4 100644
>> --- a/arch/arm64/kernel/entry.S
>> +++ b/arch/arm64/kernel/entry.S
>> @@ -608,11 +608,16 @@ ENDPROC(cpu_switch_to)
>>    */
>>   ret_fast_syscall:
>>          disable_irq                             // disable interrupts
>> -       ldr     x1, [tsk, #TI_FLAGS]
>> +       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
>>          enable_step_tsk x1, x2
>>          kernel_exit 0, ret = 1
>> +ret_fast_syscall_trace:
>> +       enable_irq                              // enable interrupts
>> +       b       __sys_trace_return
>>
>>   /*
>>    * Ok, we need to do extra processing, enter the slow path.
>
> Looks good to me. Can you post as a stand-alone patch, with commit message
> etc please?
>
>> Off topic, here's the xgbe_tx_timer BUG.  As I mentioned, this happens
>> even on a vanilla kernel, before I've run any of my tests.  I think this
>> is a legitimate bug to call disable_irq() from a softirq timer.
>
> Adding Tom, as he maintains the AMD xgbe driver (assuming that's what is
> exploding here). I'm guessing you need s/disable_irq/disable_irq_nosync/.
>

Yup, that would be the problem. I'll submit a fix to the netdev mailing
list.

Josh, are you ok if I include you on a reported-by in the patch?

Thanks,
Tom

> Will
>
>>    disable_irq -> synchronize_irq -> wait_event -> might_sleep
>>
>> [   19.841911] BUG: sleeping function called from invalid context at kernel/irq/manage.c:110
>> [   19.850081] in_atomic(): 1, irqs_disabled(): 0, pid: 1133, name: sed
>> [   19.856434] INFO: lockdep is turned off.
>> [   19.860349] CPU: 3 PID: 1133 Comm: sed Tainted: G        W       4.1.0-rc6-orig+ #5
>> [   19.867999] Hardware name: Default string Default string/Default string, BIOS ROD0080B 03/18/2015
>> [   19.876865] Call trace:
>> [   19.879306] [<fffffe0000096e08>] dump_backtrace+0x0/0x16c
>> [   19.884702] [<fffffe0000096f84>] show_stack+0x10/0x1c
>> [   19.889746] [<fffffe000071408c>] dump_stack+0x84/0xbc
>> [   19.894796] [<fffffe00000eab28>] ___might_sleep+0x15c/0x248
>> [   19.900360] [<fffffe00000ead7c>] __might_sleep+0x54/0x94
>> [   19.905685] [<fffffe0000121ee4>] synchronize_irq+0x2c/0x88
>> [   19.911167] [<fffffe00001221ec>] disable_irq+0x1c/0x2c
>> [   19.916304] [<fffffe00004d8fec>] xgbe_tx_timer+0x64/0x90
>> [   19.921606] [<fffffe00001344b4>] call_timer_fn+0xd4/0x48c
>> [   19.927002] [<fffffe00001351fc>] run_timer_softirq+0x240/0x440
>> [   19.932833] [<fffffe00000c5014>] __do_softirq+0x118/0x608
>> [   19.938221] [<fffffe00000c584c>] irq_exit+0x9c/0xdc
>> [   19.943094] [<fffffe0000121200>] __handle_domain_irq+0x68/0xb4
>> [   19.948916] [<fffffe0000090398>] gic_handle_irq+0x30/0x7c
>> [   19.954310] Exception stack(0xfffffe001ef4bb20 to 0xfffffe001ef4bc40)
>> [   19.960742] bb20: d6b401c0 fffffe03 01ea7600 fffffe00 1ef4bc60 fffffe00 00117a80 fffffe00
>> [   19.968913] bb40: d6b401f0 fffffe03 0095f450 fffffe00 01ea7610 fffffe00 00000000 00000000
>> [   19.977088] bb60: 1ef4bc10 fffffe00 1ef4bbe0 fffffe00 0109d468 fffffe00 00000000 00000000
>> [   19.985262] bb80: d4a96880 fffffe03 00000000 00000000 1ef4bc80 fffffe00 00000000 756e694c
>> [   19.993436] bba0: 00000006 00000004 00010000 00000000 98343c1c 000003ff 98343b44 000003ff
>> [   20.001603] bbc0: 00226d00 fffffe00 983440dc 000003ff 00000000 00000000 d6b401c0 fffffe03
>> [   20.009776] bbe0: 01ea7600 fffffe00 00fc0000 fffffe00 00fc2000 fffffe00 00fff3a0 fffffe00
>> [   20.017950] bc00: 00000015 00000000 0000011a 00000000 00000038 00000000 00742000 fffffe00
>> [   20.026125] bc20: 1ef48000 fffffe00 1ef4bc60 fffffe00 00228dd4 fffffe00 1ef4bc60 fffffe00
>> [   20.034299] [<fffffe0000092ce8>] el1_irq+0x68/0x100
>> [   20.039173] [<fffffe0000228dd0>] get_empty_filp+0xf8/0x1ec
>> [   20.044658] [<fffffe00002363ec>] path_openat+0x34/0x644
>> [   20.049874] [<fffffe0000237b18>] do_filp_open+0x2c/0x88
>> [   20.055095] [<fffffe0000226bf8>] do_sys_open+0x13c/0x21c
>> [   20.060397] [<fffffe0000226d0c>] SyS_openat+0xc/0x18
>>
>>
>>

[PATCH] arm64: fix missing syscall trace exit

[Josh Stone]

If a syscall is entered without TIF_SYSCALL_TRACE set, then it goes on
the fast path.  It's then possible to have TIF_SYSCALL_TRACE added in
the middle of the syscall, but ret_fast_syscall doesn't check this flag
again.  This causes a ptrace syscall-exit-stop to be missed.

For instance, from a PTRACE_EVENT_FORK reported during do_fork, the
tracer might resume with PTRACE_SYSCALL, setting TIF_SYSCALL_TRACE.
Now the completion of the fork should have a syscall-exit-stop.

Russell King fixed this on arm by re-checking _TIF_SYSCALL_WORK in the
fast exit path.  Do the same on arm64.

v2: Re-enable interrupts before branching to __sys_trace_return.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Josh Stone <jistone@redhat.com>
---
 arch/arm64/kernel/entry.S | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 959fe8733560..988bea43dc74 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -608,11 +608,16 @@ ENDPROC(cpu_switch_to)
  */
 ret_fast_syscall:
 	disable_irq				// disable interrupts
-	ldr	x1, [tsk, #TI_FLAGS]
+	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
 	enable_step_tsk x1, x2
 	kernel_exit 0, ret = 1
+ret_fast_syscall_trace:
+	enable_irq				// enable interrupts
+	b	__sys_trace_return
 
 /*
  * Ok, we need to do extra processing, enter the slow path.
-- 
2.4.1

[PATCH] arm64: fix missing syscall trace exit

[Will Deacon]

Hi Josh,

Thanks for the patch.

On Fri, Jun 05, 2015 at 10:28:03PM +0100, Josh Stone wrote:
> If a syscall is entered without TIF_SYSCALL_TRACE set, then it goes on
> the fast path.  It's then possible to have TIF_SYSCALL_TRACE added in
> the middle of the syscall, but ret_fast_syscall doesn't check this flag
> again.  This causes a ptrace syscall-exit-stop to be missed.
> 
> For instance, from a PTRACE_EVENT_FORK reported during do_fork, the
> tracer might resume with PTRACE_SYSCALL, setting TIF_SYSCALL_TRACE.
> Now the completion of the fork should have a syscall-exit-stop.
> 
> Russell King fixed this on arm by re-checking _TIF_SYSCALL_WORK in the
> fast exit path.  Do the same on arm64.
> 
> v2: Re-enable interrupts before branching to __sys_trace_return.

Please don't include the changelog in the commit message.

> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> Signed-off-by: Josh Stone <jistone@redhat.com>
> ---
>  arch/arm64/kernel/entry.S | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 959fe8733560..988bea43dc74 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -608,11 +608,16 @@ ENDPROC(cpu_switch_to)
>   */
>  ret_fast_syscall:
>  	disable_irq				// disable interrupts
> -	ldr	x1, [tsk, #TI_FLAGS]
> +	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
>  	enable_step_tsk x1, x2
>  	kernel_exit 0, ret = 1
> +ret_fast_syscall_trace:
> +	enable_irq				// enable interrupts
> +	b	__sys_trace_return

Looks good to me:

  Reviewed-by: Will Deacon <will.deacon@arm.com>

I assume this can wait until 4.2?

Will

[PATCH] arm64: fix missing syscall trace exit

[Josh Stone]

On 06/08/2015 03:21 AM, Will Deacon wrote:
>> v2: Re-enable interrupts before branching to __sys_trace_return.
> 
> Please don't include the changelog in the commit message.

Ok - do you need me to resend without it?

> I assume this can wait until 4.2?

That's fine with me, thanks.

[PATCH] arm64: fix missing syscall trace exit

[Catalin Marinas]

On Mon, Jun 08, 2015 at 09:37:32AM -0700, Josh Stone wrote:
> On 06/08/2015 03:21 AM, Will Deacon wrote:
> >> v2: Re-enable interrupts before branching to __sys_trace_return.
> > 
> > Please don't include the changelog in the commit message.
> 
> Ok - do you need me to resend without it?

No need to. I'll edit the line out and queue the patch for 4.2.

Thanks.

-- 
Catalin

[PATCH] ARM: enable_irq before ret_fast_syscall tracing

[Josh Stone]

When reached via the slow path __sys_trace, __sys_trace_return and its
callees usually have interrupts still enabled.  This is important if any
will schedule, like for a ptrace syscall-exit-stop.

In the rarer case where tracing was not enabled on syscall entry, and
then ret_fast_syscall sees tracing was enabled mid-syscall, then it
also ought to branch to __sys_trace_return with interrupts enabled.

Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Josh Stone <jistone@redhat.com>
---
 arch/arm/kernel/entry-common.S | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index 4e7f40c577e6..5d8eb11b8571 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -35,7 +35,7 @@ ret_fast_syscall:
 	disable_irq				@ disable interrupts
 	ldr	r1, [tsk, #TI_FLAGS]		@ re-check for syscall tracing
 	tst	r1, #_TIF_SYSCALL_WORK
-	bne	__sys_trace_return
+	bne	ret_fast_syscall_trace
 	tst	r1, #_TIF_WORK_MASK
 	bne	fast_work_pending
 	asm_trace_hardirqs_on
@@ -45,6 +45,10 @@ ret_fast_syscall:
 	ct_user_enter
 
 	restore_user_regs fast = 1, offset = S_OFF
+
+ret_fast_syscall_trace:
+	enable_irq				@ enable interrupts
+	b	__sys_trace_return
  UNWIND(.fnend		)
 
 /*
-- 
2.4.2

[PATCH] ARM: enable_irq before ret_fast_syscall tracing

[Josh Stone]

On 06/22/2015 05:08 PM, Josh Stone wrote:
> When reached via the slow path __sys_trace, __sys_trace_return and its
> callees usually have interrupts still enabled.  This is important if any
> will schedule, like for a ptrace syscall-exit-stop.
> 
> In the rarer case where tracing was not enabled on syscall entry, and
> then ret_fast_syscall sees tracing was enabled mid-syscall, then it
> also ought to branch to __sys_trace_return with interrupts enabled.

Side note -- I haven't actually found any hard evidence that disabled
interrupts here are a problem, even though it seems obviously bad.

I used kprobes to confirm that I am indeed reaching this case, and
modified my original testcase to spam this scenario in a loop.  But I
never encountered any instability or debug messages about interrupts.

Still, it also runs cleanly with this patch, and I think this is more
correct.  Please correct me if I'm wrong!

> Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> Signed-off-by: Josh Stone <jistone@redhat.com>
> ---
>  arch/arm/kernel/entry-common.S | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
> index 4e7f40c577e6..5d8eb11b8571 100644
> --- a/arch/arm/kernel/entry-common.S
> +++ b/arch/arm/kernel/entry-common.S
> @@ -35,7 +35,7 @@ ret_fast_syscall:
>  	disable_irq				@ disable interrupts
>  	ldr	r1, [tsk, #TI_FLAGS]		@ re-check for syscall tracing
>  	tst	r1, #_TIF_SYSCALL_WORK
> -	bne	__sys_trace_return
> +	bne	ret_fast_syscall_trace
>  	tst	r1, #_TIF_WORK_MASK
>  	bne	fast_work_pending
>  	asm_trace_hardirqs_on
> @@ -45,6 +45,10 @@ ret_fast_syscall:
>  	ct_user_enter
>  
>  	restore_user_regs fast = 1, offset = S_OFF
> +
> +ret_fast_syscall_trace:
> +	enable_irq				@ enable interrupts
> +	b	__sys_trace_return
>   UNWIND(.fnend		)
>  
>  /*
>