arm64: Fix PTRACE_SYSEMU semantics

PATCH In review Keno Fischer 5 participants 7 messages May 15, 2020 – May 20, 2020

Subsystems (2)

arm64 ptrace support

lore

Share & tools

lore

Keno Fischer

author

2020-05-15 22:22 UTC #0 lore raw

PATCH 1/1 arm64: Fix PTRACE_SYSEMU semantics

Quoth the man page:

```
       If the tracee was restarted by PTRACE_SYSCALL or PTRACE_SYSEMU, the
       tracee enters syscall-enter-stop just prior to entering any system
       call (which will not be executed if the restart was using
       PTRACE_SYSEMU, regardless of any change made to registers at this
       point or how the tracee is restarted after this stop).
```

The parenthetical comment is currently true on x86 and powerpc, but not currently true on arm64. arm64 re-checks the _TIF_SYSCALL_EMU flag after the syscall entry ptrace stop. However, at this point, it reflects which method was used to re-start the syscall
at the entry stop, rather than the method that was used to reach it. Fix that by recording the original flag before performing the ptrace stop, bringing the behavior in line with documentation and x86/powerpc.

Signed-off-by Keno Fischer <[email protected]>

---
 arch/arm64/kernel/ptrace.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

arch/arm64/kernel/ptrace.c +5 -3

