ARM64: Clear out any singlestep state on a ptrace detach operation

PATCH In review John Blackwood 3 participants 4 messages Dec 3, 2015 – Dec 7, 2015

Subsystems (19)

Arm (32-bit) x86 PowerPC MIPS +15 more subsystems

lore

Share & tools

lore

John Blackwood

author

2015-12-03 20:11 UTC #0 lore raw

PATCH 1/1 ARM64: Clear out any singlestep state on a ptrace detach operation

Hello Will,

I have a patch for a ptrace(2) issue that we encountered on arm64 kernels. If a debugger singlesteps a ptraced task, and then does a ptrace(2) PTRACE_DETACH command, the task will not resume successfully. It seems that clearing out the singlestep state, as something like a ptrace(2) PTRACE_CONT does, gets this working.

Thank you for your time and considerations.

----- -----

arm64: Clear out any singlestep state on a ptrace detach operation.

Make sure to clear out any ptrace singlestep state when a ptrace(2) PTRACE_DETACH call is made on arm64 systems.

Otherwise, the previously ptraced task will die off with a SIGTRAP signal if the debugger just previously singlestepped the ptraced task.

Signed-off-by John Blackwood <[email protected]>

arch/arm64/kernel/ptrace.c +1 -0

Index: b/arch/arm64/kernel/ptrace.c
===================================================================
@@ -58,6 +58,7 @@
  */
 void ptrace_disable(struct task_struct *child)
 {
+	user_disable_single_step(child);
 }
 
 #ifdef CONFIG_HAVE_HW_BREAKPOINT

Will Deacon

2015-12-04 10:04 UTC #1 lore raw

Quoted 1 line · John Blackwood

Hello Will,

Hi John,

Thanks for the patch.

Quoted 7 lines

I have a patch for a ptrace(2) issue that we encountered on arm64 kernels. If a debugger singlesteps a ptraced task, and then does a ptrace(2) PTRACE_DETACH command, the task will not resume successfully. It seems that clearing out the singlestep state, as something like a ptrace(2) PTRACE_CONT does, gets this working.
Thank you for your time and considerations.

I think you're correct that we should be doing this, but actually, why isn't this done by the core code? It looks to me like this should be part of ptrace_detach, just like it is part of ptrace_resume when a PTRACE_CONT request is handled. That would also be a step towards making ptrace_disable optional (since x86 and powerpc are doing stuff that could be done in core code too).

I hacked up a quick patch below (not even compile-tested), but I'm not sure what to do about hardware {break,watch}points. Some architectures explicitly clear those on detach, whereas others appear to leave them alone. Thoughts?

Will

--->8

arch/alpha/kernel/ptrace.c +0 ~~-10~~

index d9ee81769899..05feb398b7bb 100644
@@ -260,16 +260,6 @@ void user_disable_single_step(struct task_struct *child)
 	ptrace_cancel_bpt(child);
 }
 
-/*
- * Called by kernel/ptrace.c when detaching..
- *
- * Make sure the single step bit is not set.
- */
-void ptrace_disable(struct task_struct *child)
-{ 
-	user_disable_single_step(child);
-}
-
 long arch_ptrace(struct task_struct *child, long request,
 		 unsigned long addr, unsigned long data)
 {

arch/arc/kernel/ptrace.c +0 -4

index 4442204fe238..8b0aa193ab11 100644
@@ -210,10 +210,6 @@ const struct user_regset_view *task_user_regset_view(struct task_struct *task)
 	return &user_arc_view;
 }
 
-void ptrace_disable(struct task_struct *child)
-{
-}
-
 long arch_ptrace(struct task_struct *child, long request,
 		 unsigned long addr, unsigned long data)
 {

arch/arm/kernel/ptrace.c +0 -8

index ef9119f7462e..b839e6f10ef5 100644
@@ -190,14 +190,6 @@ put_user_reg(struct task_struct *task, int offset, long data)
 }
 
 /*
- * Called by kernel/ptrace.c when detaching..
- */
-void ptrace_disable(struct task_struct *child)
-{
-	/* Nothing to do. */
-}
-
-/*
  * Handle hitting a breakpoint.
  */
 void ptrace_break(struct task_struct *tsk, struct pt_regs *regs)

