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

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

[Sudeep Holla]

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.

v1->v2:
	- added comment for empty statement after tracehook_report_syscall_entry
	- dropped x86 change in syscall_slow_exit_work as I had ended
	  up changing logic unintentionally
	- removed spurious change in powerpc moving user_exit()

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         | 49 ++++++++++++----------------
 arch/x86/entry/common.c              | 12 ++-----
 arch/x86/kernel/ptrace.c             |  3 --
 include/linux/ptrace.h               |  1 +
 kernel/ptrace.c                      | 26 +++++++++++++++
 8 files changed, 61 insertions(+), 41 deletions(-)

--
2.17.1

[PATCH v2 1/6] ptrace: move clearing of TIF_SYSCALL_EMU flag to core

[Sudeep Holla]

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 <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 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 d9ac7d94656e..2e2183b800a8 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -2520,7 +2520,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

[PATCH v2 2/6] ptrace: introduce ptrace_syscall_enter to consolidate PTRACE_SYSEMU handling

[Sudeep Holla]

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 <sudeep.holla@arm.com>
---
 include/linux/ptrace.h |  1 +
 kernel/ptrace.c        | 22 ++++++++++++++++++++++
 2 files changed, 23 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..c9c505c483df 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,27 @@ 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))
+			/*
+			 * We can ignore the return code here as we need
+			 * return -1 always for syscall emulation irrespective
+			 * of whether the tracehook report fails or succeed.
+			 */
+			;
+		return -1L;
+	}
+#endif
+	return 0;
+}
+
 /*
  * Detach all tasks we were using ptrace on. Called with tasklist held
  * for writing.
-- 
2.17.1

[PATCH v2 3/6] x86: clean up _TIF_SYSCALL_EMU handling using ptrace_syscall_enter hook

[Sudeep Holla]

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 <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/x86/entry/common.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index 7bc105f47d21..5d7590994964 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
-- 
2.17.1

[PATCH v2 4/6] powerpc: use common ptrace_syscall_enter hook to handle _TIF_SYSCALL_EMU

[Sudeep Holla]

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 <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/powerpc/kernel/ptrace.c | 48 ++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 27 deletions(-)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 2e2183b800a8..05579a5dcb12 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -3278,35 +3278,29 @@ long do_syscall_trace_enter(struct pt_regs *regs)
 
 	user_exit();
 
-	flags = READ_ONCE(current_thread_info()->flags) &
-		(_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
-
-	if (flags) {
-		int rc = tracehook_report_syscall_entry(regs);
+	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 (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

[PATCH v2 5/6] arm64: add PTRACE_SYSEMU{,SINGLESTEP} definations to uapi headers

[Sudeep Holla]

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 <will.deacon@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 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 d78623acb649..627ac57c1581 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

[PATCH v2 6/6] arm64: ptrace: add support for syscall emulation

[Sudeep Holla]

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 <will.deacon@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 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 eb3ef73e07cf..c285d1ce7186 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
@@ -91,6 +92,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
@@ -109,6 +111,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)
@@ -120,7 +123,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 b82e0a9b3da3..cf29275cd4d9 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -1819,6 +1819,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

Re: [PATCH v2 4/6] powerpc: use common ptrace_syscall_enter hook to handle _TIF_SYSCALL_EMU

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 1573 bytes --]

On Mon, Mar 18, 2019 at 10:49:23AM +0000, 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 do_syscall_trace_enter.
> 
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  arch/powerpc/kernel/ptrace.c | 48 ++++++++++++++++--------------------
>  1 file changed, 21 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index 2e2183b800a8..05579a5dcb12 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -3278,35 +3278,29 @@ long do_syscall_trace_enter(struct pt_regs *regs)
>  
>  	user_exit();
>  
> -	flags = READ_ONCE(current_thread_info()->flags) &
> -		(_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
> -
> -	if (flags) {
> -		int rc = tracehook_report_syscall_entry(regs);
> +	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;
> +	}

This comment is out of sync with the changed code.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 2/6] ptrace: introduce ptrace_syscall_enter to consolidate PTRACE_SYSEMU handling

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 2341 bytes --]

On Mon, Mar 18, 2019 at 10:49:21AM +0000, 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.
> 
> Cc: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  include/linux/ptrace.h |  1 +
>  kernel/ptrace.c        | 22 ++++++++++++++++++++++
>  2 files changed, 23 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..c9c505c483df 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,27 @@ 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))
> +			/*
> +			 * We can ignore the return code here as we need
> +			 * return -1 always for syscall emulation irrespective
> +			 * of whether the tracehook report fails or succeed.
> +			 */
> +			;

