[PATCH 0/6] ptrace: consolidate PTRACE_SYSEMU handling and add support for arm64

Hi,

This patchset evolved from the discussion in the thread[0][1]. When we
wanted to add PTRACE_SYSEMU support to ARM64, we thought instead of
duplicating what other architectures like x86 and powerpc have done,
let consolidate the existing support and move it to the core as there's
nothing arch specific in it.

So this is the first attempt to the same.

Regards,
Sudeep

[0] https://patchwork.kernel.org/patch/10585505/
[1] https://patchwork.kernel.org/patch/10675237/

Sudeep Holla (6):
  ptrace: move clearing of TIF_SYSCALL_EMU flag to core
  ptrace: introduce ptrace_syscall_enter to consolidate PTRACE_SYSEMU handling
  x86: clean up _TIF_SYSCALL_EMU handling using ptrace_syscall_enter hook
  powerpc: use common ptrace_syscall_enter hook to handle _TIF_SYSCALL_EMU
  arm64: add PTRACE_SYSEMU{,SINGLESTEP} definations to uapi headers
  arm64: ptrace: add support for syscall emulation

 arch/arm64/include/asm/thread_info.h |  5 ++-
 arch/arm64/include/uapi/asm/ptrace.h |  3 ++
 arch/arm64/kernel/ptrace.c           |  3 ++
 arch/powerpc/kernel/ptrace.c         | 51 ++++++++++++----------------
 arch/x86/entry/common.c              | 22 +++---------
 arch/x86/kernel/ptrace.c             |  3 --
 include/linux/ptrace.h               |  1 +
 kernel/ptrace.c                      | 20 +++++++++++
 8 files changed, 57 insertions(+), 51 deletions(-)

--
2.17.1

While the TIF_SYSCALL_EMU is set in ptrace_resume independent of any
architecture, currently only powerpc and x86 unset the TIF_SYSCALL_EMU
flag in ptrace_disable which gets called from ptrace_detach.

Let's move the clearing of TIF_SYSCALL_EMU flag to ptrace_detach after
we return from ptrace_disable to ensure there's no change in the flow.

Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Paul Mackerras <redacted>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Thomas Gleixner <redacted>
Cc: Ingo Molnar <mingo@redhat.com>
Signed-off-by: Sudeep Holla <redacted>
---
 arch/powerpc/kernel/ptrace.c | 1 -
 arch/x86/kernel/ptrace.c     | 3 ---
 kernel/ptrace.c              | 4 ++++
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index cdd5d1d3ae41..cb7e1439cafb 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c

@@ -2508,7 +2508,6 @@ void ptrace_disable(struct task_struct *child)
 {
 	/* make sure the single step bit is not set. */
 	user_disable_single_step(child);
-	clear_tsk_thread_flag(child, TIF_SYSCALL_EMU);
 }
 
 #ifdef CONFIG_PPC_ADV_DEBUG_REGS

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 4b8ee05dd6ad..45792dbd2443 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c