arch/arm64/kernel/ptrace.c +0 -7

index 1971f491bb90..ff5a743afe10 100644
@@ -53,13 +53,6 @@
  * in exit.c or in signal.c.
  */
 
-/*
- * Called by kernel/ptrace.c when detaching..
- */
-void ptrace_disable(struct task_struct *child)
-{
-}
-
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
 /*
  * Handle hitting a HW-breakpoint.

arch/avr32/include/asm/ptrace.h +1 -0

index 630e4f9bf5f0..ae2b3531d9b0 100644
@@ -17,6 +17,7 @@
 #define arch_has_single_step()		(1)
 
 #define arch_ptrace_attach(child)       ocd_enable(child)
+#define arch_ptrace_detach(child)	ocd_disable(child)
 
 #define user_mode(regs)                 (((regs)->sr & MODE_MASK) == MODE_USER)
 #define instruction_pointer(regs)       ((regs)->pc)

arch/avr32/kernel/ptrace.c +0 ~~-11~~

index 4aedcab7cd4b..4233ff8c1d32 100644
@@ -51,19 +51,8 @@ void user_enable_single_step(struct task_struct *tsk)
 
 void user_disable_single_step(struct task_struct *child)
 {
-	/* XXX(hch): a no-op here seems wrong.. */
-}
-
-/*
- * Called by kernel/ptrace.c when detaching
- *
- * Make sure any single step bits, etc. are not set
- */
-void ptrace_disable(struct task_struct *child)
-{
 	clear_tsk_thread_flag(child, TIF_SINGLE_STEP);
 	clear_tsk_thread_flag(child, TIF_BREAKPOINT);
-	ocd_disable(child);
 }
 
 /*

arch/blackfin/include/asm/ptrace.h +0 -2

index c00491594b46..6fc6f0b61511 100644
@@ -15,8 +15,6 @@
 #define user_mode(regs) (!(((regs)->ipend & ~0x10) & (((regs)->ipend & ~0x10) - 1)))
 
 #define arch_has_single_step()	(1)
-/* common code demands this function */
-#define ptrace_disable(child) user_disable_single_step(child)
 #define current_user_stack_pointer() rdusp()
 
 extern int is_user_addr_valid(struct task_struct *child,

arch/c6x/kernel/ptrace.c +0 -8

index 3c494e84444d..62083738c1bc 100644
@@ -20,14 +20,6 @@
 #define PT_REG_SIZE	  (sizeof(struct pt_regs))
 
 /*
- * Called by kernel/ptrace.c when detaching.
- */
-void ptrace_disable(struct task_struct *child)
-{
-	/* nothing to do */
-}
-
-/*
  * Get a register number from live pt_regs for the specified task.
  */
 static inline long get_reg(struct task_struct *task, int regno)

arch/cris/arch-v10/kernel/ptrace.c +0 ~~-12~~

index bfddfb99401f..923f3569b38a 100644
@@ -55,18 +55,6 @@ inline int put_reg(struct task_struct *task, unsigned int regno,
 	return 0;
 }
 
-/*
- * Called by kernel/ptrace.c when detaching.
- *
- * Make sure the single step bit is not set.
- */
-void 
-ptrace_disable(struct task_struct *child)
-{
-       /* Todo - pending singlesteps? */
-       clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
-}
-
 /* 
  * Note that this implementation of ptrace behaves differently from vanilla
  * ptrace.  Contrary to what the man page says, in the PTRACE_PEEKTEXT,

arch/cris/arch-v32/kernel/ptrace.c +0 ~~-18~~

index f085229cf870..2fa464eef608 100644
@@ -29,7 +29,6 @@
 static int put_debugreg(long pid, unsigned int regno, long data);
 static long get_debugreg(long pid, unsigned int regno);
 static unsigned long get_pseudo_pc(struct task_struct *child);
-void deconfigure_bp(long pid);
 
 extern unsigned long cris_signal_return_page;

@@ -106,23 +105,6 @@ void user_disable_single_step(struct task_struct *child)
 	}
 }
 
-/*
- * Called by kernel/ptrace.c when detaching.
- *
- * Make sure the single step bit is not set.
- */
-void
-ptrace_disable(struct task_struct *child)
-{
-	/* Deconfigure SPC and S-bit. */
-	user_disable_single_step(child);
-	put_reg(child, PT_SPC, 0);
-
-	/* Deconfigure any watchpoints associated with the child. */
-	deconfigure_bp(child->pid);
-}
-
-
 long arch_ptrace(struct task_struct *child, long request,
 		 unsigned long addr, unsigned long data)
 {

arch/cris/include/asm/ptrace.h +5 -0

index 9e788d04a4ef..01cfd4e3bf63 100644
@@ -11,4 +11,9 @@
 #define profile_pc(regs) instruction_pointer(regs)
 #define current_user_stack_pointer() rdusp()
 
+#ifdef CONFIG_ETRAX_ARCH_V32
+void deconfigure_bp(long pid);
+#define arch_ptrace_detach(child)	deconfigure_bp(child->pid);
+#endif /* CONFIG_ETRAX_ARCH_V32 */
+
 #endif /* _CRIS_PTRACE_H */

arch/frv/kernel/ptrace.c +0 -5

index 3987ff88dab0..bd04ae38e640 100644
@@ -248,11 +248,6 @@ void user_disable_single_step(struct task_struct *child)
 	child->thread.frame0->__status &= ~REG__STATUS_STEP;
 }
 
-void ptrace_disable(struct task_struct *child)
-{
-	user_disable_single_step(child);
-}
-
 long arch_ptrace(struct task_struct *child, long request,
 		 unsigned long addr, unsigned long data)
 {

arch/h8300/kernel/ptrace.c +0 -5

index 92075544a19a..da23531e8050 100644
@@ -154,11 +154,6 @@ const struct user_regset_view *task_user_regset_view(struct task_struct *task)
 	return &user_h8300_native_view;
 }
 
-void ptrace_disable(struct task_struct *child)
-{
-	user_disable_single_step(child);
-}
-
 long arch_ptrace(struct task_struct *child, long request,
 		 unsigned long addr, unsigned long data)
 {

arch/hexagon/kernel/ptrace.c +0 -6

index 390a9ad14ca1..39c35c82afe9 100644
@@ -192,12 +192,6 @@ const struct user_regset_view *task_user_regset_view(struct task_struct *task)
 	return &hexagon_user_view;
 }
 
-void ptrace_disable(struct task_struct *child)
-{
-	/* Boilerplate - resolves to null inline if no HW single-step */
-	user_disable_single_step(child);
-}
-
 long arch_ptrace(struct task_struct *child, long request,
 		 unsigned long addr, unsigned long data)
 {

arch/ia64/kernel/ptrace.c +0 ~~-11~~

index 6f54d511cc50..cd68bb684ec1 100644
@@ -1137,17 +1137,6 @@ user_disable_single_step (struct task_struct *child)
 	child_psr->tb = 0;
 }
 
-/*
- * Called by kernel/ptrace.c when detaching..
- *
- * Make sure the single step bit is not set.
- */
-void
-ptrace_disable (struct task_struct *child)
-{
-	user_disable_single_step(child);
-}
-
 long
 arch_ptrace (struct task_struct *child, long request,
 	     unsigned long addr, unsigned long data)

arch/m32r/kernel/ptrace.c +0 ~~-10~~

index 51f5e9aa4901..69a006094645 100644
@@ -609,16 +609,6 @@ void user_disable_single_step(struct task_struct *child)
 	invalidate_cache();
 }
 