This is problematic as it causes build errors with -Werror=empty-body,
see https://lore.kernel.org/lkml/20181218205305.26647-1-malat@debian.org/


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 2/6] ptrace: introduce ptrace_syscall_enter to consolidate PTRACE_SYSEMU handling

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 601 bytes --]

On Mon, Mar 18, 2019 at 10:49:21AM +0000, 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.

Sorry if I'm late for the party, but the new name looks confusing.
If all it does is related to TIF_SYSCALL_EMU, why does it have a generic
name 'ptrace_syscall_enter' without any hint of being specific to
TIF_SYSCALL_EMU?


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 2/6] ptrace: introduce ptrace_syscall_enter to consolidate PTRACE_SYSEMU handling

[Sudeep Holla]

On Mon, Mar 18, 2019 at 05:31:47PM +0300, Dmitry V. Levin wrote:
> On Mon, Mar 18, 2019 at 10:49:21AM +0000, 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.
> > 
> > Cc: Oleg Nesterov <oleg@redhat.com>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  include/linux/ptrace.h |  1 +
> >  kernel/ptrace.c        | 22 ++++++++++++++++++++++
> >  2 files changed, 23 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..c9c505c483df 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,27 @@ 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))
> > +			/*
> > +			 * We can ignore the return code here as we need
> > +			 * return -1 always for syscall emulation irrespective
> > +			 * of whether the tracehook report fails or succeed.
> > +			 */
> > +			;
> 
> This is problematic as it causes build errors with -Werror=empty-body,
> see https://lore.kernel.org/lkml/20181218205305.26647-1-malat@debian.org/
> 

Thanks for the pointer, will update.

--
Regards,
Sudeep

Re: [PATCH v2 2/6] ptrace: introduce ptrace_syscall_enter to consolidate PTRACE_SYSEMU handling

[Sudeep Holla]

On Mon, Mar 18, 2019 at 05:41:15PM +0300, Dmitry V. Levin wrote:
> On Mon, Mar 18, 2019 at 10:49:21AM +0000, 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.
> 
> Sorry if I'm late for the party, but the new name looks confusing.
> If all it does is related to TIF_SYSCALL_EMU, why does it have a generic
> name 'ptrace_syscall_enter' without any hint of being specific to
> TIF_SYSCALL_EMU?
> 

Not at all late. Infact Haibo Xu pointed that out, I updated but somehow
missed to commit and lost those changes. I will rename as
ptrace_sysemu_syscall_enter

--
Regards,
Sudeep

Re: [PATCH v2 4/6] powerpc: use common ptrace_syscall_enter hook to handle _TIF_SYSCALL_EMU

[Sudeep Holla]