@@ -746,9 +746,6 @@ static int ioperm_get(struct task_struct *target,
 void ptrace_disable(struct task_struct *child)
 {
 	user_disable_single_step(child);
-#ifdef TIF_SYSCALL_EMU
-	clear_tsk_thread_flag(child, TIF_SYSCALL_EMU);
-#endif
 }
 
 #if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 771e93f9c43f..4fa3b7f4c3c7 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c

@@ -534,6 +534,10 @@ static int ptrace_detach(struct task_struct *child, unsigned int data)
 	/* Architecture-specific hardware disable .. */
 	ptrace_disable(child);
 
+#ifdef TIF_SYSCALL_EMU
+	clear_tsk_thread_flag(child, TIF_SYSCALL_EMU);
+#endif
+
 	write_lock_irq(&tasklist_lock);
 	/*
 	 * We rely on ptrace_freeze_traced(). It can't be killed and

-- 
2.17.1

Currently each architecture handles PTRACE_SYSEMU in very similar way.
It's completely arch independent and can be handled in the code helping
to consolidate PTRACE_SYSEMU handling.

Let's introduce a hook 'ptrace_syscall_enter' that arch specific syscall
entry code can call.

Cc: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Sudeep Holla <redacted>
---
 include/linux/ptrace.h |  1 +
 kernel/ptrace.c        | 16 ++++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index edb9b040c94c..e30f51e3363e 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h

@@ -407,6 +407,7 @@ static inline void user_single_step_report(struct pt_regs *regs)
 #define current_user_stack_pointer() user_stack_pointer(current_pt_regs())
 #endif
 
+extern long ptrace_syscall_enter(struct pt_regs *regs);
 extern int task_current_syscall(struct task_struct *target, long *callno,
 				unsigned long args[6], unsigned int maxargs,
 				unsigned long *sp, unsigned long *pc);

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 4fa3b7f4c3c7..6724eaf98e79 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c

@@ -29,6 +29,7 @@
 #include <linux/hw_breakpoint.h>
 #include <linux/cn_proc.h>
 #include <linux/compat.h>
+#include <linux/tracehook.h>
 
 /*
  * Access another process' address space via ptrace.

@@ -557,6 +558,21 @@ static int ptrace_detach(struct task_struct *child, unsigned int data)
 	return 0;
 }
 
+/*
+ * Hook to check and report for PTRACE_SYSEMU, can be called from arch
+ * arch syscall entry code
+ */
+long ptrace_syscall_enter(struct pt_regs *regs)
+{
+#ifdef TIF_SYSCALL_EMU
+	if (test_thread_flag(TIF_SYSCALL_EMU)) {
+		if (tracehook_report_syscall_entry(regs));
+		return -1L;
+	}
+#endif
+	return 0;
+}
+
 /*
  * Detach all tasks we were using ptrace on. Called with tasklist held
  * for writing.

-- 
2.17.1

On 2019/3/1 2:32, Sudeep Holla wrote:

Currently each architecture handles PTRACE_SYSEMU in very similar way.
It's completely arch independent and can be handled in the code helping
to consolidate PTRACE_SYSEMU handling.

Let's introduce a hook 'ptrace_syscall_enter' that arch specific syscall
entry code can call.

The 'ptrace_syscall_enter' is dedicated for PTRACE_SYSEMU flag,
So I suggest to rename the function to something like 'ptrace_syscall_emu_enter".

+/*
+ * Hook to check and report for PTRACE_SYSEMU, can be called from arch
+ * arch syscall entry code
+ */
+long ptrace_syscall_enter(struct pt_regs *regs)
+{
+#ifdef TIF_SYSCALL_EMU
+if (test_thread_flag(TIF_SYSCALL_EMU)) {
+if (tracehook_report_syscall_entry(regs));

Shall we remove the semi-colon at end of the above line?

+return -1L;
+}
+#endif
+return 0;
+}
+
 /*
  * Detach all tasks we were using ptrace on. Called with tasklist held
  * for writing.

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

On Mon, Mar 04, 2019 at 08:03:47AM +0000, Haibo Xu (Arm Technology China) wrote:

On 2019/3/1 2:32, Sudeep Holla wrote:

quoted

Currently each architecture handles PTRACE_SYSEMU in very similar way.
It's completely arch independent and can be handled in the code helping
to consolidate PTRACE_SYSEMU handling.

Let's introduce a hook 'ptrace_syscall_enter' that arch specific syscall
entry code can call.

The 'ptrace_syscall_enter' is dedicated for PTRACE_SYSEMU flag,
So I suggest to rename the function to something like 'ptrace_syscall_emu_enter".

I am fine to rename.

quoted

+/*
+ * Hook to check and report for PTRACE_SYSEMU, can be called from arch
+ * arch syscall entry code
+ */
+long ptrace_syscall_enter(struct pt_regs *regs)
+{
+#ifdef TIF_SYSCALL_EMU
+	if (test_thread_flag(TIF_SYSCALL_EMU)) {
+		if (tracehook_report_syscall_entry(regs));

Shall we remove the semi-colon at end of the above line?

Added intentionally to keep GCC happy.

--
Regards,
Sudeep

On Mon, Mar 04, 2019 at 10:46:43AM +0000, Sudeep Holla wrote:

On Mon, Mar 04, 2019 at 08:03:47AM +0000, Haibo Xu (Arm Technology China) wrote:

quoted

On 2019/3/1 2:32, Sudeep Holla wrote:

quoted

+long ptrace_syscall_enter(struct pt_regs *regs)
+{
+#ifdef TIF_SYSCALL_EMU
+	if (test_thread_flag(TIF_SYSCALL_EMU)) {
+		if (tracehook_report_syscall_entry(regs));

Shall we remove the semi-colon at end of the above line?

Added intentionally to keep GCC happy.

GCC warns because the user explicitly asked for it, with __must_check.
If you want to do things with an "if" like this, you should write e.g.

		if (tracehook_report_syscall_entry(regs))
			/*
			 * We can ignore the return code here, because of
			 * X and Y and Z.
			 */
			;

Or it probably is nicer to use a block:

		if (tracehook_report_syscall_entry(regs)) {
			/*
			 * We can ignore the return code here, because of
			 * X and Y and Z.
			 */
		}

The point is, you *always* should have a nice fat comment if you are
ignoring the return code of a __must_check function.


Segher

On Mon, Mar 04, 2019 at 06:23:32AM -0600, Segher Boessenkool wrote:

On Mon, Mar 04, 2019 at 10:46:43AM +0000, Sudeep Holla wrote:

quoted

On Mon, Mar 04, 2019 at 08:03:47AM +0000, Haibo Xu (Arm Technology China) wrote:

quoted

On 2019/3/1 2:32, Sudeep Holla wrote:

quoted

+long ptrace_syscall_enter(struct pt_regs *regs)
+{
+#ifdef TIF_SYSCALL_EMU
+	if (test_thread_flag(TIF_SYSCALL_EMU)) {
+		if (tracehook_report_syscall_entry(regs));

Shall we remove the semi-colon at end of the above line?

Added intentionally to keep GCC happy.

GCC warns because the user explicitly asked for it, with __must_check.
If you want to do things with an "if" like this, you should write e.g.

		if (tracehook_report_syscall_entry(regs))
			/*
			 * We can ignore the return code here, because of
			 * X and Y and Z.
			 */
			;

Or it probably is nicer to use a block:

		if (tracehook_report_syscall_entry(regs)) {
			/*
			 * We can ignore the return code here, because of
			 * X and Y and Z.
			 */
		}

The point is, you *always* should have a nice fat comment if you are
ignoring the return code of a __must_check function.

Agreed, will add the comment.

--
Regards,
Sudeep

Now that we have a new hook ptrace_syscall_enter that can be called from
syscall entry code and it handles PTRACE_SYSEMU in generic code, we
can do some cleanup using the same in syscall_trace_enter.

Further the extra logic to find single stepping PTRACE_SYSEMU_SINGLESTEP
in syscall_slow_exit_work seems unnecessary. Let's remove the same.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Thomas Gleixner <redacted>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Sudeep Holla <redacted>
---
 arch/x86/entry/common.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index 7bc105f47d21..36457c1f87d2 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c

@@ -70,22 +70,16 @@ static long syscall_trace_enter(struct pt_regs *regs)
 
 	struct thread_info *ti = current_thread_info();
 	unsigned long ret = 0;
-	bool emulated = false;
 	u32 work;
 
 	if (IS_ENABLED(CONFIG_DEBUG_ENTRY))
 		BUG_ON(regs != task_pt_regs(current));
 
-	work = READ_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY;
-
-	if (unlikely(work & _TIF_SYSCALL_EMU))
-		emulated = true;
-
-	if ((emulated || (work & _TIF_SYSCALL_TRACE)) &&
-	    tracehook_report_syscall_entry(regs))
+	if (unlikely(ptrace_syscall_enter(regs)))
 		return -1L;
 
-	if (emulated)
+	work = READ_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY;
+	if ((work & _TIF_SYSCALL_TRACE) && tracehook_report_syscall_entry(regs))
 		return -1L;
 
 #ifdef CONFIG_SECCOMP

@@ -227,15 +221,7 @@ static void syscall_slow_exit_work(struct pt_regs *regs, u32 cached_flags)
 	if (cached_flags & _TIF_SYSCALL_TRACEPOINT)
 		trace_sys_exit(regs, regs->ax);
 
-	/*
-	 * If TIF_SYSCALL_EMU is set, we only get here because of
-	 * TIF_SINGLESTEP (i.e. this is PTRACE_SYSEMU_SINGLESTEP).
-	 * We already reported this syscall instruction in
-	 * syscall_trace_enter().
-	 */
-	step = unlikely(
-		(cached_flags & (_TIF_SINGLESTEP | _TIF_SYSCALL_EMU))
-		== _TIF_SINGLESTEP);
+	step = unlikely((cached_flags & _TIF_SINGLESTEP));
 	if (step || cached_flags & _TIF_SYSCALL_TRACE)
 		tracehook_report_syscall_exit(regs, step);
 }

-- 
2.17.1

On Thu, Feb 28, 2019 at 10:32 AM Sudeep Holla [off-list ref] wrote:

Now that we have a new hook ptrace_syscall_enter that can be called from
syscall entry code and it handles PTRACE_SYSEMU in generic code, we
can do some cleanup using the same in syscall_trace_enter.

Further the extra logic to find single stepping PTRACE_SYSEMU_SINGLESTEP
in syscall_slow_exit_work seems unnecessary. Let's remove the same.

I wasn't cc'd on the whole series, so I can't easily review this.  Do
you have a test case to make sure that emulation still works?  Are
there adequate tests in tools/testing/selftests/x86?  Do they still
pass after this patch?

--Andy

On Sat, Mar 02, 2019 at 05:11:40PM -0800, Andy Lutomirski wrote:

On Thu, Feb 28, 2019 at 10:32 AM Sudeep Holla [off-list ref] wrote:

quoted

Now that we have a new hook ptrace_syscall_enter that can be called from
syscall entry code and it handles PTRACE_SYSEMU in generic code, we
can do some cleanup using the same in syscall_trace_enter.

Further the extra logic to find single stepping PTRACE_SYSEMU_SINGLESTEP
in syscall_slow_exit_work seems unnecessary. Let's remove the same.

I wasn't cc'd on the whole series, so I can't easily review this.  Do
you have a test case to make sure that emulation still works?  Are
there adequate tests in tools/testing/selftests/x86?  Do they still
pass after this patch?

I will ensure you are cc-ed on the whole threads, sorry for missing.
I remember seeing some selftests, but I haven't run them yet.

--
Regards,
Sudeep

On 2019/3/1 2:32, Sudeep Holla wrote:

Now that we have a new hook ptrace_syscall_enter that can be called from
syscall entry code and it handles PTRACE_SYSEMU in generic code, we
can do some cleanup using the same in syscall_trace_enter.

Further the extra logic to find single stepping PTRACE_SYSEMU_SINGLESTEP
in syscall_slow_exit_work seems unnecessary. Let's remove the same.

I think we should not change the logic here. Is so, it will double the report of syscall
when PTRACE_SYSEMU_SINGLESTEP is enabled.

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

On Mon, Mar 04, 2019 at 08:25:28AM +0000, Haibo Xu (Arm Technology China) wrote:

On 2019/3/1 2:32, Sudeep Holla wrote:

quoted

Now that we have a new hook ptrace_syscall_enter that can be called from
syscall entry code and it handles PTRACE_SYSEMU in generic code, we
can do some cleanup using the same in syscall_trace_enter.

Further the extra logic to find single stepping PTRACE_SYSEMU_SINGLESTEP
in syscall_slow_exit_work seems unnecessary. Let's remove the same.

I think we should not change the logic here. Is so, it will double the report of syscall
when PTRACE_SYSEMU_SINGLESTEP is enabled.

I don't think that should happen, but I may be missing something.
Can you explain how ?

--
Regards,
Sudeep

On 2019/3/4 18:12, Sudeep Holla wrote:

On Mon, Mar 04, 2019 at 08:25:28AM +0000, Haibo Xu (Arm Technology China) wrote:

quoted

On 2019/3/1 2:32, Sudeep Holla wrote:

quoted

Now that we have a new hook ptrace_syscall_enter that can be called from
syscall entry code and it handles PTRACE_SYSEMU in generic code, we
can do some cleanup using the same in syscall_trace_enter.

Further the extra logic to find single stepping PTRACE_SYSEMU_SINGLESTEP
in syscall_slow_exit_work seems unnecessary. Let's remove the same.

I think we should not change the logic here. Is so, it will double the report of syscall
when PTRACE_SYSEMU_SINGLESTEP is enabled.

I don't think that should happen, but I may be missing something.
Can you explain how ?

--
Regards,
Sudeep

When PTRACE_SYSEMU_SINGLESTEP is enabled, both the _TIF_SYSCALL_EMU and _TIF_SINGLESTEP
flags are set, but ptrace only need to report(send SIGTRAP) at the entry of a system call,
no need to report at the exit of a system call.

Regards,
Haibo
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

(I thought I had sent this email, last Tuesday itself, but saw this in my
draft today, something went wrong, sorry for the delay)

On Tue, Mar 05, 2019 at 02:14:47AM +0000, Haibo Xu (Arm Technology China) wrote:

On 2019/3/4 18:12, Sudeep Holla wrote:

quoted

On Mon, Mar 04, 2019 at 08:25:28AM +0000, Haibo Xu (Arm Technology China) wrote:

quoted

On 2019/3/1 2:32, Sudeep Holla wrote:

quoted

Now that we have a new hook ptrace_syscall_enter that can be called from
syscall entry code and it handles PTRACE_SYSEMU in generic code, we
can do some cleanup using the same in syscall_trace_enter.

Further the extra logic to find single stepping PTRACE_SYSEMU_SINGLESTEP
in syscall_slow_exit_work seems unnecessary. Let's remove the same.

I think we should not change the logic here. Is so, it will double the report of syscall
when PTRACE_SYSEMU_SINGLESTEP is enabled.

I don't think that should happen, but I may be missing something.
Can you explain how ?

When PTRACE_SYSEMU_SINGLESTEP is enabled, both the _TIF_SYSCALL_EMU and
_TIF_SINGLESTEP flags are set, but ptrace only need to report(send SIGTRAP)
at the entry of a system call, no need to report at the exit of a system
call.

Sorry, but I still not get it, we have:

	step = ((flags & (_TIF_SINGLESTEP | _TIF_SYSCALL_EMU)) == _TIF_SINGLESTEP);

For me, this is same as:
	step = ((flags & _TIF_SINGLESTEP) == _TIF_SINGLESTEP)
	or
	if (flags & _TIF_SINGLESTEP)
		step = true;

So when PTRACE_SYSEMU_SINGLESTEP, _TIF_SYSCALL_EMU and _TIF_SINGLESTEP
are set and step evaluates to true.

So dropping _TIF_SYSCALL_EMU here should be fine. Am I still missing
something ?

--
Regards,
Sudeep

On 2019/3/12 2:34, Sudeep Holla wrote:

(I thought I had sent this email, last Tuesday itself, but saw this in my
draft today, something went wrong, sorry for the delay)

On Tue, Mar 05, 2019 at 02:14:47AM +0000, Haibo Xu (Arm Technology China) wrote:

quoted

On 2019/3/4 18:12, Sudeep Holla wrote:

quoted

On Mon, Mar 04, 2019 at 08:25:28AM +0000, Haibo Xu (Arm Technology China) wrote:

quoted

On 2019/3/1 2:32, Sudeep Holla wrote:

quoted

Now that we have a new hook ptrace_syscall_enter that can be called from
syscall entry code and it handles PTRACE_SYSEMU in generic code, we
can do some cleanup using the same in syscall_trace_enter.

Further the extra logic to find single stepping PTRACE_SYSEMU_SINGLESTEP
in syscall_slow_exit_work seems unnecessary. Let's remove the same.

I think we should not change the logic here. Is so, it will double the report of syscall
when PTRACE_SYSEMU_SINGLESTEP is enabled.

I don't think that should happen, but I may be missing something.
Can you explain how ?

When PTRACE_SYSEMU_SINGLESTEP is enabled, both the _TIF_SYSCALL_EMU and
_TIF_SINGLESTEP flags are set, but ptrace only need to report(send SIGTRAP)
at the entry of a system call, no need to report at the exit of a system
call.

Sorry, but I still not get it, we have:

step = ((flags & (_TIF_SINGLESTEP | _TIF_SYSCALL_EMU)) == _TIF_SINGLESTEP);

For me, this is same as:
step = ((flags & _TIF_SINGLESTEP) == _TIF_SINGLESTEP)
or
if (flags & _TIF_SINGLESTEP)
step = true;

I don't think so! As I mentioned in the last email loop, when PTRACE_SYSEMU_SINGLESTEP
is enabled, both the _TIF_SYSCALL_EMU and _TIF_SINGLESTEP flags are set, in which case
the step should be "false" for the old logic. But with the new logic, the step is "true".

So when PTRACE_SYSEMU_SINGLESTEP, _TIF_SYSCALL_EMU and _TIF_SINGLESTEP
are set and step evaluates to true.

So dropping _TIF_SYSCALL_EMU here should be fine. Am I still missing
something ?

--
Regards,
Sudeep

For the PTRACE_SYSEMU_SINGLESTEP request, ptrace only need to report(send SIGTRAP)
at the entry of a system call, no need to report at the exit of a system call.That's
why the old logic-{step = ((flags & (_TIF_SINGLESTEP | _TIF_SYSCALL_EMU)) == _TIF_SINGLESTEP)}
here try to filter out the special case(PTRACE_SYSEMU_SINGLESTEP).

Another way to make sure the logic is fine, you can run some tests with respect to both logic,
and to check whether they have the same behavior.

Regards,

Haibo
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

On Mon, Mar 11, 2019 at 6:35 PM Haibo Xu (Arm Technology China)
[off-list ref] wrote:

On 2019/3/12 2:34, Sudeep Holla wrote:

quoted

(I thought I had sent this email, last Tuesday itself, but saw this in my
draft today, something went wrong, sorry for the delay)

On Tue, Mar 05, 2019 at 02:14:47AM +0000, Haibo Xu (Arm Technology China) wrote:

quoted

On 2019/3/4 18:12, Sudeep Holla wrote:

quoted

On Mon, Mar 04, 2019 at 08:25:28AM +0000, Haibo Xu (Arm Technology China) wrote:

quoted

On 2019/3/1 2:32, Sudeep Holla wrote:

quoted

Now that we have a new hook ptrace_syscall_enter that can be called from
syscall entry code and it handles PTRACE_SYSEMU in generic code, we
can do some cleanup using the same in syscall_trace_enter.

Further the extra logic to find single stepping PTRACE_SYSEMU_SINGLESTEP
in syscall_slow_exit_work seems unnecessary. Let's remove the same.

I think we should not change the logic here. Is so, it will double the report of syscall
when PTRACE_SYSEMU_SINGLESTEP is enabled.

I don't think that should happen, but I may be missing something.
Can you explain how ?

When PTRACE_SYSEMU_SINGLESTEP is enabled, both the _TIF_SYSCALL_EMU and
_TIF_SINGLESTEP flags are set, but ptrace only need to report(send SIGTRAP)
at the entry of a system call, no need to report at the exit of a system
call.

Sorry, but I still not get it, we have:

step = ((flags & (_TIF_SINGLESTEP | _TIF_SYSCALL_EMU)) == _TIF_SINGLESTEP);

For me, this is same as:
step = ((flags & _TIF_SINGLESTEP) == _TIF_SINGLESTEP)
or
if (flags & _TIF_SINGLESTEP)
step = true;

I don't think so! As I mentioned in the last email loop, when PTRACE_SYSEMU_SINGLESTEP
is enabled, both the _TIF_SYSCALL_EMU and _TIF_SINGLESTEP flags are set, in which case
the step should be "false" for the old logic. But with the new logic, the step is "true".

quoted

So when PTRACE_SYSEMU_SINGLESTEP, _TIF_SYSCALL_EMU and _TIF_SINGLESTEP
are set and step evaluates to true.

So dropping _TIF_SYSCALL_EMU here should be fine. Am I still missing
something ?

--
Regards,
Sudeep

For the PTRACE_SYSEMU_SINGLESTEP request, ptrace only need to report(send SIGTRAP)
at the entry of a system call, no need to report at the exit of a system call.That's
why the old logic-{step = ((flags & (_TIF_SINGLESTEP | _TIF_SYSCALL_EMU)) == _TIF_SINGLESTEP)}
here try to filter out the special case(PTRACE_SYSEMU_SINGLESTEP).

Another way to make sure the logic is fine, you can run some tests with respect to both logic,
and to check whether they have the same behavior.

tools/testing/selftests/x86/ptrace_syscall.c has a test intended to
exercise this.  Can one of you either confirm that it does exercise it
and that it still passes or can you improve the test?

Thanks,
Andy

On Mon, Mar 11, 2019 at 08:04:39PM -0700, Andy Lutomirski wrote:

On Mon, Mar 11, 2019 at 6:35 PM Haibo Xu (Arm Technology China)
[off-list ref] wrote:

quoted

[...]

quoted

For the PTRACE_SYSEMU_SINGLESTEP request, ptrace only need to report(send
SIGTRAP) at the entry of a system call, no need to report at the exit of a
system call.That's why the old logic-{step = ((flags & (_TIF_SINGLESTEP |
_TIF_SYSCALL_EMU)) == _TIF_SINGLESTEP)} here try to filter out the special
case(PTRACE_SYSEMU_SINGLESTEP).

Another way to make sure the logic is fine, you can run some tests with
respect to both logic, and to check whether they have the same behavior.

tools/testing/selftests/x86/ptrace_syscall.c has a test intended to
exercise this.  Can one of you either confirm that it does exercise it
and that it still passes or can you improve the test?

I did run the tests which didn't flag anything. I haven't looked at the
details of test implementation, but seem to miss this case. I will see
what can be improved(if it's possible). Also I think single_step_syscall
is the one I need to look for this particular one. Both single_step_syscall
ptrace_syscall reported no errors.

--
Regards,
Sudeep

On 2019/3/12 20:09, Sudeep Holla wrote:

On Mon, Mar 11, 2019 at 08:04:39PM -0700, Andy Lutomirski wrote:

quoted

On Mon, Mar 11, 2019 at 6:35 PM Haibo Xu (Arm Technology China)
[off-list ref] wrote:

quoted

[...]

quoted

quoted

For the PTRACE_SYSEMU_SINGLESTEP request, ptrace only need to report(send
SIGTRAP) at the entry of a system call, no need to report at the exit of a
system call.That's why the old logic-{step = ((flags & (_TIF_SINGLESTEP |
_TIF_SYSCALL_EMU)) == _TIF_SINGLESTEP)} here try to filter out the special
case(PTRACE_SYSEMU_SINGLESTEP).