-/*
- * Called by kernel/ptrace.c when detaching..
- *
- * Make sure single step bits etc are not set.
- */
-void ptrace_disable(struct task_struct *child)
-{
-	/* nothing to do.. */
-}
-
 long
 arch_ptrace(struct task_struct *child, long request,
 	    unsigned long addr, unsigned long data)

arch/m68k/kernel/ptrace.c +0 -8

index 1bc10e62b9af..04b4cb07372b 100644
@@ -130,14 +130,6 @@ static inline void singlestep_disable(struct task_struct *child)
 	clear_tsk_thread_flag(child, TIF_DELAYED_TRACE);
 }
 
-/*
- * Called by kernel/ptrace.c when detaching..
- */
-void ptrace_disable(struct task_struct *child)
-{
-	singlestep_disable(child);
-}
-
 void user_enable_single_step(struct task_struct *child)
 {
 	unsigned long tmp = get_reg(child, PT_SR) & ~TRACE_BITS;

arch/metag/kernel/ptrace.c +0 ~~-10~~

index 7563628822bd..ae9db2e0f574 100644
@@ -367,16 +367,6 @@ const struct user_regset_view *task_user_regset_view(struct task_struct *task)
 	return &user_metag_view;
 }
 
-/*
- * Called by kernel/ptrace.c when detaching..
- *
- * Make sure single step bits etc are not set.
- */
-void ptrace_disable(struct task_struct *child)
-{
-	/* nothing to do.. */
-}
-
 long arch_ptrace(struct task_struct *child, long request, unsigned long addr,
 		 unsigned long data)
 {

arch/microblaze/kernel/ptrace.c +0 -5

index 8cfa98cadf3d..b77c5d5efc7b 100644
@@ -162,8 +162,3 @@ asmlinkage void do_syscall_trace_leave(struct pt_regs *regs)
 	if (step || test_thread_flag(TIF_SYSCALL_TRACE))
 		tracehook_report_syscall_exit(regs, step);
 }
