[PATCH] arm64: Fix PTRACE_SYSEMU semantics

[PATCH] arm64: Fix PTRACE_SYSEMU semantics

[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 <keno@juliacomputing.com>
---
 arch/arm64/kernel/ptrace.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index b3d3005d9515..b67b4d14aa17 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -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;
 	}
 
-- 
2.25.1

Re: [PATCH] arm64: Fix PTRACE_SYSEMU semantics

[Will Deacon]

On Fri, May 15, 2020 at 06:22:53PM -0400, Keno Fischer wrote:
> 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 <keno@juliacomputing.com>
> ---
>  arch/arm64/kernel/ptrace.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index b3d3005d9515..b67b4d14aa17 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -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 <will@kernel.org>

Also needs:

Cc: <stable@vger.kernel.org>
Fixes: f086f67485c5 ("arm64: ptrace: add support for syscall emulation")

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

Will

Re: [PATCH] arm64: Fix PTRACE_SYSEMU semantics

[Catalin Marinas]

On Mon, May 18, 2020 at 12:41:20PM +0100, Will Deacon wrote:
> On Fri, May 15, 2020 at 06:22:53PM -0400, Keno Fischer wrote:
> > 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 <keno@juliacomputing.com>
> > ---
> >  arch/arm64/kernel/ptrace.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> > index b3d3005d9515..b67b4d14aa17 100644
> > --- a/arch/arm64/kernel/ptrace.c
> > +++ b/arch/arm64/kernel/ptrace.c
> > @@ -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 <will@kernel.org>
> 
> Also needs:
> 
> Cc: <stable@vger.kernel.org>
> 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.

-- 
Catalin

Re: [PATCH] arm64: Fix PTRACE_SYSEMU semantics

[Sudeep Holla]

Hi Catalin,

On Tue, May 19, 2020 at 01:07:27PM +0100, Catalin Marinas wrote:
> On Mon, May 18, 2020 at 12:41:20PM +0100, Will Deacon wrote:
> > On Fri, May 15, 2020 at 06:22:53PM -0400, Keno Fischer wrote:
> > > 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 <keno@juliacomputing.com>
> > > ---
> > >  arch/arm64/kernel/ptrace.c | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> > > index b3d3005d9515..b67b4d14aa17 100644
> > > --- a/arch/arm64/kernel/ptrace.c
> > > +++ b/arch/arm64/kernel/ptrace.c
> > > @@ -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 <will@kernel.org>
> >
> > Also needs:
> >
> > Cc: <stable@vger.kernel.org>
> > 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.
>

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 <sudeep.holla@arm.com>

--
Regards,
Sudeep

Re: [PATCH] arm64: Fix PTRACE_SYSEMU semantics

[Catalin Marinas]

On Wed, May 20, 2020 at 11:03:30AM +0100, Sudeep Holla wrote:
> On Tue, May 19, 2020 at 01:07:27PM +0100, Catalin Marinas wrote:
> > On Mon, May 18, 2020 at 12:41:20PM +0100, Will Deacon wrote:
> > > On Fri, May 15, 2020 at 06:22:53PM -0400, Keno Fischer wrote:
> > > > 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 <keno@juliacomputing.com>
> > > > ---
> > > >  arch/arm64/kernel/ptrace.c | 8 +++++---
> > > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> > > > index b3d3005d9515..b67b4d14aa17 100644
> > > > --- a/arch/arm64/kernel/ptrace.c
> > > > +++ b/arch/arm64/kernel/ptrace.c
> > > > @@ -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 <will@kernel.org>
> > >
> > > Also needs:
> > >
> > > Cc: <stable@vger.kernel.org>
> > > 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.
> 
> 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 <sudeep.holla@arm.com>

Thanks Sudeep.

-- 
Catalin

回复: [PATCH] arm64: Fix PTRACE_SYSEMU semantics

[Bin Lu]

Tested-by: Bin Lu <bin.lu@arm.com >

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

> -----邮件原件-----
> 发件人: linux-arm-kernel <linux-arm-kernel-bounces@lists.infradead.org> 代
> 表 Catalin Marinas
> 发送时间: 2020年5月19日 20:07
> 收件人: Will Deacon <will@kernel.org>
> 抄送: Will Deacon <Will.Deacon@arm.com>; linux-kernel@vger.kernel.org;
> oleg@redhat.com; Keno Fischer <keno@juliacomputing.com>; Sudeep Holla
> <Sudeep.Holla@arm.com>; linux-arm-kernel@lists.infradead.org
> 主题: Re: [PATCH] arm64: Fix PTRACE_SYSEMU semantics
> 
> On Mon, May 18, 2020 at 12:41:20PM +0100, Will Deacon wrote:
> > On Fri, May 15, 2020 at 06:22:53PM -0400, Keno Fischer wrote:
> > > 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 <keno@juliacomputing.com>
> > > ---
> > >  arch/arm64/kernel/ptrace.c | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> > > index b3d3005d9515..b67b4d14aa17 100644
> > > --- a/arch/arm64/kernel/ptrace.c
> > > +++ b/arch/arm64/kernel/ptrace.c
> > > @@ -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 <will@kernel.org>
> >
> > Also needs:
> >
> > Cc: <stable@vger.kernel.org>
> > 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.
> 
> --
> Catalin
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: Fix PTRACE_SYSEMU semantics

[Bin Lu]

From: Bin Lu <Bin.Lu@arm.com>

On Tue, May 19, 2020 at 01:07:27PM +0100, Catalin Marinas wrote:
> On Mon, May 18, 2020 at 12:41:20PM +0100, Will Deacon wrote:
> > On Fri, May 15, 2020 at 06:22:53PM -0400, Keno Fischer wrote:
> > > 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 <keno@juliacomputing.com>
> > > ---
> > >  arch/arm64/kernel/ptrace.c | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> > > index b3d3005d9515..b67b4d14aa17 100644
> > > --- a/arch/arm64/kernel/ptrace.c
> > > +++ b/arch/arm64/kernel/ptrace.c
> > > @@ -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 <will@kernel.org>
> >
> > Also needs:
> >
> > Cc: <stable@vger.kernel.org>
> > 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.
>

Tested-by: Bin Lu <Bin.Lu@arm.com> (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