Another way to make sure the logic is fine, you can run some tests with
respect to both logic, and to check whether they have the same behavior.

tools/testing/selftests/x86/ptrace_syscall.c has a test intended to
exercise this.  Can one of you either confirm that it does exercise it
and that it still passes or can you improve the test?

I did run the tests which didn't flag anything. I haven't looked at the
details of test implementation, but seem to miss this case. I will see
what can be improved(if it's possible). Also I think single_step_syscall
is the one I need to look for this particular one. Both single_step_syscall
ptrace_syscall reported no errors.

--
Regards,
Sudeep

Since ptrace() system call do have so many request type, I'm not sure whether the
test cases have covered all of that. But here we'd better make sure the PTRACE_SYSEMU
and PTRACE_SYSEMU_SINGLESTEP requests are work correctly. May be you can verify them with
tests from Bin Lu(bin.lu@arm.com).

Regards,
Haibo
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

On Wed, Mar 13, 2019 at 01:03:18AM +0000, Haibo Xu (Arm Technology China) wrote:
[...]

Since ptrace() system call do have so many request type, I'm not sure
whether the test cases have covered all of that. But here we'd better make
sure the PTRACE_SYSEMU and PTRACE_SYSEMU_SINGLESTEP requests are work
correctly. May be you can verify them with tests from Bin Lu(bin.lu@arm.com).

Sure happy to try them. Can you point me to them ?
I did end up writing few more tests.

--
Regards,
Sudeep

On 2019/3/14 18:51, Sudeep Holla wrote:

On Wed, Mar 13, 2019 at 01:03:18AM +0000, Haibo Xu (Arm Technology China) wrote:
[...]

quoted

Since ptrace() system call do have so many request type, I'm not sure
whether the test cases have covered all of that. But here we'd better make
sure the PTRACE_SYSEMU and PTRACE_SYSEMU_SINGLESTEP requests are work
correctly. May be you can verify them with tests from Bin Lu(bin.lu@arm.com).

Sure happy to try them. Can you point me to them ?
I did end up writing few more tests.

--
Regards,
Sudeep

You can get them from Steve Capper. BTW, I also have a program to verify the
PTRACE_SYSEMU function, and will send to you in a separate email loop.

Regards,
Haibo
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

On Tue, Mar 12, 2019 at 01:34:44AM +0000, Haibo Xu (Arm Technology China) wrote:

On 2019/3/12 2:34, Sudeep Holla wrote:

quoted

(I thought I had sent this email, last Tuesday itself, but saw this in my
draft today, something went wrong, sorry for the delay)

On Tue, Mar 05, 2019 at 02:14:47AM +0000, Haibo Xu (Arm Technology China) wrote:

quoted

On 2019/3/4 18:12, Sudeep Holla wrote:

quoted

On Mon, Mar 04, 2019 at 08:25:28AM +0000, Haibo Xu (Arm Technology China) wrote:

quoted

On 2019/3/1 2:32, Sudeep Holla wrote:

quoted

Now that we have a new hook ptrace_syscall_enter that can be called from
syscall entry code and it handles PTRACE_SYSEMU in generic code, we
can do some cleanup using the same in syscall_trace_enter.

Further the extra logic to find single stepping PTRACE_SYSEMU_SINGLESTEP
in syscall_slow_exit_work seems unnecessary. Let's remove the same.

I think we should not change the logic here. Is so, it will double the report of syscall
when PTRACE_SYSEMU_SINGLESTEP is enabled.

I don't think that should happen, but I may be missing something.
Can you explain how ?

When PTRACE_SYSEMU_SINGLESTEP is enabled, both the _TIF_SYSCALL_EMU and
_TIF_SINGLESTEP flags are set, but ptrace only need to report(send SIGTRAP)
at the entry of a system call, no need to report at the exit of a system
call.

Sorry, but I still not get it, we have:

	step = ((flags & (_TIF_SINGLESTEP | _TIF_SYSCALL_EMU)) == _TIF_SINGLESTEP);

For me, this is same as:
	step = ((flags & _TIF_SINGLESTEP) == _TIF_SINGLESTEP)
	or
	if (flags & _TIF_SINGLESTEP)
		step = true;

I don't think so! As I mentioned in the last email loop, when
PTRACE_SYSEMU_SINGLESTE is enabled, both the _TIF_SYSCALL_EMU and
_TIF_SINGLESTEP flags are set, in which case the step should be "false" for
the old logic. But with the new logic, the step is "true".

Ah right, sorry I missed that.

quoted

So when PTRACE_SYSEMU_SINGLESTEP, _TIF_SYSCALL_EMU and _TIF_SINGLESTEP
are set and step evaluates to true.

So dropping _TIF_SYSCALL_EMU here should be fine. Am I still missing
something ?

--
Regards,
Sudeep

For the PTRACE_SYSEMU_SINGLESTEP request, ptrace only need to report(send
SIGTRAP) at the entry of a system call, no need to report at the exit of a
system call.That's why the old logic-{step = ((flags & (_TIF_SINGLESTEP |
_TIF_SYSCALL_EMU)) == _TIF_SINGLESTEP)} here try to filter out the special
case(PTRACE_SYSEMU_SINGLESTEP).