-
-void ptrace_disable(struct task_struct *child)
-{
-	/* nothing to do */
-}

arch/mips/include/asm/ptrace.h +2 -0

index f6fc6aac5496..12b84503bf6f 100644
@@ -191,4 +191,6 @@ static inline void user_stack_pointer_set(struct pt_regs *regs,
 	regs->regs[29] = val;
 }
 
+#define arch_ptrace_detach(child) clear_tsk_thread_flag(child, TIF_LOAD_WATCH)
+
 #endif /* _ASM_PTRACE_H */

arch/mips/kernel/ptrace.c +0 ~~-11~~

index 4f0ac78d17f1..53891dc334e0 100644
@@ -69,17 +69,6 @@ static void init_fp_ctx(struct task_struct *target)
 }
 
 /*
- * Called by kernel/ptrace.c when detaching..
- *
- * Make sure single step bits etc are not set.
- */
-void ptrace_disable(struct task_struct *child)
-{
-	/* Don't load the watchpoint registers for the ex-child. */
-	clear_tsk_thread_flag(child, TIF_LOAD_WATCH);
-}
-
-/*
  * Read a general register set.	 We always use the 64-bit format, even
  * for 32-bit kernels and for 32-bit processes on a 64-bit kernel.
  * Registers are sign extended to fill the available space.

arch/mn10300/kernel/ptrace.c +0 -5

index 5bd58514e739..a5adcd1914cb 100644
@@ -286,11 +286,6 @@ void user_disable_single_step(struct task_struct *child)
 #endif
 }
 
-void ptrace_disable(struct task_struct *child)
-{
-	user_disable_single_step(child);
-}
-
 /*
  * handle the arch-specific side of process tracing
  */

arch/nios2/kernel/ptrace.c +0 -5

index 681dda92eff1..7da1fa5f0b26 100644
@@ -138,11 +138,6 @@ const struct user_regset_view *task_user_regset_view(struct task_struct *task)
 	return &nios2_user_view;
 }
 