index b3d3005d9515..b67b4d14aa17 100644
@@ -1829,10 +1829,12 @@ static void tracehook_report_syscall(struct pt_regs *regs,
 
 int syscall_trace_enter(struct pt_regs *regs)
 {
-	if (test_thread_flag(TIF_SYSCALL_TRACE) ||
-		test_thread_flag(TIF_SYSCALL_EMU)) {
+	u32 flags = READ_ONCE(current_thread_info()->flags) &
+		(_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
+
+	if (flags) {
 		tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);
-		if (!in_syscall(regs) || test_thread_flag(TIF_SYSCALL_EMU))
+		if (!in_syscall(regs) || (flags & _TIF_SYSCALL_EMU))
 			return -1;
 	}

Signature · 1 line

2.25.1

Will Deacon

maintainer Acked-by on v1 1/1

2020-05-18 11:41 UTC #1 lore raw

Quoted 27 lines · Keno Fischer

Quoth the man page:
```
       If the tracee was restarted by PTRACE_SYSCALL or PTRACE_SYSEMU, the
       tracee enters syscall-enter-stop just prior to entering any system
       call (which will not be executed if the restart was using
       PTRACE_SYSEMU, regardless of any change made to registers at this
       point or how the tracee is restarted after this stop).
```
The parenthetical comment is currently true on x86 and powerpc, but not currently true on arm64. arm64 re-checks the _TIF_SYSCALL_EMU flag after the syscall entry ptrace stop. However, at this point, it reflects which method was used to re-start the syscall
at the entry stop, rather than the method that was used to reach it. Fix that by recording the original flag before performing the ptrace stop, bringing the behavior in line with documentation and x86/powerpc.
Signed-off-by Keno Fischer <[email protected]>
---
 arch/arm64/kernel/ptrace.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
arch/arm64/kernel/ptrace.c +0 -0
index b3d3005d9515..b67b4d14aa17 100644

arch/arm64/kernel/ptrace.c static void tracehook_report_syscall(struct pt_regs *regs, · line 1835

Quoted 9 lines

arch/arm64/kernel/ptrace.c +4 -2

@@ -1829,10 +1829,12 @@ static void tracehook_report_syscall(struct pt_regs *regs,
 
 int syscall_trace_enter(struct pt_regs *regs)
 {
-	if (test_thread_flag(TIF_SYSCALL_TRACE) ||
-		test_thread_flag(TIF_SYSCALL_EMU)) {
+	u32 flags = READ_ONCE(current_thread_info()->flags) &
+		(_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
+
+	if (flags) {

nit: but I'd rather the '&' operation was in the conditional so that the 'flags' variable holds all of the flags.

With that:

Acked-by Will Deacon <[email protected]>

Also needs:

Cc <[email protected]>

Fixes f086f67485c5 ("arm64: ptrace: add support for syscall emulation")

Catalin -- can you pick this up for 5.7 please, with my 'nit' addressed?

Will

Catalin Marinas

maintainer

2020-05-19 12:07 UTC #2 lore raw

Quoted 43 lines · Will Deacon

Quoted 27 lines · Keno Fischer
Quoth the man page:
```
       If the tracee was restarted by PTRACE_SYSCALL or PTRACE_SYSEMU, the
       tracee enters syscall-enter-stop just prior to entering any system
       call (which will not be executed if the restart was using
       PTRACE_SYSEMU, regardless of any change made to registers at this
       point or how the tracee is restarted after this stop).
```
The parenthetical comment is currently true on x86 and powerpc, but not currently true on arm64. arm64 re-checks the _TIF_SYSCALL_EMU flag after the syscall entry ptrace stop. However, at this point, it reflects which method was used to re-start the syscall
at the entry stop, rather than the method that was used to reach it. Fix that by recording the original flag before performing the ptrace stop, bringing the behavior in line with documentation and x86/powerpc.
Signed-off-by Keno Fischer <[email protected]>
---
 arch/arm64/kernel/ptrace.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
arch/arm64/kernel/ptrace.c +0 -0
index b3d3005d9515..b67b4d14aa17 100644
arch/arm64/kernel/ptrace.c static void tracehook_report_syscall(struct pt_regs *regs, · line 1835
Quoted 9 lines
arch/arm64/kernel/ptrace.c +4 -2
@@ -1829,10 +1829,12 @@ static void tracehook_report_syscall(struct pt_regs *regs,
 
 int syscall_trace_enter(struct pt_regs *regs)
 {
-	if (test_thread_flag(TIF_SYSCALL_TRACE) ||
-		test_thread_flag(TIF_SYSCALL_EMU)) {
+	u32 flags = READ_ONCE(current_thread_info()->flags) &
+		(_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
+
+	if (flags) {
nit: but I'd rather the '&' operation was in the conditional so that the 'flags' variable holds all of the flags.
With that:

Quoted 8 lines

Acked-by Will Deacon <[email protected]>
Also needs:
Cc <[email protected]>
Fixes f086f67485c5 ("arm64: ptrace: add support for syscall emulation")
Catalin -- can you pick this up for 5.7 please, with my 'nit' addressed?

I'll queue it with the above addressed. I think flags also needs to be unsigned long rather than u32.

However, before sending the pull request, I'd like Sudeep to confirm that it doesn't break his original use-case for this feature.

Signature · 1 line

Catalin

Sudeep Holla

Tested-by

2020-05-20 10:03 UTC #3 lore raw

Hi Catalin,

Quoted 51 lines · Catalin Marinas

Quoted 50 lines · Will Deacon
Quoted 27 lines · Keno Fischer
Quoth the man page:
```
       If the tracee was restarted by PTRACE_SYSCALL or PTRACE_SYSEMU, the
       tracee enters syscall-enter-stop just prior to entering any system
       call (which will not be executed if the restart was using
       PTRACE_SYSEMU, regardless of any change made to registers at this
       point or how the tracee is restarted after this stop).
```
The parenthetical comment is currently true on x86 and powerpc, but not currently true on arm64. arm64 re-checks the _TIF_SYSCALL_EMU flag after the syscall entry ptrace stop. However, at this point, it reflects which method was used to re-start the syscall
at the entry stop, rather than the method that was used to reach it. Fix that by recording the original flag before performing the ptrace stop, bringing the behavior in line with documentation and x86/powerpc.
Signed-off-by Keno Fischer <[email protected]>
---
 arch/arm64/kernel/ptrace.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
arch/arm64/kernel/ptrace.c +0 -0
index b3d3005d9515..b67b4d14aa17 100644
arch/arm64/kernel/ptrace.c static void tracehook_report_syscall(struct pt_regs *regs, · line 1835
Quoted 9 lines
arch/arm64/kernel/ptrace.c +4 -2
@@ -1829,10 +1829,12 @@ static void tracehook_report_syscall(struct pt_regs *regs,
 
 int syscall_trace_enter(struct pt_regs *regs)
 {
-	if (test_thread_flag(TIF_SYSCALL_TRACE) ||
-		test_thread_flag(TIF_SYSCALL_EMU)) {
+	u32 flags = READ_ONCE(current_thread_info()->flags) &
+		(_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
+
+	if (flags) {
nit: but I'd rather the '&' operation was in the conditional so that the 'flags' variable holds all of the flags.
With that:
Acked-by Will Deacon <[email protected]>
Also needs:
Cc <[email protected]>
Fixes f086f67485c5 ("arm64: ptrace: add support for syscall emulation")

Quoted 8 lines

Quoted 1 line
Catalin -- can you pick this up for 5.7 please, with my 'nit' addressed?
I'll queue it with the above addressed. I think flags also needs to be unsigned long rather than u32.
However, before sending the pull request, I'd like Sudeep to confirm that it doesn't break his original use-case for this feature.

I just tested it with my simple programs I had before. I have also asked teams working on gvisor to test. They have tested it and see no regression. I will ask them to reply here.

Tested-by Sudeep Holla <[email protected]>

--
Regards,
Sudeep

Catalin Marinas

maintainer

2020-05-20 10:05 UTC #4 lore raw

Quoted 57 lines · Sudeep Holla

Quoted 56 lines · Catalin Marinas
Quoted 43 lines · Will Deacon
Quoted 27 lines · Keno Fischer
Quoth the man page:
```
       If the tracee was restarted by PTRACE_SYSCALL or PTRACE_SYSEMU, the
       tracee enters syscall-enter-stop just prior to entering any system
       call (which will not be executed if the restart was using
       PTRACE_SYSEMU, regardless of any change made to registers at this
       point or how the tracee is restarted after this stop).
```
The parenthetical comment is currently true on x86 and powerpc, but not currently true on arm64. arm64 re-checks the _TIF_SYSCALL_EMU flag after the syscall entry ptrace stop. However, at this point, it reflects which method was used to re-start the syscall
at the entry stop, rather than the method that was used to reach it. Fix that by recording the original flag before performing the ptrace stop, bringing the behavior in line with documentation and x86/powerpc.
Signed-off-by Keno Fischer <[email protected]>
---
 arch/arm64/kernel/ptrace.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
arch/arm64/kernel/ptrace.c +0 -0
index b3d3005d9515..b67b4d14aa17 100644
arch/arm64/kernel/ptrace.c static void tracehook_report_syscall(struct pt_regs *regs, · line 1835
Quoted 9 lines
arch/arm64/kernel/ptrace.c +4 -2
@@ -1829,10 +1829,12 @@ static void tracehook_report_syscall(struct pt_regs *regs,
 
 int syscall_trace_enter(struct pt_regs *regs)
 {
-	if (test_thread_flag(TIF_SYSCALL_TRACE) ||
-		test_thread_flag(TIF_SYSCALL_EMU)) {
+	u32 flags = READ_ONCE(current_thread_info()->flags) &
+		(_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
+
+	if (flags) {
nit: but I'd rather the '&' operation was in the conditional so that the 'flags' variable holds all of the flags.
With that:
Quoted 8 lines
Acked-by Will Deacon <[email protected]>
Also needs:
Cc <[email protected]>
Fixes f086f67485c5 ("arm64: ptrace: add support for syscall emulation")
Catalin -- can you pick this up for 5.7 please, with my 'nit' addressed?
I'll queue it with the above addressed. I think flags also needs to be unsigned long rather than u32.

Quoted 8 lines

Quoted 2 lines
However, before sending the pull request, I'd like Sudeep to confirm that it doesn't break his original use-case for this feature.
I just tested it with my simple programs I had before. I have also asked teams working on gvisor to test. They have tested it and see no regression. I will ask them to reply here.
Tested-by Sudeep Holla <[email protected]>

Thanks Sudeep.

Signature · 1 line

Catalin

Bin Lu

Tested-by

2020-05-20 10:07 UTC #5 lore raw

In reply to Catalin Marinas

Tested-by Bin Lu <[email protected] >

I have just tested all gVisor syscall test cases with ptrace(Regs, FPRegs, TLS) on Arm64 platform. The test results are the same as before there was no patch.

My idea is that this kernel patch has no bad effects on gVisor. Linux Kernel version: 5.7.0-rc6+
gVisor version: release-20200511.0

Quoted 71 lines

-----邮件原件-----
发件人: linux-arm-kernel <[email protected]> 代表 Catalin Marinas
发送时间: 2020年5月19日 20:07
收件人: Will Deacon <[email protected]>
抄送: Will Deacon <[email protected]>; [email protected]; [email protected]; Keno Fischer <[email protected]>; Sudeep Holla <[email protected]>; [email protected] 主题: Re: [PATCH] arm64: Fix PTRACE_SYSEMU semantics
Quoted 4 lines · Will Deacon
Quoted 3 lines · Keno Fischer
Quoth the man page:
```
If the tracee was restarted by PTRACE_SYSCALL or PTRACE_SYSEMU,
the
Quoted 41 lines
Quoted 24 lines
tracee enters syscall-enter-stop just prior to entering any system call (which will not be executed if the restart was using PTRACE_SYSEMU, regardless of any change made to registers at this point or how the tracee is restarted after this stop).
```
The parenthetical comment is currently true on x86 and powerpc, but not currently true on arm64. arm64 re-checks the _TIF_SYSCALL_EMU flag after the syscall entry ptrace stop. However, at this point, it reflects which method was used to re-start the syscall at the entry stop, rather than the method that was used to reach it.
Fix that by recording the original flag before performing the ptrace stop, bringing the behavior in line with documentation and x86/powerpc.
Signed-off-by Keno Fischer <[email protected]>
---
 arch/arm64/kernel/ptrace.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
arch/arm64/kernel/ptrace.c +0 -0
index b3d3005d9515..b67b4d14aa17 100644
arch/arm64/kernel/ptrace.c static void tracehook_report_syscall(struct · line 1835
Quoted 9 lines
arch/arm64/kernel/ptrace.c +4 -2
@@ -1829,10 +1829,12 @@ static void tracehook_report_syscall(struct
 pt_regs *regs,
 
 int syscall_trace_enter(struct pt_regs *regs)  {
-	if (test_thread_flag(TIF_SYSCALL_TRACE) ||
-		test_thread_flag(TIF_SYSCALL_EMU)) {
+	u32 flags = READ_ONCE(current_thread_info()->flags) &
+		(_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
+
+	if (flags) {
nit: but I'd rather the '&' operation was in the conditional so that the 'flags' variable holds all of the flags.
With that:
Acked-by Will Deacon <[email protected]>
Quoted 7 lines
Also needs:
Cc <[email protected]>
Fixes f086f67485c5 ("arm64: ptrace: add support for syscall
emulation")
Catalin -- can you pick this up for 5.7 please, with my 'nit' addressed?
I'll queue it with the above addressed. I think flags also needs to be unsigned long rather than u32.
However, before sending the pull request, I'd like Sudeep to confirm that it doesn't break his original use-case for this feature.

Quoted 7 lines

--
Catalin
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Bin Lu

Tested-by

2020-05-20 10:35 UTC #6 lore raw

In reply to Catalin Marinas

From: Bin Lu <[email protected]>

Quoted 51 lines · Catalin Marinas

Quoted 50 lines · Will Deacon
Quoted 27 lines · Keno Fischer
Quoth the man page:
```
       If the tracee was restarted by PTRACE_SYSCALL or PTRACE_SYSEMU, the
       tracee enters syscall-enter-stop just prior to entering any system
       call (which will not be executed if the restart was using
       PTRACE_SYSEMU, regardless of any change made to registers at this
       point or how the tracee is restarted after this stop).
```
The parenthetical comment is currently true on x86 and powerpc, but not currently true on arm64. arm64 re-checks the _TIF_SYSCALL_EMU flag after the syscall entry ptrace stop. However, at this point, it reflects which method was used to re-start the syscall
at the entry stop, rather than the method that was used to reach it. Fix that by recording the original flag before performing the ptrace stop, bringing the behavior in line with documentation and x86/powerpc.
Signed-off-by Keno Fischer <[email protected]>
---
 arch/arm64/kernel/ptrace.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
arch/arm64/kernel/ptrace.c +0 -0
index b3d3005d9515..b67b4d14aa17 100644
arch/arm64/kernel/ptrace.c static void tracehook_report_syscall(struct pt_regs *regs, · line 1835
Quoted 9 lines
arch/arm64/kernel/ptrace.c +4 -2
@@ -1829,10 +1829,12 @@ static void tracehook_report_syscall(struct pt_regs *regs,
 
 int syscall_trace_enter(struct pt_regs *regs)
 {
-	if (test_thread_flag(TIF_SYSCALL_TRACE) ||
-		test_thread_flag(TIF_SYSCALL_EMU)) {
+	u32 flags = READ_ONCE(current_thread_info()->flags) &
+		(_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
+
+	if (flags) {
nit: but I'd rather the '&' operation was in the conditional so that the 'flags' variable holds all of the flags.
With that:
Acked-by Will Deacon <[email protected]>
Also needs:
Cc <[email protected]>
Fixes f086f67485c5 ("arm64: ptrace: add support for syscall emulation")

Quoted 8 lines

Quoted 1 line
Catalin -- can you pick this up for 5.7 please, with my 'nit' addressed?
I'll queue it with the above addressed. I think flags also needs to be unsigned long rather than u32.
However, before sending the pull request, I'd like Sudeep to confirm that it doesn't break his original use-case for this feature.

Tested-by Bin Lu <[email protected]> (for gVisor)

I have just tested all gVisor syscall test cases with ptrace(Regs, FPRegs, TLS) on Arm64 platform.
The test results are the same as before there was no patch.

My idea is that this kernel patch has no bad effects on gVisor. Linux Kernel version: 5.7.0-rc6+
gVisor version: release-20200511.0