Understood

Another way to make sure the logic is fine, you can run some tests with
respect to both logic, and to check whether they have the same behavior.

I did run selftests after Andy Lutomirski pointed out. Nothing got flagged,
I haven't looked at the tests themselves yet, but it clearly misses this
case.

--
Regards,
Sudeep

Now that we have a new hook ptrace_syscall_enter that can be called from
syscall entry code and it handles PTRACE_SYSEMU in generic code, we
can do some cleanup using the same in do_syscall_trace_enter.

Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Paul Mackerras <redacted>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Sudeep Holla <redacted>
---
 arch/powerpc/kernel/ptrace.c | 50 ++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 28 deletions(-)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index cb7e1439cafb..978cd2aac29e 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c

@@ -3264,37 +3264,31 @@ long do_syscall_trace_enter(struct pt_regs *regs)
 {
 	u32 flags;
 
-	user_exit();
-
-	flags = READ_ONCE(current_thread_info()->flags) &
-		(_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
+	if (unlikely(ptrace_syscall_enter(regs))) {
+		/*
+		 * A nonzero return code from tracehook_report_syscall_entry()
+		 * tells us to prevent the syscall execution, but we are not
+		 * going to execute it anyway.
+		 *
+		 * Returning -1 will skip the syscall execution. We want to
+		 * avoid clobbering any registers, so we don't goto the skip
+		 * label below.
+		 */
+		return -1;
+	}
 
-	if (flags) {
-		int rc = tracehook_report_syscall_entry(regs);
+	user_exit();
 
-		if (unlikely(flags & _TIF_SYSCALL_EMU)) {
-			/*
-			 * A nonzero return code from
-			 * tracehook_report_syscall_entry() tells us to prevent
-			 * the syscall execution, but we are not going to
-			 * execute it anyway.
-			 *
-			 * Returning -1 will skip the syscall execution. We want
-			 * to avoid clobbering any registers, so we don't goto
-			 * the skip label below.
-			 */
-			return -1;
-		}
+	flags = READ_ONCE(current_thread_info()->flags) & _TIF_SYSCALL_TRACE;
 
-		if (rc) {
-			/*
-			 * The tracer decided to abort the syscall. Note that
-			 * the tracer may also just change regs->gpr[0] to an
-			 * invalid syscall number, that is handled below on the
-			 * exit path.
-			 */
-			goto skip;
-		}
+	if (flags && tracehook_report_syscall_entry(regs)) {
+		/*
+		 * The tracer decided to abort the syscall. Note that
+		 * the tracer may also just change regs->gpr[0] to an
+		 * invalid syscall number, that is handled below on the
+		 * exit path.
+		 */
+		goto skip;
 	}
 
 	/* Run seccomp after ptrace; allow it to set gpr[3]. */

-- 
2.17.1

On 2019/3/1 2:32, Sudeep Holla wrote:

We'd better keep the user_exit() at here in case both context tracking and SYSCALL_EMU
are enabled.

-
-flags = READ_ONCE(current_thread_info()->flags) &
-(_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
+if (unlikely(ptrace_syscall_enter(regs))) {
+/*
+ * A nonzero return code from tracehook_report_syscall_entry()
+ * tells us to prevent the syscall execution, but we are not
+ * going to execute it anyway.
+ *
+ * Returning -1 will skip the syscall execution. We want to
+ * avoid clobbering any registers, so we don't goto the skip
+ * label below.
+ */
+return -1;
+}

-if (flags) {
-int rc = tracehook_report_syscall_entry(regs);
+user_exit();

-if (unlikely(flags & _TIF_SYSCALL_EMU)) {
-/*
- * A nonzero return code from
- * tracehook_report_syscall_entry() tells us to prevent
- * the syscall execution, but we are not going to
- * execute it anyway.
- *
- * Returning -1 will skip the syscall execution. We want
- * to avoid clobbering any registers, so we don't goto
- * the skip label below.
- */
-return -1;
-}
+flags = READ_ONCE(current_thread_info()->flags) & _TIF_SYSCALL_TRACE;

-if (rc) {
-/*
- * The tracer decided to abort the syscall. Note that
- * the tracer may also just change regs->gpr[0] to an
- * invalid syscall number, that is handled below on the
- * exit path.
- */
-goto skip;
-}
+if (flags && tracehook_report_syscall_entry(regs)) {
+/*
+ * The tracer decided to abort the syscall. Note that
+ * the tracer may also just change regs->gpr[0] to an
+ * invalid syscall number, that is handled below on the
+ * exit path.
+ */
+goto skip;
 }

 /* Run seccomp after ptrace; allow it to set gpr[3]. */

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

On Mon, Mar 04, 2019 at 09:36:27AM +0000, Haibo Xu (Arm Technology China) wrote:

On 2019/3/1 2:32, Sudeep Holla wrote:

quoted

Now that we have a new hook ptrace_syscall_enter that can be called from
syscall entry code and it handles PTRACE_SYSEMU in generic code, we
can do some cleanup using the same in do_syscall_trace_enter.

Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Paul Mackerras <redacted>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Sudeep Holla <redacted>
---
 arch/powerpc/kernel/ptrace.c | 50 ++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 28 deletions(-)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index cb7e1439cafb..978cd2aac29e 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c

@@ -3264,37 +3264,31 @@ long do_syscall_trace_enter(struct pt_regs *regs)
 {
 	u32 flags;

-	user_exit();

We'd better keep the user_exit() at here in case both context tracking and
SYSCALL_EMU are enabled.

Ah right, spurious change will fix it.

--
Regards,
Sudeep

x86 and um use 31 and 32 for PTRACE_SYSEMU and PTRACE_SYSEMU_SINGLESTEP
while powerpc uses different value maybe for legacy reasons.

Though handling of PTRACE_SYSEMU can be made architecture independent,
it's hard to make these definations generic. To add to this existing
mess few architectures like arm, c6x and sh use 31 for PTRACE_GETFDPIC
(get the ELF fdpic loadmap address). It's not possible to move the
definations to generic headers.

So we unfortunately have to duplicate the same defination to ARM64 if
we need to support PTRACE_SYSEMU.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <redacted>
Signed-off-by: Sudeep Holla <redacted>
---
 arch/arm64/include/uapi/asm/ptrace.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/include/uapi/asm/ptrace.h b/arch/arm64/include/uapi/asm/ptrace.h
index 28d77c9ed531..8478b9007f9e 100644
--- a/arch/arm64/include/uapi/asm/ptrace.h
+++ b/arch/arm64/include/uapi/asm/ptrace.h

@@ -62,6 +62,9 @@
 #define PSR_x		0x0000ff00	/* Extension		*/
 #define PSR_c		0x000000ff	/* Control		*/
 
+/* syscall emulation path in ptrace */
+#define PTRACE_SYSEMU		  31
+#define PTRACE_SYSEMU_SINGLESTEP  32
 
 #ifndef __ASSEMBLY__
 

-- 
2.17.1

Add PTRACE_SYSEMU and PTRACE_SYSEMU_SINGLESTEP support on arm64.
We can just make sure of the generic ptrace_syscall_enter hook to
support PTRACE_SYSEMU. We don't need any special handling for
PTRACE_SYSEMU_SINGLESTEP.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <redacted>
Signed-off-by: Sudeep Holla <redacted>
---
 arch/arm64/include/asm/thread_info.h | 5 ++++-
 arch/arm64/kernel/ptrace.c           | 3 +++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
index bbca68b54732..c86aeb6379d5 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h

@@ -75,6 +75,7 @@ void arch_release_task_struct(struct task_struct *tsk);
  *  TIF_SYSCALL_TRACE	- syscall trace active
  *  TIF_SYSCALL_TRACEPOINT - syscall tracepoint for ftrace
  *  TIF_SYSCALL_AUDIT	- syscall auditing
+ *  TIF_SYSCALL_EMU     - syscall emulation active
  *  TIF_SECOMP		- syscall secure computing
  *  TIF_SIGPENDING	- signal pending
  *  TIF_NEED_RESCHED	- rescheduling necessary

@@ -92,6 +93,7 @@ void arch_release_task_struct(struct task_struct *tsk);
 #define TIF_SYSCALL_AUDIT	9
 #define TIF_SYSCALL_TRACEPOINT	10
 #define TIF_SECCOMP		11
+#define TIF_SYSCALL_EMU		12
 #define TIF_MEMDIE		18	/* is terminating due to OOM killer */
 #define TIF_FREEZE		19
 #define TIF_RESTORE_SIGMASK	20

@@ -110,6 +112,7 @@ void arch_release_task_struct(struct task_struct *tsk);
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
 #define _TIF_SYSCALL_TRACEPOINT	(1 << TIF_SYSCALL_TRACEPOINT)
 #define _TIF_SECCOMP		(1 << TIF_SECCOMP)
+#define _TIF_SYSCALL_EMU	(1 << TIF_SYSCALL_EMU)
 #define _TIF_UPROBE		(1 << TIF_UPROBE)
 #define _TIF_FSCHECK		(1 << TIF_FSCHECK)
 #define _TIF_32BIT		(1 << TIF_32BIT)

@@ -121,7 +124,7 @@ void arch_release_task_struct(struct task_struct *tsk);
 
 #define _TIF_SYSCALL_WORK	(_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \
 				 _TIF_SYSCALL_TRACEPOINT | _TIF_SECCOMP | \
-				 _TIF_NOHZ)
+				 _TIF_NOHZ | _TIF_SYSCALL_EMU)
 
 #define INIT_THREAD_INFO(tsk)						\
 {									\

diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index ddaea0fd2fa4..c377ce597f92 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c

@@ -1672,6 +1672,9 @@ static void tracehook_report_syscall(struct pt_regs *regs,
 
 int syscall_trace_enter(struct pt_regs *regs)
 {
+	if (unlikely(ptrace_syscall_enter(regs)))
+		return -1;
+
 	if (test_thread_flag(TIF_SYSCALL_TRACE))
 		tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);
 

-- 
2.17.1