-void ptrace_disable(struct task_struct *child)
-{
-
-}
-
 long arch_ptrace(struct task_struct *child, long request, unsigned long addr,
 		 unsigned long data)
 {

arch/openrisc/kernel/ptrace.c +0 ~~-14~~

index 4f59fa4e34e5..8ddb36b46ab0 100644
@@ -142,20 +142,6 @@ const struct user_regset_view *task_user_regset_view(struct task_struct *task)
  * in exit.c or in signal.c.
  */
 
-
-/*
- * Called by kernel/ptrace.c when detaching..
- *
- * Make sure the single step bit is not set.
- */
-void ptrace_disable(struct task_struct *child)
-{
-	pr_debug("ptrace_disable(): TODO\n");
-
-	user_disable_single_step(child);
-	clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
-}
-
 long arch_ptrace(struct task_struct *child, long request, unsigned long addr,
 		 unsigned long data)
 {

arch/parisc/kernel/ptrace.c +4 ~~-14~~

index 9585c81f755f..0cbdcecbca07 100644
@@ -31,11 +31,10 @@
 #define USER_PSW_BITS	(PSW_N | PSW_B | PSW_V | PSW_CB)
 
 /*
- * Called by kernel/ptrace.c when detaching..
- *
- * Make sure single step bits etc are not set.
+ * The following functions are called by ptrace_resume() when
+ * enabling or disabling single/block tracing.
  */
-void ptrace_disable(struct task_struct *task)
+void user_disable_single_step(struct task_struct *task)
 {
 	clear_tsk_thread_flag(task, TIF_SINGLESTEP);
 	clear_tsk_thread_flag(task, TIF_BLOCKSTEP);

@@ -47,15 +46,6 @@ void ptrace_disable(struct task_struct *task)
 	pa_psw(task)->l = 0;
 }
 
-/*
- * The following functions are called by ptrace_resume() when
- * enabling or disabling single/block tracing.
- */
-void user_disable_single_step(struct task_struct *task)
-{
-	ptrace_disable(task);
-}
-
 void user_enable_single_step(struct task_struct *task)
 {
 	clear_tsk_thread_flag(task, TIF_BLOCKSTEP);

@@ -73,7 +63,7 @@ void user_enable_single_step(struct task_struct *task)
 		pa_psw(task)->y = 0;
 		pa_psw(task)->z = 0;
 		pa_psw(task)->b = 0;
-		ptrace_disable(task);
+		user_disable_single_step(task);
 		/* Don't wake up the task, but let the
 		   parent know something happened. */
 		si.si_code = TRAP_TRACE;

arch/powerpc/kernel/ptrace.c +0 ~~-11~~

index 737c0d0b53ac..9080469ee684 100644
@@ -1056,17 +1056,6 @@ static int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
 	return 0;
 }
 
-/*
- * Called by kernel/ptrace.c when detaching..
- *
- * Make sure single step bits etc are not set.
- */
-void ptrace_disable(struct task_struct *child)
-{
-	/* make sure the single step bit is not set. */
-	user_disable_single_step(child);
-}
-
 #ifdef CONFIG_PPC_ADV_DEBUG_REGS
 static long set_instruction_bp(struct task_struct *child,
 			      struct ppc_hw_breakpoint *bp_info)

arch/s390/include/asm/ptrace.h +3 -0

index 37cbc50947f2..60f10335d820 100644
@@ -148,6 +148,9 @@ static inline int test_pt_regs_flag(struct pt_regs *regs, int flag)
 #define arch_has_single_step()	(1)
 #define arch_has_block_step()	(1)
 
+void arch_ptrace_detach(struct task_struct *task);
+#define arch_ptrace_detach	arch_ptrace_detach
+
 #define user_mode(regs) (((regs)->psw.mask & PSW_MASK_PSTATE) != 0)
 #define instruction_pointer(regs) ((regs)->psw.addr & PSW_ADDR_INSN)
 #define user_stack_pointer(regs)((regs)->gprs[15])

arch/s390/kernel/ptrace.c +1 -2

index 01c37b36caf9..a7ac6e44531f 100644
@@ -121,11 +121,10 @@ void user_enable_block_step(struct task_struct *task)
  *
  * Clear all debugging related fields.
  */
-void ptrace_disable(struct task_struct *task)
+void arch_ptrace_detach(struct task_struct *task)
 {
 	memset(&task->thread.per_user, 0, sizeof(task->thread.per_user));
 	memset(&task->thread.per_event, 0, sizeof(task->thread.per_event));
-	clear_tsk_thread_flag(task, TIF_SINGLE_STEP);
 	clear_pt_regs_flag(task_pt_regs(task), PIF_PER_TRAP);
 	task->thread.per_flags = 0;
 }

arch/score/kernel/ptrace.c +0 -5

index 55836188b217..3ef83188450d 100644
@@ -319,11 +319,6 @@ void user_disable_single_step(struct task_struct *child)
 	child->thread.ss_nextcnt = 0;
 }
 
-void ptrace_disable(struct task_struct *child)
-{
-	user_disable_single_step(child);
-}
-
 long
 arch_ptrace(struct task_struct *child, long request,
 	    unsigned long addr, unsigned long data)

arch/sh/kernel/ptrace_32.c +0 ~~-10~~

index c1a6b89bfe70..d036e820f677 100644
@@ -125,16 +125,6 @@ void user_disable_single_step(struct task_struct *child)
 	clear_tsk_thread_flag(child, TIF_SINGLESTEP);
 }
 
