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]>
Index: b/arch/arm64/kernel/ptrace.c
===================================================================
--- a/arch/arm64/kernel/ptrace.c
+++ 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
On Thu, Dec 03, 2015 at 02:05:31PM -0600, John Blackwood wrote:
> Hello Will,
Hi John,
Thanks for the patch.
> 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
diff --git a/arch/alpha/kernel/ptrace.c b/arch/alpha/kernel/ptrace.c
index d9ee81769899..05feb398b7bb 100644
--- a/arch/alpha/kernel/ptrace.c
+++ b/arch/alpha/kernel/ptrace.c
@@ -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)
{
diff --git a/arch/arc/kernel/ptrace.c b/arch/arc/kernel/ptrace.c
index 4442204fe238..8b0aa193ab11 100644
--- a/arch/arc/kernel/ptrace.c
+++ b/arch/arc/kernel/ptrace.c
@@ -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)
{
diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index ef9119f7462e..b839e6f10ef5 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -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)
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 1971f491bb90..ff5a743afe10 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -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.
diff --git a/arch/avr32/include/asm/ptrace.h b/arch/avr32/include/asm/ptrace.h
index 630e4f9bf5f0..ae2b3531d9b0 100644
--- a/arch/avr32/include/asm/ptrace.h
+++ b/arch/avr32/include/asm/ptrace.h
@@ -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)
diff --git a/arch/avr32/kernel/ptrace.c b/arch/avr32/kernel/ptrace.c
index 4aedcab7cd4b..4233ff8c1d32 100644
--- a/arch/avr32/kernel/ptrace.c
+++ b/arch/avr32/kernel/ptrace.c
@@ -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);
}
/*
diff --git a/arch/blackfin/include/asm/ptrace.h b/arch/blackfin/include/asm/ptrace.h
index c00491594b46..6fc6f0b61511 100644
--- a/arch/blackfin/include/asm/ptrace.h
+++ b/arch/blackfin/include/asm/ptrace.h
@@ -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,
diff --git a/arch/c6x/kernel/ptrace.c b/arch/c6x/kernel/ptrace.c
index 3c494e84444d..62083738c1bc 100644
--- a/arch/c6x/kernel/ptrace.c
+++ b/arch/c6x/kernel/ptrace.c
@@ -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)
diff --git a/arch/cris/arch-v10/kernel/ptrace.c b/arch/cris/arch-v10/kernel/ptrace.c
index bfddfb99401f..923f3569b38a 100644
--- a/arch/cris/arch-v10/kernel/ptrace.c
+++ b/arch/cris/arch-v10/kernel/ptrace.c
@@ -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,
diff --git a/arch/cris/arch-v32/kernel/ptrace.c b/arch/cris/arch-v32/kernel/ptrace.c
index f085229cf870..2fa464eef608 100644
--- a/arch/cris/arch-v32/kernel/ptrace.c
+++ b/arch/cris/arch-v32/kernel/ptrace.c
@@ -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)
{
diff --git a/arch/cris/include/asm/ptrace.h b/arch/cris/include/asm/ptrace.h
index 9e788d04a4ef..01cfd4e3bf63 100644
--- a/arch/cris/include/asm/ptrace.h
+++ b/arch/cris/include/asm/ptrace.h
@@ -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 */
diff --git a/arch/frv/kernel/ptrace.c b/arch/frv/kernel/ptrace.c
index 3987ff88dab0..bd04ae38e640 100644
--- a/arch/frv/kernel/ptrace.c
+++ b/arch/frv/kernel/ptrace.c
@@ -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)
{
diff --git a/arch/h8300/kernel/ptrace.c b/arch/h8300/kernel/ptrace.c
index 92075544a19a..da23531e8050 100644
--- a/arch/h8300/kernel/ptrace.c
+++ b/arch/h8300/kernel/ptrace.c
@@ -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)
{
diff --git a/arch/hexagon/kernel/ptrace.c b/arch/hexagon/kernel/ptrace.c
index 390a9ad14ca1..39c35c82afe9 100644
--- a/arch/hexagon/kernel/ptrace.c
+++ b/arch/hexagon/kernel/ptrace.c
@@ -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)
{
diff --git a/arch/ia64/kernel/ptrace.c b/arch/ia64/kernel/ptrace.c
index 6f54d511cc50..cd68bb684ec1 100644
--- a/arch/ia64/kernel/ptrace.c
+++ b/arch/ia64/kernel/ptrace.c
@@ -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)
diff --git a/arch/m32r/kernel/ptrace.c b/arch/m32r/kernel/ptrace.c
index 51f5e9aa4901..69a006094645 100644
--- a/arch/m32r/kernel/ptrace.c
+++ b/arch/m32r/kernel/ptrace.c
@@ -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)
diff --git a/arch/m68k/kernel/ptrace.c b/arch/m68k/kernel/ptrace.c
index 1bc10e62b9af..04b4cb07372b 100644
--- a/arch/m68k/kernel/ptrace.c
+++ b/arch/m68k/kernel/ptrace.c
@@ -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;
diff --git a/arch/metag/kernel/ptrace.c b/arch/metag/kernel/ptrace.c
index 7563628822bd..ae9db2e0f574 100644
--- a/arch/metag/kernel/ptrace.c
+++ b/arch/metag/kernel/ptrace.c
@@ -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)
{
diff --git a/arch/microblaze/kernel/ptrace.c b/arch/microblaze/kernel/ptrace.c
index 8cfa98cadf3d..b77c5d5efc7b 100644
--- a/arch/microblaze/kernel/ptrace.c
+++ b/arch/microblaze/kernel/ptrace.c
@@ -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 */
-}
diff --git a/arch/mips/include/asm/ptrace.h b/arch/mips/include/asm/ptrace.h
index f6fc6aac5496..12b84503bf6f 100644
--- a/arch/mips/include/asm/ptrace.h
+++ b/arch/mips/include/asm/ptrace.h
@@ -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 */
diff --git a/arch/mips/kernel/ptrace.c b/arch/mips/kernel/ptrace.c
index 4f0ac78d17f1..53891dc334e0 100644
--- a/arch/mips/kernel/ptrace.c
+++ b/arch/mips/kernel/ptrace.c
@@ -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.
diff --git a/arch/mn10300/kernel/ptrace.c b/arch/mn10300/kernel/ptrace.c
index 5bd58514e739..a5adcd1914cb 100644
--- a/arch/mn10300/kernel/ptrace.c
+++ b/arch/mn10300/kernel/ptrace.c
@@ -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
*/
diff --git a/arch/nios2/kernel/ptrace.c b/arch/nios2/kernel/ptrace.c
index 681dda92eff1..7da1fa5f0b26 100644
--- a/arch/nios2/kernel/ptrace.c
+++ b/arch/nios2/kernel/ptrace.c
@@ -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)
{
diff --git a/arch/openrisc/kernel/ptrace.c b/arch/openrisc/kernel/ptrace.c
index 4f59fa4e34e5..8ddb36b46ab0 100644
--- a/arch/openrisc/kernel/ptrace.c
+++ b/arch/openrisc/kernel/ptrace.c
@@ -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)
{
diff --git a/arch/parisc/kernel/ptrace.c b/arch/parisc/kernel/ptrace.c
index 9585c81f755f..0cbdcecbca07 100644
--- a/arch/parisc/kernel/ptrace.c
+++ b/arch/parisc/kernel/ptrace.c
@@ -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;
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 737c0d0b53ac..9080469ee684 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -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)
diff --git a/arch/s390/include/asm/ptrace.h b/arch/s390/include/asm/ptrace.h
index 37cbc50947f2..60f10335d820 100644
--- a/arch/s390/include/asm/ptrace.h
+++ b/arch/s390/include/asm/ptrace.h
@@ -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])
diff --git a/arch/s390/kernel/ptrace.c b/arch/s390/kernel/ptrace.c
index 01c37b36caf9..a7ac6e44531f 100644
--- a/arch/s390/kernel/ptrace.c
+++ b/arch/s390/kernel/ptrace.c
@@ -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;
}
diff --git a/arch/score/kernel/ptrace.c b/arch/score/kernel/ptrace.c
index 55836188b217..3ef83188450d 100644
--- a/arch/score/kernel/ptrace.c
+++ b/arch/score/kernel/ptrace.c
@@ -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)
diff --git a/arch/sh/kernel/ptrace_32.c b/arch/sh/kernel/ptrace_32.c
index c1a6b89bfe70..d036e820f677 100644
--- a/arch/sh/kernel/ptrace_32.c
+++ b/arch/sh/kernel/ptrace_32.c
@@ -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,
diff --git a/arch/sh/kernel/ptrace_64.c b/arch/sh/kernel/ptrace_64.c
index 5cea973a65b2..1e6be45b6f24 100644
--- a/arch/sh/kernel/ptrace_64.c
+++ b/arch/sh/kernel/ptrace_64.c
@@ -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);
-}
diff --git a/arch/sparc/kernel/ptrace_32.c b/arch/sparc/kernel/ptrace_32.c
index a331fdc11a2c..52f5cea94e01 100644
--- a/arch/sparc/kernel/ptrace_32.c
+++ b/arch/sparc/kernel/ptrace_32.c
@@ -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,
diff --git a/arch/sparc/kernel/ptrace_64.c b/arch/sparc/kernel/ptrace_64.c
index 9ddc4928a089..022127c824d8 100644
--- a/arch/sparc/kernel/ptrace_64.c
+++ b/arch/sparc/kernel/ptrace_64.c
@@ -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
diff --git a/arch/tile/kernel/ptrace.c b/arch/tile/kernel/ptrace.c
index bdc126faf741..6d5fbf1cc5bb 100644
--- a/arch/tile/kernel/ptrace.c
+++ b/arch/tile/kernel/ptrace.c
@@ -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
diff --git a/arch/um/kernel/ptrace.c b/arch/um/kernel/ptrace.c
index 6a826cbb15c4..2e7e0597de8a 100644
--- a/arch/um/kernel/ptrace.c
+++ b/arch/um/kernel/ptrace.c
@@ -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);
diff --git a/arch/unicore32/kernel/ptrace.c b/arch/unicore32/kernel/ptrace.c
index 9f07c08da050..4e46339db6f0 100644
--- a/arch/unicore32/kernel/ptrace.c
+++ b/arch/unicore32/kernel/ptrace.c
@@ -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,
diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 558f50edebca..8277c5e37279 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -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
diff --git a/arch/xtensa/kernel/ptrace.c b/arch/xtensa/kernel/ptrace.c
index 4d54b481123b..2496b5325452 100644
--- a/arch/xtensa/kernel/ptrace.c
+++ b/arch/xtensa/kernel/ptrace.c
@@ -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);
diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index 061265f92876..e04a1367f9ff 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -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);
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index b760bae64cf1..1b61ecea4857 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -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);
> Subject: Re: [PATCH] ARM64: Clear out any singlestep state on a ptrace detach operation
> From: Will Deacon <[email protected]>
> Date: 12/04/2015 04:03 AM
> To: "Blackwood, John" <[email protected]>
> CC: "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "[email protected]" <[email protected]>
>
> On Thu, Dec 03, 2015 at 02:05:31PM -0600, John Blackwood wrote:
> > Hello Will,
>
> Hi John,
>
> Thanks for the patch.
>
> > 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?
Hi Will,
Thanks for looking into this issue.
I can see your point about possibly having the common code handle
the singlestep cleanup on detach. However, I'm a newbie to arm64,
and also not knowledgeable at all in any of the other architectures,
so I wouldn't be able to comment on whether you patch is a good fit for
all other architectures.
I took a look at the hardware breakpoints on x86, and indeed, it is up to
the debugger to clear out any current hardware breakpoints (and software
breakpoints for that matter) before doing a PTRACE_DETACH. Otherwise,
the previously ptraced task(s) may hit leftover breakpoints and SIGTRAP
and die.
I wrote an x86 test that sets a hw breakpoint and then detaches
and the ptraced task will/can hit the breakpoint and die with a SIGTRAP.
The only clearing of hw breakpoints is when a task execs or when a
task forks and the new task will not inherit the task->ptrace_bps[]
values. The kernel's perf event support is used to context switch in
and out a task's hw breakpoint state and unregistering this perf event
is only done in flush_thread() during execs.
However, unlike the situation with detaching after an arm64 singlestep
operation occurred, the debugger does have the opportunity to remove
the hw/sw breakpoints before detaching if it wants the task to be able
to continue on successfully.
Just in case you are interested, I have below a simple test that shows
the arm64 singlestep/detach sequence issue. When the singlestep state
is not cleaned up, then the 'PASSED' echo will not show up.
Thanks again for your time and considerations.
-----
/*
* Singlestep detach test
* If sucessful, 'PASSED' should be seen when executing this test.
* Checks for a kernel bug in arm64 where detaching after a singlestep
* would cause the previously ptraced task to send itself a SIGTRAP
* signal and die.
*/
#include <unistd.h>
#include <stdio.h>
#include <errno.h>
#include <sys/ptrace.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <string.h>
static int inferior_wait (int inferior)
{
int wait_status;
int status = waitpid(inferior, &wait_status, 0);
if (status == -1) {
perror("waitpid");
return 1;
}
printf("pid: %d ", status);
if (WIFEXITED(wait_status)) {
printf("exited with status %d", WEXITSTATUS(wait_status));
} else if (WIFSIGNALED(wait_status)) {
printf("terminated with signal %d %s",
WTERMSIG(wait_status), strsignal(WTERMSIG(wait_status)));
} else if (WIFSTOPPED(wait_status)) {
printf("stopped with signal %d %s",
WSTOPSIG(wait_status), strsignal(WSTOPSIG(wait_status)));
} else {
printf("don't understand wait status");
}
printf("\n");
return 0;
}
#define unused __attribute__((unused))
int main(int unused argc, unused char **argv)
{
int status, inferior;
inferior = fork();
if (inferior == -1) {
perror("fork");
return 1;
}
if (inferior == 0) {
status = ptrace(PTRACE_TRACEME, 0, 0, 0);
if (status == -1) {
perror("inferior ptrace(PTRACE_TRACEME,...)");
return 1;
}
execl("/bin/echo", "/bin/echo", "PASSED", NULL);
perror("inferior execl");
return 1;
}
/* Parent (debugger) from here on
*/
status = inferior_wait(inferior);
if (status != 0)
return status;
status = ptrace(PTRACE_SINGLESTEP, inferior, 0, 0);
if (status == -1) {
perror("ptrace(PTRACE_SINGLE_STEP,...)");
return 1;
}
status = inferior_wait(inferior);
if (status != 0)
return status;
status = ptrace(PTRACE_DETACH, inferior, 0, 0);
if (status == -1) {
perror("ptrace(PTRACE_DETACH,...)");
return 1;
}
return 0;
}
On 12/04, Will Deacon wrote:
>
> 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.
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -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);
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.
On Sat, Dec 05, 2015 at 07:48:40PM +0100, Oleg Nesterov wrote:
> On 12/04, Will Deacon wrote:
> >
> > 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.
Oh, joy!
> > --- a/kernel/ptrace.c
> > +++ b/kernel/ptrace.c
> > @@ -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);
>
> 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