On Mon, Mar 18, 2019 at 05:26:18PM +0300, Dmitry V. Levin wrote:
> On Mon, Mar 18, 2019 at 10:49:23AM +0000, 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 do_syscall_trace_enter.
> > 
> > Cc: Oleg Nesterov <oleg@redhat.com>
> > Cc: Paul Mackerras <paulus@samba.org>
> > Cc: Michael Ellerman <mpe@ellerman.id.au>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  arch/powerpc/kernel/ptrace.c | 48 ++++++++++++++++--------------------
> >  1 file changed, 21 insertions(+), 27 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> > index 2e2183b800a8..05579a5dcb12 100644
> > --- a/arch/powerpc/kernel/ptrace.c
> > +++ b/arch/powerpc/kernel/ptrace.c
> > @@ -3278,35 +3278,29 @@ long do_syscall_trace_enter(struct pt_regs *regs)
> >  
> >  	user_exit();
> >  
> > -	flags = READ_ONCE(current_thread_info()->flags) &
> > -		(_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
> > -
> > -	if (flags) {
> > -		int rc = tracehook_report_syscall_entry(regs);
> > +	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;
> > +	}
> 
> This comment is out of sync with the changed code.

Still applicable indirectly as ptrace_syscall_enter just executes
tracehook_report_syscall_entry, but I agree needs rewording, will update.

--
Regards,
Sudeep

Re: [PATCH v2 3/6] x86: clean up _TIF_SYSCALL_EMU handling using ptrace_syscall_enter hook

[Oleg Nesterov]

On 03/18, Sudeep Holla wrote:
>
> --- 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;

Well, I won't really argue, but to be honest I think this change doesn't make
the code better... With this patch tracehook_report_syscall_entry() has 2 callers,
to me this just adds some confusion.

I agree that the usage of emulated/_TIF_SYSCALL_EMU looks a bit overcomplicated,
I'd suggest a simple cleanup below.

And it seems that _TIF_WORK_SYSCALL_ENTRY needs some cleanups too... We don't need
"& _TIF_WORK_SYSCALL_ENTRY" in syscall_trace_enter, and _TIF_WORK_SYSCALL_ENTRY
should not include _TIF_NOHZ?

Oleg.


--- x/arch/x86/entry/common.c
+++ x/arch/x86/entry/common.c
@@ -70,23 +70,18 @@ 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;
+	work = READ_ONCE(ti->flags);
 
-	if (unlikely(work & _TIF_SYSCALL_EMU))
-		emulated = true;
-
-	if ((emulated || (work & _TIF_SYSCALL_TRACE)) &&
-	    tracehook_report_syscall_entry(regs))
-		return -1L;
-
-	if (emulated)
-		return -1L;
+	if (work & (_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE)) {
+		ret = tracehook_report_syscall_entry(regs);
+		if (ret || (work & _TIF_SYSCALL_EMU))
+			return -1L;
+	}
 
 #ifdef CONFIG_SECCOMP
 	/*

Re: [PATCH v2 4/6] powerpc: use common ptrace_syscall_enter hook to handle _TIF_SYSCALL_EMU

[Oleg Nesterov]

On 03/18, Sudeep Holla wrote:
>
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -3278,35 +3278,29 @@ long do_syscall_trace_enter(struct pt_regs *regs)
>  
>  	user_exit();
>  
> -	flags = READ_ONCE(current_thread_info()->flags) &
> -		(_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
> -
> -	if (flags) {
> -		int rc = tracehook_report_syscall_entry(regs);
> +	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 (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;

Why do we need READ_ONCE() with this change?

And now that we change a single bit "flags" doesn't look like a good name.

Again, to me this patch just makes the code look worse. Honestly, I don't
think that the new (badly named) ptrace_syscall_enter() hook makes any sense.

Oleg.

Re: [PATCH v2 4/6] powerpc: use common ptrace_syscall_enter hook to handle _TIF_SYSCALL_EMU

[Sudeep Holla]

On Mon, Mar 18, 2019 at 06:20:24PM +0100, Oleg Nesterov wrote:
> On 03/18, Sudeep Holla wrote:
> >
> > --- a/arch/powerpc/kernel/ptrace.c
> > +++ b/arch/powerpc/kernel/ptrace.c
> > @@ -3278,35 +3278,29 @@ long do_syscall_trace_enter(struct pt_regs *regs)
> >
> >  	user_exit();
> >
> > -	flags = READ_ONCE(current_thread_info()->flags) &
> > -		(_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
> > -
> > -	if (flags) {
> > -		int rc = tracehook_report_syscall_entry(regs);
> > +	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 (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;
>
> Why do we need READ_ONCE() with this change?
>
> And now that we change a single bit "flags" doesn't look like a good name.
>
> Again, to me this patch just makes the code look worse. Honestly, I don't
> think that the new (badly named) ptrace_syscall_enter() hook makes any sense.
>

Worse because we end up reading current_thread_info->flags twice ?

--
Regards,
Sudeep

Re: [PATCH v2 1/6] ptrace: move clearing of TIF_SYSCALL_EMU flag to core

[Oleg Nesterov]

On 03/18, Sudeep Holla wrote:
>
 @@ -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

perhaps it makes sense to factor out clear_tsk_thread_flag(TIF_SYSCALL_EMU), but
then we should probably clear it along with TIF_SYSCALL_TRACE in __ptrace_unlink?

Oleg.

Re: [PATCH v2 4/6] powerpc: use common ptrace_syscall_enter hook to handle _TIF_SYSCALL_EMU

[Oleg Nesterov]

On 03/18, Sudeep Holla wrote:
>
> On Mon, Mar 18, 2019 at 06:20:24PM +0100, Oleg Nesterov wrote:
> >
> > Again, to me this patch just makes the code look worse. Honestly, I don't
> > think that the new (badly named) ptrace_syscall_enter() hook makes any sense.
> >
>
> Worse because we end up reading current_thread_info->flags twice ?

Mostly because in my opinion ptrace_syscall_enter() buys nothing but makes
the caller's code less readable/understandable.

Sure, this is subjective.

Oleg.

Re: [PATCH v2 4/6] powerpc: use common ptrace_syscall_enter hook to handle _TIF_SYSCALL_EMU

[Sudeep Holla]

On Mon, Mar 18, 2019 at 06:33:41PM +0100, Oleg Nesterov wrote:
> On 03/18, Sudeep Holla wrote:
> >
> > On Mon, Mar 18, 2019 at 06:20:24PM +0100, Oleg Nesterov wrote:
> > >
> > > Again, to me this patch just makes the code look worse. Honestly, I don't
> > > think that the new (badly named) ptrace_syscall_enter() hook makes any sense.
> > >
> >
> > Worse because we end up reading current_thread_info->flags twice ?
>
> Mostly because in my opinion ptrace_syscall_enter() buys nothing but makes
> the caller's code less readable/understandable.
>
> Sure, this is subjective.
>

Based on what we have in that function today, I tend to agree. Will and
Richard were in the opinion to consolidate SYSEMU handling(in the threads
pointed in my cover letter). If there's a better way to achieve the same
I am in for it. I have just tried to put something together based on
what I could think of.

--
Regards,
Sudeep

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

[Andy Lutomirski]

On Mon, Mar 18, 2019 at 3:49 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> 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.
>

In the discussion from the first version, there was talk of some
testing.  Can you put the test cases in question into v3?

--Andy

Re: [PATCH v2 6/6] arm64: ptrace: add support for syscall emulation

[Haibo Xu (Arm Technology China)]

On 2019/3/18 18:49, Sudeep Holla wrote:
> 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.

This looks good to me. But it'd be better to add the same logic to handle
PTRACE_SYSEMU_SINGLESTEP as that of x86 in case we may need enable the single
step trace function in the future.

>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  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 eb3ef73e07cf..c285d1ce7186 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
> @@ -91,6 +92,7 @@ void arch_release_task_struct(struct task_struct *tsk);
>  #define TIF_SYSCALL_AUDIT9
>  #define TIF_SYSCALL_TRACEPOINT10
>  #define TIF_SECCOMP11
> +#define TIF_SYSCALL_EMU12
>  #define TIF_MEMDIE18/* is terminating due to OOM killer */
>  #define TIF_FREEZE19
>  #define TIF_RESTORE_SIGMASK20
> @@ -109,6 +111,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)
> @@ -120,7 +123,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 b82e0a9b3da3..cf29275cd4d9 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -1819,6 +1819,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);
>
>
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.