-/*
- * Called by kernel/ptrace.c when detaching..
- *
- * Make sure single step bits etc are not set.
- */
-void ptrace_disable(struct task_struct *child)
-{
-	user_disable_single_step(child);
-}
-
 static int genregs_get(struct task_struct *target,
 		       const struct user_regset *regset,
 		       unsigned int pos, unsigned int count,

arch/sh/kernel/ptrace_64.c +0 ~~-10~~

index 5cea973a65b2..1e6be45b6f24 100644
@@ -566,13 +566,3 @@ BUILD_TRAP_HANDLER(breakpoint)
 	force_sig(SIGTRAP, current);
 	regs->pc += 4;
 }
-
-/*
- * Called by kernel/ptrace.c when detaching..
- *
- * Make sure single step bits etc are not set.
- */
-void ptrace_disable(struct task_struct *child)
-{
-	user_disable_single_step(child);
-}

arch/sparc/kernel/ptrace_32.c +0 ~~-10~~

index a331fdc11a2c..52f5cea94e01 100644
@@ -30,16 +30,6 @@
 
 /* #define ALLOW_INIT_TRACING */
 
-/*
- * Called by kernel/ptrace.c when detaching..
- *
- * Make sure single step bits etc are not set.
- */
-void ptrace_disable(struct task_struct *child)
-{
-	/* nothing to do */
-}
-
 enum sparc_regset {
 	REGSET_GENERAL,
 	REGSET_FP,

arch/sparc/kernel/ptrace_64.c +0 ~~-10~~

index 9ddc4928a089..022127c824d8 100644
@@ -46,16 +46,6 @@
 
 /* #define ALLOW_INIT_TRACING */
 
-/*
- * Called by kernel/ptrace.c when detaching..
- *
- * Make sure single step bits etc are not set.
- */
-void ptrace_disable(struct task_struct *child)
-{
-	/* nothing to do */
-}
-
 /* To get the necessary page struct, access_process_vm() first calls
  * get_user_pages().  This has done a flush_dcache_page() on the
  * accessed page.  Then our caller (copy_{to,from}_user_page()) did

arch/tile/kernel/ptrace.c +0 ~~-14~~

index bdc126faf741..6d5fbf1cc5bb 100644
@@ -40,20 +40,6 @@ void user_disable_single_step(struct task_struct *child)
 }
 
 /*
- * Called by kernel/ptrace.c when detaching..
- */
-void ptrace_disable(struct task_struct *child)
-{
-	clear_tsk_thread_flag(child, TIF_SINGLESTEP);
-
-	/*
-	 * These two are currently unused, but will be set by arch_ptrace()
-	 * and used in the syscall assembly when we do support them.
-	 */
-	clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
-}
-
-/*
  * Get registers from task and ready the result for userspace.
  * Note that we localize the API issues to getregs() and putregs() at
  * some cost in performance, e.g. we need a full pt_regs copy for

arch/um/kernel/ptrace.c +0 -8

index 6a826cbb15c4..2e7e0597de8a 100644
@@ -30,14 +30,6 @@ void user_disable_single_step(struct task_struct *child)
 #endif
 }
 
-/*
- * Called by kernel/ptrace.c when detaching..
- */
-void ptrace_disable(struct task_struct *child)
-{
-	user_disable_single_step(child);
-}
-
 extern int peek_user(struct task_struct * child, long addr, long data);
 extern int poke_user(struct task_struct * child, long addr, long data);

arch/unicore32/kernel/ptrace.c +0 -7

index 9f07c08da050..4e46339db6f0 100644
@@ -51,13 +51,6 @@ put_user_reg(struct task_struct *task, int offset, long data)
 }
 
 /*
- * Called by kernel/ptrace.c when detaching..
- */
-void ptrace_disable(struct task_struct *child)
-{
-}
-
-/*
  * We actually access the pt_regs stored on the kernel stack.
  */
 static int ptrace_read_user(struct task_struct *tsk, unsigned long off,

arch/x86/kernel/ptrace.c +0 ~~-13~~

index 558f50edebca..8277c5e37279 100644
@@ -793,19 +793,6 @@ static int ioperm_get(struct task_struct *target,
 				   0, IO_BITMAP_BYTES);
 }
 
