arm syscall fast path can miss a ptrace syscall-exit
Subsystems (2)
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>
Quoted 5 lines · Josh Stone
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_TRACEset, then it goes on the fast path. It's then possible to haveTIF_SYSCALL_TRACEadded in the middle of the syscall, butret_fast_syscalldoesn'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(-)
arch/arm/kernel/entry-common.S +3 -1
@@ -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
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
Quoted 11 lines · Russell King - ARM Linux
Quoted 5 lines · Josh Stone
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_TRACEset, then it goes on the fast path. It's then possible to haveTIF_SYSCALL_TRACEadded in the middle of the syscall, butret_fast_syscalldoesn't check this flag again.Yes, we assume that if
TIF_SYSCALL_TRACEwas 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.
Quoted 6 lines
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.
Quoted 21 lines
arch/arm/kernel/entry-common.S | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)arch/arm/kernel/entry-common.S +3
-1@@ -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
Hi Russell,
Do you plan to commit this check for syscall flags?
Quoted 8 lines · Josh Stone
Quoted 7 lines · Russell King - ARM Linux
Quoted 5 lines · Josh Stone
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_TRACEset, then it goes on the fast path. It's then possible to haveTIF_SYSCALL_TRACEadded in the middle of the syscall, butret_fast_syscalldoesn't check this flag again.
Quoted 7 lines
Quoted 4 lines
Yes, we assume that if
TIF_SYSCALL_TRACEwas 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:
arch/arm64/kernel/entry.S +3 -1
@@ -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
Quoted 33 lines
Quoted 6 lines
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.
Quoted 21 lines
arch/arm/kernel/entry-common.S | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)arch/arm/kernel/entry-common.S +3
-1@@ -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
Quoted 3 lines · Josh Stone
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.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
Quoted 8 lines · Russell King - ARM Linux
Quoted 3 lines · Josh Stone
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?
Quoted 4 lines · Josh Stone
Quoted 3 lines · Russell King - ARM Linux
Quoted 1 line · Josh Stone
Do you plan to commit this check for syscall flags?
Quoted 8 lines
Quoted 3 lines
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 1/1 arm64: fix missing syscall trace exit
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.
--- arch/arm64/kernel/entry.S | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
arch/arm64/kernel/entry.S +3 -1
@@ -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
Signature · 1 line
2.4.1
PATCH 1/1 arm64: fix missing syscall trace exit
Quoted 25 lines · Josh Stone
If a syscall is entered without
TIF_SYSCALL_TRACEset, then it goes on the fast path. It's then possible to haveTIF_SYSCALL_TRACEadded in the middle of the syscall, butret_fast_syscalldoesn't check this flag again. This causes a ptrace syscall-exit-stop to be missed.For instance, from a
PTRACE_EVENT_FORKreported duringdo_fork, the tracer might resume withPTRACE_SYSCALL, settingTIF_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_WORKin the fast exit path. Do the same on arm64.Cc Catalin Marinas <[email protected]>Cc Will Deacon <[email protected]>Cc Russell King <[email protected]>Signed-off-by Josh Stone <[email protected]>--- arch/arm64/kernel/entry.S | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)arch/arm64/kernel/entry.S +0
-0
arch/arm64/kernel/entry.S ENDPROC(cpu_switch_to) · line 616
Quoted 10 lines
arch/arm64/kernel/entry.S +3
-1@@ -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 1/1 arm64: fix missing syscall trace exit
Quoted 31 lines · Josh Stone
Quoted 30 lines · Josh Stone
If a syscall is entered without
TIF_SYSCALL_TRACEset, then it goes on the fast path. It's then possible to haveTIF_SYSCALL_TRACEadded in the middle of the syscall, butret_fast_syscalldoesn't check this flag again. This causes a ptrace syscall-exit-stop to be missed.For instance, from a
PTRACE_EVENT_FORKreported duringdo_fork, the tracer might resume withPTRACE_SYSCALL, settingTIF_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_WORKin the fast exit path. Do the same on arm64.Cc Catalin Marinas <[email protected]>Cc Will Deacon <[email protected]>Cc Russell King <[email protected]>Signed-off-by Josh Stone <[email protected]>--- arch/arm64/kernel/entry.S | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)arch/arm64/kernel/entry.S +1
-1@@ -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
Quoted 8 lines
arch/arm64/kernel/entry.S ENDPROC(cpu_switch_to) · line 616
Quoted 5 lines
arch/arm64/kernel/entry.S +2
-0+ 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, x2I do have one concern about this, also in Russell's ARM patch. Is it really ok to branch to
__sys_trace_returnwith interrupts disabled?
I think you're right to be concerned!
Quoted 3 lines
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 1/1 arm64: fix missing syscall trace exit
Quoted 17 lines · Will Deacon
Quoted 13 lines · Josh Stone
arch/arm64/kernel/entry.S ENDPROC(cpu_switch_to) · line 616
Quoted 9 lines · Josh Stone
ret_fast_syscall:arch/arm64/kernel/entry.S +3
-1disable_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, x2I do have one concern about this, also in Russell's ARM patch. Is it really ok to branch to
__sys_trace_returnwith interrupts disabled?I think you're right to be concerned!
Quoted 7 lines
Quoted 3 lines
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 1/1 arm64: fix missing syscall trace exit
In reply to Josh StoneQuoted 31 lines · Josh Stone
Quoted 30 lines · Josh Stone
If a syscall is entered without
TIF_SYSCALL_TRACEset, then it goes on the fast path. It's then possible to haveTIF_SYSCALL_TRACEadded in the middle of the syscall, butret_fast_syscalldoesn't check this flag again. This causes a ptrace syscall-exit-stop to be missed.For instance, from a
PTRACE_EVENT_FORKreported duringdo_fork, the tracer might resume withPTRACE_SYSCALL, settingTIF_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_WORKin the fast exit path. Do the same on arm64.Cc Catalin Marinas <[email protected]>Cc Will Deacon <[email protected]>Cc Russell King <[email protected]>Signed-off-by Josh Stone <[email protected]>--- arch/arm64/kernel/entry.S | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)arch/arm64/kernel/entry.S +1
-1@@ -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
Quoted 8 lines
arch/arm64/kernel/entry.S ENDPROC(cpu_switch_to) · line 616
Quoted 5 lines
arch/arm64/kernel/entry.S +2
-0+ 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, x2I do have one concern about this, also in Russell's ARM patch. Is it really ok to branch to
__sys_trace_returnwith interrupts disabled?
I'm not that happy to hear that you have concerns over the patch after hurrying its submission into the -rc kernels.
Quoted 3 lines
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?
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
PATCH 1/1 arm64: fix missing syscall trace exit
Quoted 41 lines · Russell King - ARM Linux
Quoted 31 lines · Josh Stone
Quoted 30 lines · Josh Stone
If a syscall is entered without
TIF_SYSCALL_TRACEset, then it goes on the fast path. It's then possible to haveTIF_SYSCALL_TRACEadded in the middle of the syscall, butret_fast_syscalldoesn't check this flag again. This causes a ptrace syscall-exit-stop to be missed.For instance, from a
PTRACE_EVENT_FORKreported duringdo_fork, the tracer might resume withPTRACE_SYSCALL, settingTIF_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_WORKin the fast exit path. Do the same on arm64.Cc Catalin Marinas <[email protected]>Cc Will Deacon <[email protected]>Cc Russell King <[email protected]>Signed-off-by Josh Stone <[email protected]>--- arch/arm64/kernel/entry.S | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)arch/arm64/kernel/entry.S +1
-1@@ -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 tracingQuoted 8 lines
arch/arm64/kernel/entry.S ENDPROC(cpu_switch_to) · line 616
Quoted 5 lines
arch/arm64/kernel/entry.S +2
-0+ 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, x2I do have one concern about this, also in Russell's ARM patch. Is it really ok to branch to
__sys_trace_returnwith interrupts disabled?
Quoted 2 lines
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.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 2 lines
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.
Quoted 6 lines
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 1/1 arm64: fix missing syscall trace exit
Quoted 6 lines · Josh Stone
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_syscallto 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.
Quoted 3 lines
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?
arch/arm64/kernel/entry.S +6 -1
@@ -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 1/1 arm64: fix missing syscall trace exit
Quoted 15 lines · Josh Stone
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?
arch/arm64/kernel/entry.S +3
-1@@ -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
arch/arm64/kernel/entry.S ENDPROC(cpu_switch_to) · line 623
Quoted 10 lines
arch/arm64/kernel/entry.S +3
-0and 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?
Quoted 3 lines
Off topic, here's the
xgbe_tx_timerBUG. 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 calldisable_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
Quoted 41 lines
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 1/1 arm64: fix missing syscall trace exit
Quoted 30 lines · Will Deacon
Quoted 15 lines · Josh Stone
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?
arch/arm64/kernel/entry.S +3
-1@@ -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_tracearch/arm64/kernel/entry.S ENDPROC(cpu_switch_to) · line 623
Quoted 10 lines
arch/arm64/kernel/entry.S +3
-0and 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?
Quoted 7 lines
Quoted 3 lines
Off topic, here's the
xgbe_tx_timerBUG. 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 calldisable_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
Quoted 43 lines
Will
Quoted 41 lines
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 1/1 arm64: fix missing syscall trace exit
In reply to Will DeaconIf 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.
--- arch/arm64/kernel/entry.S | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
arch/arm64/kernel/entry.S +6 -1
@@ -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.
Signature · 1 line
2.4.1
PATCH 1/1 arm64: fix missing syscall trace exit
Hi Josh,
Thanks for the patch.
Quoted 5 lines · Josh Stone
If a syscall is entered without
TIF_SYSCALL_TRACEset, then it goes on the fast path. It's then possible to haveTIF_SYSCALL_TRACEadded in the middle of the syscall, butret_fast_syscalldoesn't check this flag again. This causes a ptrace syscall-exit-stop to be missed.
Quoted 8 lines
For instance, from a
PTRACE_EVENT_FORKreported duringdo_fork, the tracer might resume withPTRACE_SYSCALL, settingTIF_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_WORKin 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.
Quoted 17 lines
Cc Catalin Marinas <[email protected]>Cc Will Deacon <[email protected]>Cc Russell King <[email protected]>Signed-off-by Josh Stone <[email protected]>--- arch/arm64/kernel/entry.S | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)arch/arm64/kernel/entry.S +0
-1@@ -608,11 +608,16 @@ ENDPROC(cpu_switch_to) */ ret_fast_syscall: disable_irq // disable interrupts - ldr x1, [tsk, #TI_FLAGS]
arch/arm64/kernel/entry.S ENDPROC(cpu_switch_to) · line 620
Quoted 10 lines
arch/arm64/kernel/entry.S +6
-0+ 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:
I assume this can wait until 4.2?
Will
PATCH 1/1 arm64: fix missing syscall trace exit
Quoted 3 lines · Will Deacon
Quoted 1 line
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?
Quoted 1 line
I assume this can wait until 4.2?
That's fine with me, thanks.
Catalin Marinas
maintainerPATCH 1/1 arm64: fix missing syscall trace exit
Quoted 6 lines · Josh Stone
Quoted 3 lines · Will Deacon
Quoted 1 line
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.
Signature · 1 line
Catalin
PATCH 1/1 ARM: enable_irq before ret_fast_syscall tracing
In reply to Russell King - ARM LinuxWhen 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.
--- arch/arm/kernel/entry-common.S | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
arch/arm/kernel/entry-common.S +5 -1
@@ -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 ) /*
Signature · 1 line
2.4.2
PATCH 1/1 ARM: enable_irq before ret_fast_syscall tracing
Quoted 7 lines · Josh Stone
When reached via the slow path
__sys_trace,__sys_trace_returnand 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_syscallsees tracing was enabled mid-syscall, then it also ought to branch to__sys_trace_returnwith 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!
Quoted 31 lines
Cc Russell King <[email protected]>Signed-off-by Josh Stone <[email protected]>--- arch/arm/kernel/entry-common.S | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)arch/arm/kernel/entry-common.S +5
-1@@ -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 ) /*