Re: [PATCH v2 4/6] powerpc: use common ptrace_syscall_enter hook to handle _TIF_SYSCALL_EMU

[Oleg Nesterov]

On 03/18, Sudeep Holla wrote:
>
> On Mon, Mar 18, 2019 at 06:33:41PM +0100, Oleg Nesterov wrote:
> > On 03/18, Sudeep Holla wrote:
> > >
> > > On Mon, Mar 18, 2019 at 06:20:24PM +0100, Oleg Nesterov wrote:
> > > >
> > > > Again, to me this patch just makes the code look worse. Honestly, I don't
> > > > think that the new (badly named) ptrace_syscall_enter() hook makes any sense.
> > > >
> > >
> > > Worse because we end up reading current_thread_info->flags twice ?
> >
> > Mostly because in my opinion ptrace_syscall_enter() buys nothing but makes
> > the caller's code less readable/understandable.
> >
> > Sure, this is subjective.
> >
>
> Based on what we have in that function today, I tend to agree. Will and
> Richard were in the opinion to consolidate SYSEMU handling

Well, personally I see no point... Again, after the trivial simplification
x86 does

	if (work & (_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE)) {
		ret = tracehook_report_syscall_entry(regs);
		if (ret || (work & _TIF_SYSCALL_EMU))
			return -1L;
	}

this looks simple enough for copy-and-paste.

> If there's a better way to achieve the same

I can only say that if we add a common helper, I think it should absorb
tracehook_report_syscall_entry() and handle both TIF's just like the code
above does. Not sure this makes any sense.

Oleg.

Re: [PATCH v2 4/6] powerpc: use common ptrace_syscall_enter hook to handle _TIF_SYSCALL_EMU

[Oleg Nesterov]