-/*
- * Called by kernel/ptrace.c when detaching..
- *
- * Make sure the single step bit is not set.
- */
-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
 static const struct user_regset_view user_x86_32_view; /* Initialized below. */
 #endif

arch/xtensa/kernel/ptrace.c +0 -9

index 4d54b481123b..2496b5325452 100644
@@ -40,15 +40,6 @@ void user_disable_single_step(struct task_struct *child)
 	child->ptrace &= ~PT_SINGLESTEP;
 }
 
-/*
- * Called by kernel/ptrace.c when detaching to disable single stepping.
- */
-
-void ptrace_disable(struct task_struct *child)
-{
-	/* Nothing to do.. */
-}
-
 int ptrace_getregs(struct task_struct *child, void __user *uregs)
 {
 	struct pt_regs *regs = task_pt_regs(child);

include/linux/ptrace.h +0 -1

index 061265f92876..e04a1367f9ff 100644
@@ -46,7 +46,6 @@ extern long arch_ptrace(struct task_struct *child, long request,
 			unsigned long addr, unsigned long data);
 extern int ptrace_readdata(struct task_struct *tsk, unsigned long src, char __user *dst, int len);
 extern int ptrace_writedata(struct task_struct *tsk, char __user *src, unsigned long dst, int len);
-extern void ptrace_disable(struct task_struct *);
 extern int ptrace_request(struct task_struct *child, long request,
 			  unsigned long addr, unsigned long data);
 extern void ptrace_notify(int exit_code);

kernel/ptrace.c +9 -2

index b760bae64cf1..1b61ecea4857 100644
@@ -454,13 +454,20 @@ static bool __ptrace_detach(struct task_struct *tracer, struct task_struct *p)
 	return dead;
 }
 
+#ifndef arch_ptrace_detach
+#define arch_ptrace_detach(child)	do { } while (0)
+#endif
+
 static int ptrace_detach(struct task_struct *child, unsigned int data)
 {
 	if (!valid_signal(data))
 		return -EIO;
 
-	/* Architecture-specific hardware disable .. */
-	ptrace_disable(child);
+	arch_ptrace_detach(child);
+	user_disable_single_step(child);
+#ifdef TIF_SYSCALL_EMU
+	clear_tsk_thread_flag(child, TIF_SYSCALL_EMU);
+#endif
 	clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
 
 	write_lock_irq(&tasklist_lock);

Oleg Nesterov

maintainer

2015-12-05 18:48 UTC #2 lore raw

Quoted 5 lines · Will Deacon

I hacked up a quick patch below (not even compile-tested), but I'm not sure what to do about hardware {break,watch}points. Some architectures explicitly clear those on detach, whereas others appear to leave them alone. Thoughts?

Heh ;)

Please see fab840fc2d542fabcab "ptrace: PTRACE_DETACH should do flush_ptrace_hw_breakpoint(child)".

And the next "revert" commit, 35114fcbe0b9b0fa3f6653a2.

Quoted 13 lines

kernel/ptrace.c +4 -0

@@ -454,13 +454,20 @@ static bool __ptrace_detach(struct task_struct *tracer, struct task_struct *p)
 	return dead;
 }
 
+#ifndef arch_ptrace_detach
+#define arch_ptrace_detach(child)	do { } while (0)
+#endif
+
 static int ptrace_detach(struct task_struct *child, unsigned int data)
 {
 	if (!valid_signal(data))

kernel/ptrace.c static bool __ptrace_detach(struct task_struct *tracer, struct task_struct *p) · line 471

Quoted 10 lines

kernel/ptrace.c +5 -2

 		return -EIO;
 
-	/* Architecture-specific hardware disable .. */
-	ptrace_disable(child);
+	arch_ptrace_detach(child);
+	user_disable_single_step(child);
+#ifdef TIF_SYSCALL_EMU
+	clear_tsk_thread_flag(child, TIF_SYSCALL_EMU);
+#endif
 	clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);

Well, personally I'd prefer to keep the arch-dependent ptrace_disable(), this just looks safer to me. Although I agree that its name is bad and arch_ptrace_detach() looks much better.

Oleg.

Will Deacon

2015-12-07 11:47 UTC #3 lore raw

Quoted 7 lines · Oleg Nesterov

Quoted 5 lines · Will Deacon
I hacked up a quick patch below (not even compile-tested), but I'm not sure what to do about hardware {break,watch}points. Some architectures explicitly clear those on detach, whereas others appear to leave them alone. Thoughts?

Quoted 6 lines

Heh ;)
Please see fab840fc2d542fabcab "ptrace: PTRACE_DETACH should do flush_ptrace_hw_breakpoint(child)".
And the next "revert" commit, 35114fcbe0b9b0fa3f6653a2.

Oh, joy!

Quoted 15 lines

kernel/ptrace.c +4 -0

@@ -454,13 +454,20 @@ static bool __ptrace_detach(struct task_struct *tracer, struct task_struct *p)
 	return dead;
 }
 
+#ifndef arch_ptrace_detach
+#define arch_ptrace_detach(child)	do { } while (0)
+#endif
+
 static int ptrace_detach(struct task_struct *child, unsigned int data)
 {
 	if (!valid_signal(data))
 		return -EIO;

Quoted 12 lines

kernel/ptrace.c static bool __ptrace_detach(struct task_struct *tracer, struct task_struct *p) · line 471
Quoted 8 lines
kernel/ptrace.c +5 -2
-	/* Architecture-specific hardware disable .. */
-	ptrace_disable(child);
+	arch_ptrace_detach(child);
+	user_disable_single_step(child);
+#ifdef TIF_SYSCALL_EMU
+	clear_tsk_thread_flag(child, TIF_SYSCALL_EMU);
+#endif
 	clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
Well, personally I'd prefer to keep the arch-dependent ptrace_disable(), this just looks safer to me. Although I agree that its name is bad and arch_ptrace_detach() looks much better.

Fair enough. I don't think my patch changed any behaviour, but I can't test it for all the architectures I touched and this area is horribly fragile wrt userspace.

I'll merge the original patch from John.

Will