On 03/19, Oleg Nesterov wrote:
>
> Well, personally I see no point... Again, after the trivial simplification
> x86 does
>
> 	if (work & (_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE)) {
> 		ret = tracehook_report_syscall_entry(regs);
> 		if (ret || (work & _TIF_SYSCALL_EMU))
> 			return -1L;
> 	}
>
> this looks simple enough for copy-and-paste.
>
> > If there's a better way to achieve the same
>
> I can only say that if we add a common helper, I think it should absorb
> tracehook_report_syscall_entry() and handle both TIF's just like the code
> above does. Not sure this makes any sense.

this won't work, looking at 6/6 I see that arm64 needs to distinguish
_TRACE and _EMU ... I don't understand this code, but it looks suspicious.
If tracehook_report_syscall_entry() returns nonzero the tracee was killed,
syscall_trace_enter() should just return.

To me this is another indication that consolidation makes no sense ;)

Oleg.

Re: [PATCH v2 4/6] powerpc: use common ptrace_syscall_enter hook to handle _TIF_SYSCALL_EMU

[Will Deacon]

Hi Oleg,

On Tue, Mar 19, 2019 at 06:32:33PM +0100, Oleg Nesterov wrote:
> On 03/19, Oleg Nesterov wrote:
> >
> > Well, personally I see no point... Again, after the trivial simplification
> > x86 does
> >
> > 	if (work & (_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE)) {
> > 		ret = tracehook_report_syscall_entry(regs);
> > 		if (ret || (work & _TIF_SYSCALL_EMU))
> > 			return -1L;
> > 	}
> >
> > this looks simple enough for copy-and-paste.
> >
> > > If there's a better way to achieve the same
> >
> > I can only say that if we add a common helper, I think it should absorb
> > tracehook_report_syscall_entry() and handle both TIF's just like the code
> > above does. Not sure this makes any sense.
> 
> this won't work, looking at 6/6 I see that arm64 needs to distinguish
> _TRACE and _EMU ... I don't understand this code, but it looks suspicious.
> If tracehook_report_syscall_entry() returns nonzero the tracee was killed,
> syscall_trace_enter() should just return.
> 
> To me this is another indication that consolidation makes no sense ;)

The reason I'm pushing for consolidation here is because I think it's the
only sane way to maintain the tracing and debug hooks on the syscall
entry/exit paths. Having to look at all the different arch implementations
and distil the portable semantics is a nightmare and encourages gradual
divergence over time. Given that we don't support this SYSCALL_EMU stuff
on arm64 today, we have the opportunity to make this generic and allow other
architectures (e.g. riscv) to hook in the same way that we do. It clearly
shouldn't affect the behaviour of existing architectures which already
support the functionality.

However, I also agree that this patch series looks dodgy as it stands -- we
shouldn't have code paths that can result in calling
tracehook_report_syscall_entry() twice.

Will

Re: [PATCH v2 3/6] x86: clean up _TIF_SYSCALL_EMU handling using ptrace_syscall_enter hook

[Sudeep Holla]

On Mon, Mar 18, 2019 at 04:33:22PM +0100, Oleg Nesterov wrote:
> On 03/18, Sudeep Holla wrote:
> >
> > --- 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;
>
[...]

>
> And it seems that _TIF_WORK_SYSCALL_ENTRY needs some cleanups too... We don't need
> "& _TIF_WORK_SYSCALL_ENTRY" in syscall_trace_enter, and _TIF_WORK_SYSCALL_ENTRY
> should not include _TIF_NOHZ?
>

I was about to post the updated version and checked this to make sure I have
covered everything or not. I had missed the above comment. All architectures
have _TIF_NOHZ in their mask that they check to do work. And from x86, I read
"...syscall_trace_enter(). Also includes TIF_NOHZ for enter_from_user_mode()"
So I don't understand why _TIF_NOHZ needs to be dropped.

Also if we need to drop, we can address that separately examining all archs.
I will post the cleanup as you suggested for now.

--
Regards,
Sudeep

Re: [PATCH v2 3/6] x86: clean up _TIF_SYSCALL_EMU handling using ptrace_syscall_enter hook

[Andy Lutomirski]

On Mon, Mar 18, 2019 at 3:49 AM Sudeep Holla <sudeep.holla@arm.com> 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.
>

Unless the patch set contains a selftest that exercises all the
interesting cases here, NAK.  To be clear, there needs to be a test
that passes on an unmodified kernel and still passes on a patched
kernel.  And that test case needs to *fail* if, for example, you force
"emulated" to either true or false rather than reading out the actual
value.

--Andy

Re: [PATCH v2 3/6] x86: clean up _TIF_SYSCALL_EMU handling using ptrace_syscall_enter hook

[Sudeep Holla]

On 30/04/2019 17:46, Andy Lutomirski wrote:
> On Mon, Mar 18, 2019 at 3:49 AM Sudeep Holla <sudeep.holla@arm.com> 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.
>>
> 
> Unless the patch set contains a selftest that exercises all the
> interesting cases here, NAK.  To be clear, there needs to be a test
> that passes on an unmodified kernel and still passes on a patched
> kernel.  And that test case needs to *fail* if, for example, you force
> "emulated" to either true or false rather than reading out the actual
> value.
> 

Tested using tools/testing/selftests/x86/ptrace_syscall.c

Also v3 doesn't change any logic or additional call to new function as
in v2. It's just simple cleanup as suggested by Oleg.

-- 
Regards,
Sudeep

Re: [PATCH v2 3/6] x86: clean up _TIF_SYSCALL_EMU handling using ptrace_syscall_enter hook

[Oleg Nesterov]

On 04/30, Sudeep Holla wrote:
>
> On Mon, Mar 18, 2019 at 04:33:22PM +0100, Oleg Nesterov wrote:
> >
> > And it seems that _TIF_WORK_SYSCALL_ENTRY needs some cleanups too... We don't need
> > "& _TIF_WORK_SYSCALL_ENTRY" in syscall_trace_enter, and _TIF_WORK_SYSCALL_ENTRY
> > should not include _TIF_NOHZ?
> >
>
> I was about to post the updated version and checked this to make sure I have
> covered everything or not. I had missed the above comment. All architectures
> have _TIF_NOHZ in their mask that they check to do work. And from x86, I read
> "...syscall_trace_enter(). Also includes TIF_NOHZ for enter_from_user_mode()"
> So I don't understand why _TIF_NOHZ needs to be dropped.

I have already forgot this discussion... But after I glanced at this code again
I still think the same, and I don't understand why do you disagree.

> Also if we need to drop, we can address that separately examining all archs.

Sure, and I was only talking about x86. We can keep TIF_NOHZ and even
set_tsk_thread_flag(TIF_NOHZ) in context_tracking_cpu_set() if some arch needs
this but remove TIF_NOHZ from TIF_WORK_SYSCALL_ENTRY in arch/x86/include/asm/thread_info.h,
afaics this shouldn't make any difference.

And I see no reason why x86 needs to use TIF_WORK_SYSCALL_ENTRY in
syscall_trace_enter().

Oleg.

Re: [PATCH v2 3/6] x86: clean up _TIF_SYSCALL_EMU handling using ptrace_syscall_enter hook

[Sudeep Holla]

On Wed, May 01, 2019 at 05:57:11PM +0200, Oleg Nesterov wrote:
> On 04/30, Sudeep Holla wrote:
> >
> > On Mon, Mar 18, 2019 at 04:33:22PM +0100, Oleg Nesterov wrote:
> > >
> > > And it seems that _TIF_WORK_SYSCALL_ENTRY needs some cleanups too... We don't need
> > > "& _TIF_WORK_SYSCALL_ENTRY" in syscall_trace_enter, and _TIF_WORK_SYSCALL_ENTRY
> > > should not include _TIF_NOHZ?
> > >
> >
> > I was about to post the updated version and checked this to make sure I have
> > covered everything or not. I had missed the above comment. All architectures
> > have _TIF_NOHZ in their mask that they check to do work. And from x86, I read
> > "...syscall_trace_enter(). Also includes TIF_NOHZ for enter_from_user_mode()"
> > So I don't understand why _TIF_NOHZ needs to be dropped.
>
> I have already forgot this discussion... But after I glanced at this code again
> I still think the same, and I don't understand why do you disagree.
>

Sorry, but I didn't have any disagreement, I just said I don't understand
the usage on all architectures at that moment.

> > Also if we need to drop, we can address that separately examining all archs.
>
> Sure, and I was only talking about x86. We can keep TIF_NOHZ and even
> set_tsk_thread_flag(TIF_NOHZ) in context_tracking_cpu_set() if some arch needs
> this but remove TIF_NOHZ from TIF_WORK_SYSCALL_ENTRY in arch/x86/include/asm/thread_info.h,
> afaics this shouldn't make any difference.
>

OK, it's just x86, then I understand your point. I was looking at all
the architectures, sorry for the confusion.

> And I see no reason why x86 needs to use TIF_WORK_SYSCALL_ENTRY in
> syscall_trace_enter().
>

Agreed

--
Regards,
Sudeep