LinuxLists.cc - [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h

[permalink] [raw]

Subject: [PATCH 3/14] arm: use generic ptrace_resume code

Use the generic ptrace_resume code for PTRACE_SYSCALL, PTRACE_CONT,
PTRACE_KILL and PTRACE_SINGLESTEP. This implies defining
arch_has_single_step in <asm/ptrace.h> and implementing the
user_enable_single_step and user_disable_single_step functions, which
also causes the breakpoint information to be cleared on fork, which
could be considered a bug fix.

Also the TIF_SYSCALL_TRACE thread flag is now cleared on PTRACE_KILL
which it previously wasn't and the single stepping disable only happens
if the tracee process isn't a zombie yet, which is consistent with all
architectures using the modern ptrace code.

Signed-off-by: Christoph Hellwig <[email protected]>

Index: linux-2.6/arch/arm/kernel/ptrace.c
===================================================================
--- linux-2.6.orig/arch/arm/kernel/ptrace.c 2010-02-02 11:00:49.883004121 +0100
+++ linux-2.6/arch/arm/kernel/ptrace.c 2010-02-02 11:10:20.549264275 +0100
@@ -452,12 +452,23 @@ void ptrace_cancel_bpt(struct task_struc
clear_breakpoint(child, &child->thread.debug.bp[i]);
}

+void user_disable_single_step(struct task_struct *task)
+{
+ task->ptrace &= ~PT_SINGLESTEP;
+ ptrace_cancel_bpt(task);
+}
+
+void user_enable_single_step(struct task_struct *task)
+{
+ task->ptrace |= PT_SINGLESTEP;
+}
+
/*
* Called by kernel/ptrace.c when detaching..
*/
void ptrace_disable(struct task_struct *child)
{
- single_step_disable(child);
+ user_disable_single_step(child);
}

/*
@@ -723,53 +734,6 @@ long arch_ptrace(struct task_struct *chi
ret = ptrace_write_user(child, addr, data);
break;

- /*
- * continue/restart and stop at next (return from) syscall
- */
- case PTRACE_SYSCALL:
- case PTRACE_CONT:
- ret = -EIO;
- if (!valid_signal(data))
- break;
- if (request == PTRACE_SYSCALL)
- set_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
- else
- clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
- child->exit_code = data;
- single_step_disable(child);
- wake_up_process(child);
- ret = 0;
- break;
-
- /*
- * make the child exit. Best I can do is send it a sigkill.
- * perhaps it should be put in the status that it wants to
- * exit.
- */
- case PTRACE_KILL:
- single_step_disable(child);
- if (child->exit_state != EXIT_ZOMBIE) {
- child->exit_code = SIGKILL;
- wake_up_process(child);
- }
- ret = 0;
- break;
-
- /*
- * execute single instruction.
- */
- case PTRACE_SINGLESTEP:
- ret = -EIO;
- if (!valid_signal(data))
- break;
- single_step_enable(child);
- clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
- child->exit_code = data;
- /* give it a chance to run. */
- wake_up_process(child);
- ret = 0;
- break;
-
case PTRACE_GETREGS:
ret = ptrace_getregs(child, (void __user *)data);
break;
Index: linux-2.6/arch/arm/kernel/ptrace.h
===================================================================
--- linux-2.6.orig/arch/arm/kernel/ptrace.h 2010-02-02 11:00:49.892003451 +0100
+++ linux-2.6/arch/arm/kernel/ptrace.h 2010-02-02 11:08:43.476003178 +0100
@@ -14,20 +14,6 @@ extern void ptrace_set_bpt(struct task_s
extern void ptrace_break(struct task_struct *, struct pt_regs *);

/*
- * make sure single-step breakpoint is gone.
- */
-static inline void single_step_disable(struct task_struct *task)
-{
- task->ptrace &= ~PT_SINGLESTEP;
- ptrace_cancel_bpt(task);
-}
-
-static inline void single_step_enable(struct task_struct *task)
-{
- task->ptrace |= PT_SINGLESTEP;
-}
-
-/*
* Send SIGTRAP if we're single-stepping
*/
static inline void single_step_trap(struct task_struct *task)
Index: linux-2.6/arch/arm/include/asm/ptrace.h
===================================================================
--- linux-2.6.orig/arch/arm/include/asm/ptrace.h 2010-02-02 11:08:55.072253922 +0100
+++ linux-2.6/arch/arm/include/asm/ptrace.h 2010-02-02 11:09:10.909066877 +0100
@@ -128,6 +128,8 @@ struct pt_regs {

#ifdef __KERNEL__

+#define arch_has_single_step() (1)
+
#define user_mode(regs) \
(((regs)->ARM_cpsr & 0xf) == 0)

2010-02-02 18:59:15

[permalink] [raw]

Subject: [PATCH 2/14] alpha: use generic ptrace_resume code

2010-02-02 18:59:25

[permalink] [raw]

Subject: [PATCH 4/14] avr32: use generic ptrace_resume code

2010-02-02 18:59:40

[permalink] [raw]

Subject: [PATCH 5/14] blackfin: use generic ptrace_resume code

2010-02-02 18:59:59

[permalink] [raw]

Subject: [PATCH 7/14] m68knommu: use generic ptrace_resume code

Use the generic ptrace_resume code for PTRACE_SYSCALL, PTRACE_CONT,
PTRACE_KILL and PTRACE_SINGLESTEP. m68knommu already defines the
nessecary user_enable_single_step and user_disable_single_step functions
for this.

Also the TIF_SYSCALL_TRACE thread flag is now cleared on PTRACE_KILL
which it previously wasn't which is consistent with all architectures
using the modern ptrace code.

Signed-off-by: Christoph Hellwig <[email protected]>

Index: linux-2.6/arch/m68knommu/kernel/ptrace.c
===================================================================
--- linux-2.6.orig/arch/m68knommu/kernel/ptrace.c 2010-02-02 11:00:49.987254269 +0100
+++ linux-2.6/arch/m68knommu/kernel/ptrace.c 2010-02-02 11:22:17.003259609 +0100
@@ -190,62 +190,6 @@ long arch_ptrace(struct task_struct *chi
}
break;

- case PTRACE_SYSCALL: /* continue and stop at next (return from) syscall */
- case PTRACE_CONT: { /* restart after signal. */
- long tmp;
-
- ret = -EIO;
- if (!valid_signal(data))
- break;
- if (request == PTRACE_SYSCALL)
- set_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
- else
- clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
- child->exit_code = data;
- /* make sure the single step bit is not set. */
- tmp = get_reg(child, PT_SR) & ~(TRACE_BITS << 16);
- put_reg(child, PT_SR, tmp);
- wake_up_process(child);
- ret = 0;
- break;
- }
-
- /*
- * make the child exit. Best I can do is send it a sigkill.
- * perhaps it should be put in the status that it wants to
- * exit.
- */
- case PTRACE_KILL: {
- long tmp;
-
- ret = 0;
- if (child->exit_state == EXIT_ZOMBIE) /* already dead */
- break;
- child->exit_code = SIGKILL;
- /* make sure the single step bit is not set. */
- tmp = get_reg(child, PT_SR) & ~(TRACE_BITS << 16);
- put_reg(child, PT_SR, tmp);
- wake_up_process(child);
- break;
- }
-
- case PTRACE_SINGLESTEP: { /* set the trap flag. */
- long tmp;
-
- ret = -EIO;
- if (!valid_signal(data))
- break;
- clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
- tmp = get_reg(child, PT_SR) | (TRACE_BITS << 16);
- put_reg(child, PT_SR, tmp);
-
- child->exit_code = data;
- /* give it a chance to run. */
- wake_up_process(child);
- ret = 0;
- break;
- }
-
case PTRACE_GETREGS: { /* Get all gp regs from the child. */
int i;
unsigned long tmp;

2010-02-02 19:00:10

[permalink] [raw]

Subject: [PATCH 6/14] h8300: use generic ptrace_resume code

Use the generic ptrace_resume code for PTRACE_SYSCALL, PTRACE_CONT,
PTRACE_KILL and PTRACE_SINGLESTEP. This implies defining
arch_has_single_step in <asm/ptrace.h> and implementing the
user_enable_single_step and user_disable_single_step functions, which
also causes the breakpoint information to be cleared on fork, which
could be considered a bug fix.

Also the TIF_SYSCALL_TRACE thread flag is now cleared on PTRACE_KILL
which it previously wasn't which is consistent with all architectures
using the modern ptrace code.

Signed-off-by: Christoph Hellwig <[email protected]>

Index: linux-2.6/arch/h8300/kernel/ptrace.c
===================================================================
--- linux-2.6.orig/arch/h8300/kernel/ptrace.c 2010-02-02 11:01:22.072254015 +0100
+++ linux-2.6/arch/h8300/kernel/ptrace.c 2010-02-02 11:20:07.256255344 +0100
@@ -34,8 +34,11 @@
/* cpu depend functions */
extern long h8300_get_reg(struct task_struct *task, int regno);
extern int h8300_put_reg(struct task_struct *task, int regno, unsigned long data);
-extern void h8300_disable_trace(struct task_struct *child);
-extern void h8300_enable_trace(struct task_struct *child);
+
+
+void user_disable_single_step(struct task_struct *child)
+{
+}

/*
* does not yet catch signals sent when the child dies.
@@ -44,7 +47,7 @@ extern void h8300_enable_trace(struct ta

void ptrace_disable(struct task_struct *child)
{
- h8300_disable_trace(child);
+ user_disable_single_step(child);
}

long arch_ptrace(struct task_struct *child, long request, long addr, long data)
@@ -107,49 +110,6 @@ long arch_ptrace(struct task_struct *chi
}
ret = -EIO;
break ;
- case PTRACE_SYSCALL: /* continue and stop at next (return from) syscall */
- case PTRACE_CONT: { /* restart after signal. */
- ret = -EIO;
- if (!valid_signal(data))
- break ;
- if (request == PTRACE_SYSCALL)
- set_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
- else
- clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
- child->exit_code = data;
- wake_up_process(child);
- /* make sure the single step bit is not set. */
- h8300_disable_trace(child);
- ret = 0;
- }
-
-/*
- * make the child exit. Best I can do is send it a sigkill.
- * perhaps it should be put in the status that it wants to
- * exit.
- */
- case PTRACE_KILL: {
-
- ret = 0;
- if (child->exit_state == EXIT_ZOMBIE) /* already dead */
- break;
- child->exit_code = SIGKILL;
- h8300_disable_trace(child);
- wake_up_process(child);
- break;
- }
-
- case PTRACE_SINGLESTEP: { /* set the trap flag. */
- ret = -EIO;
- if (!valid_signal(data))
- break;
- clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
- child->exit_code = data;
- h8300_enable_trace(child);
- wake_up_process(child);
- ret = 0;
- break;
- }

case PTRACE_GETREGS: { /* Get all gp regs from the child. */
int i;
Index: linux-2.6/arch/h8300/platform/h8300h/ptrace_h8300h.c
===================================================================
--- linux-2.6.orig/arch/h8300/platform/h8300h/ptrace_h8300h.c 2010-02-02 11:00:49.957253800 +0100
+++ linux-2.6/arch/h8300/platform/h8300h/ptrace_h8300h.c 2010-02-02 11:19:15.327003827 +0100
@@ -60,7 +60,7 @@ int h8300_put_reg(struct task_struct *ta
}

/* disable singlestep */
-void h8300_disable_trace(struct task_struct *child)
+void user_disable_single_step(struct task_struct *child)
{
if((long)child->thread.breakinfo.addr != -1L) {
*child->thread.breakinfo.addr = child->thread.breakinfo.inst;
@@ -264,7 +264,7 @@ static unsigned short *getnextpc(struct

/* Set breakpoint(s) to simulate a single step from the current PC. */

-void h8300_enable_trace(struct task_struct *child)
+void user_enable_single_step(struct task_struct *child)
{
unsigned short *nextpc;
nextpc = getnextpc(child,(unsigned short *)h8300_get_reg(child, PT_PC));
@@ -276,7 +276,7 @@ void h8300_enable_trace(struct task_stru
asmlinkage void trace_trap(unsigned long bp)
{
if ((unsigned long)current->thread.breakinfo.addr == bp) {
- h8300_disable_trace(current);
+ user_disable_single_step(current);
force_sig(SIGTRAP,current);
} else
force_sig(SIGILL,current);
Index: linux-2.6/arch/h8300/platform/h8s/ptrace_h8s.c
===================================================================
--- linux-2.6.orig/arch/h8300/platform/h8s/ptrace_h8s.c 2010-02-02 11:00:49.968253944 +0100
+++ linux-2.6/arch/h8300/platform/h8s/ptrace_h8s.c 2010-02-02 11:19:15.329011624 +0100
@@ -65,13 +65,13 @@ int h8300_put_reg(struct task_struct *ta
}

/* disable singlestep */
-void h8300_disable_trace(struct task_struct *child)
+void user_disable_single_step(struct task_struct *child)
{
*(unsigned short *)(child->thread.esp0 + h8300_register_offset[PT_EXR]) &= ~EXR_TRACE;
}

/* enable singlestep */
-void h8300_enable_trace(struct task_struct *child)
+void user_enable_single_step(struct task_struct *child)
{
*(unsigned short *)(child->thread.esp0 + h8300_register_offset[PT_EXR]) |= EXR_TRACE;
}
Index: linux-2.6/arch/h8300/include/asm/ptrace.h
===================================================================
--- linux-2.6.orig/arch/h8300/include/asm/ptrace.h 2010-02-02 11:19:24.753253118 +0100
+++ linux-2.6/arch/h8300/include/asm/ptrace.h 2010-02-02 11:19:36.822033076 +0100
@@ -55,6 +55,8 @@ struct pt_regs {
/* Find the stack offset for a register, relative to thread.esp0. */
#define PT_REG(reg) ((long)&((struct pt_regs *)0)->reg)

+#define arch_has_single_step() (1)
+
#define user_mode(regs) (!((regs)->ccr & PS_S))
#define instruction_pointer(regs) ((regs)->pc)
#define profile_pc(regs) instruction_pointer(regs)

2010-02-02 19:00:31

[permalink] [raw]

Subject: [PATCH 10/14] um: use generic ptrace_resume code

2010-02-02 19:00:29

[permalink] [raw]

Subject: [PATCH 9/14] mips: use generic ptrace_resume code

Use the generic ptrace_resume code for PTRACE_SYSCALL, PTRACE_CONT and
PTRACE_KILL.

Also the TIF_SYSCALL_TRACE thread flag is now cleared on PTRACE_KILL
which it previously wasn't which is consistent with all architectures
using the modern ptrace code.

Signed-off-by: Christoph Hellwig <[email protected]>

Index: linux-2.6/arch/mips/kernel/ptrace.c
===================================================================
--- linux-2.6.orig/arch/mips/kernel/ptrace.c 2010-02-02 11:00:50.011010089 +0100
+++ linux-2.6/arch/mips/kernel/ptrace.c 2010-02-02 19:18:27.564005697 +0100
@@ -481,36 +481,6 @@ long arch_ptrace(struct task_struct *chi
ret = ptrace_setfpregs(child, (__u32 __user *) data);
break;

- case PTRACE_SYSCALL: /* continue and stop at next (return from) syscall */
- case PTRACE_CONT: { /* restart after signal. */
- ret = -EIO;
- if (!valid_signal(data))
- break;
- if (request == PTRACE_SYSCALL) {
- set_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
- }
- else {
- clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
- }
- child->exit_code = data;
- wake_up_process(child);
- ret = 0;
- break;
- }
-
- /*
- * make the child exit. Best I can do is send it a sigkill.
- * perhaps it should be put in the status that it wants to
- * exit.
- */
- case PTRACE_KILL:
- ret = 0;
- if (child->exit_state == EXIT_ZOMBIE) /* already dead */
- break;
- child->exit_code = SIGKILL;
- wake_up_process(child);
- break;
-
case PTRACE_GET_THREAD_AREA:
ret = put_user(task_thread_info(child)->tp_value,
(unsigned long __user *) data);

2010-02-02 19:00:49

[permalink] [raw]

Subject: [PATCH 11/14] xtensa: use generic ptrace_resume code

2010-02-02 19:00:59

[permalink] [raw]

Subject: [PATCH 12/14] cris arch-v10: use generic ptrace_resume code

Use the generic ptrace_resume code for PTRACE_SYSCALL, PTRACE_CONT and
PTRACE_KILL. This also makes PTRACE_SINGLESTEP return -EIO while it
previously succeeded despite not actually causing any kind of single
stepping.

Also the TIF_SYSCALL_TRACE thread flag is now cleared on PTRACE_KILL
which it previously wasn't which is consistent with all architectures
using the modern ptrace code.

Signed-off-by: Christoph Hellwig <[email protected]>

Index: linux-2.6/arch/cris/arch-v10/kernel/ptrace.c
===================================================================
--- linux-2.6.orig/arch/cris/arch-v10/kernel/ptrace.c 2010-02-02 19:23:58.567254756 +0100
+++ linux-2.6/arch/cris/arch-v10/kernel/ptrace.c 2010-02-02 19:24:33.483004794 +0100
@@ -115,57 +115,6 @@ long arch_ptrace(struct task_struct *chi
ret = 0;
break;

- case PTRACE_SYSCALL:
- case PTRACE_CONT:
- ret = -EIO;
-
- if (!valid_signal(data))
- break;
-
- if (request == PTRACE_SYSCALL) {
- set_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
- }
- else {
- clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
- }
-
- child->exit_code = data;
-
- /* TODO: make sure any pending breakpoint is killed */
- wake_up_process(child);
- ret = 0;
-
- break;
-
- /* Make the child exit by sending it a sigkill. */
- case PTRACE_KILL:
- ret = 0;
-
- if (child->exit_state == EXIT_ZOMBIE)
- break;
-
- child->exit_code = SIGKILL;
-
- /* TODO: make sure any pending breakpoint is killed */
- wake_up_process(child);
- break;
-
- /* Set the trap flag. */
- case PTRACE_SINGLESTEP:
- ret = -EIO;
-
- if (!valid_signal(data))
- break;
-
- clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
-
- /* TODO: set some clever breakpoint mechanism... */
-
- child->exit_code = data;
- wake_up_process(child);
- ret = 0;
- break;
-
/* Get all GP registers from the child. */
case PTRACE_GETREGS: {
int i;

2010-02-02 19:01:10

[permalink] [raw]

Subject: [PATCH, RFC 13/14] cris arch-v32: use generic ptrace_resume code

Use the generic ptrace_resume code for PTRACE_SYSCALL, PTRACE_CONT,
PTRACE_KILL and PTRACE_SINGLESTEP. This implies defining
arch_has_single_step in <asm/ptrace.h> and implementing the
user_enable_single_step and user_disable_single_step functions, which
also causes the breakpoint information to be cleared on fork, which
could be considered a bug fix.

Also the TIF_SYSCALL_TRACE thread flag is now cleared on PTRACE_KILL
which it previously wasn't which is consistent with all architectures
using the modern ptrace code.

The way breakpoints are disabled is entirely inconsistent
currently, I tried to make some sense of it, but I suspect all of the
content of ptrace_disable should be moved into user_disable_single_step,
this defintively needs some revisting as the current patch changes
behaviour in not quite designed ways.

Signed-off-by: Christoph Hellwig <[email protected]>

Index: linux-2.6/arch/cris/arch-v32/kernel/ptrace.c
===================================================================
--- linux-2.6.orig/arch/cris/arch-v32/kernel/ptrace.c 2010-02-02 19:23:12.314003057 +0100
+++ linux-2.6/arch/cris/arch-v32/kernel/ptrace.c 2010-02-02 19:25:27.396253967 +0100
@@ -78,6 +78,35 @@ int put_reg(struct task_struct *task, un
return 0;
}

+void user_enable_single_step(struct task_struct *child)
+{
+ unsigned long tmp;
+
+ /*
+ * Set up SPC if not set already (in which case we have no other
+ * choice but to trust it).
+ */
+ if (!get_reg(child, PT_SPC)) {
+ /* In case we're stopped in a delay slot. */
+ tmp = get_reg(child, PT_ERP) & ~1;
+ put_reg(child, PT_SPC, tmp);
+ }
+ tmp = get_reg(child, PT_CCS) | SBIT_USER;
+ put_reg(child, PT_CCS, tmp);
+}
+
+void user_disable_single_step(struct task_struct *child)
+{
+ put_reg(child, PT_SPC, 0);
+
+ if (!get_debugreg(child->pid, PT_BP_CTRL)) {
+ unsigned long tmp;
+ /* If no h/w bp configured, disable S bit. */
+ tmp = get_reg(child, PT_CCS) & ~SBIT_USER;
+ put_reg(child, PT_CCS, tmp);
+ }
+}
+
/*
* Called by kernel/ptrace.c when detaching.
*
@@ -89,8 +118,7 @@ ptrace_disable(struct task_struct *child
unsigned long tmp;

/* Deconfigure SPC and S-bit. */
- tmp = get_reg(child, PT_CCS) & ~SBIT_USER;
- put_reg(child, PT_CCS, tmp);
+ user_disable_single_step(child);
put_reg(child, PT_SPC, 0);

/* Deconfigure any watchpoints associated with the child. */
@@ -163,83 +191,6 @@ long arch_ptrace(struct task_struct *chi
ret = 0;
break;

- case PTRACE_SYSCALL:
- case PTRACE_CONT:
- ret = -EIO;
-
- if (!valid_signal(data))
- break;
-
- /* Continue means no single-step. */
- put_reg(child, PT_SPC, 0);
-
- if (!get_debugreg(child->pid, PT_BP_CTRL)) {
- unsigned long tmp;
- /* If no h/w bp configured, disable S bit. */
- tmp = get_reg(child, PT_CCS) & ~SBIT_USER;
- put_reg(child, PT_CCS, tmp);
- }
-
- if (request == PTRACE_SYSCALL) {
- set_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
- }
- else {
- clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
- }
-
- child->exit_code = data;
-
- /* TODO: make sure any pending breakpoint is killed */
- wake_up_process(child);
- ret = 0;
-
- break;
-
- /* Make the child exit by sending it a sigkill. */
- case PTRACE_KILL:
- ret = 0;
-
- if (child->exit_state == EXIT_ZOMBIE)
- break;
-
- child->exit_code = SIGKILL;
-
- /* Deconfigure single-step and h/w bp. */
- ptrace_disable(child);
-
- /* TODO: make sure any pending breakpoint is killed */
- wake_up_process(child);
- break;
-
- /* Set the trap flag. */
- case PTRACE_SINGLESTEP: {
- unsigned long tmp;
- ret = -EIO;
-
- /* Set up SPC if not set already (in which case we have
- no other choice but to trust it). */
- if (!get_reg(child, PT_SPC)) {
- /* In case we're stopped in a delay slot. */
- tmp = get_reg(child, PT_ERP) & ~1;
- put_reg(child, PT_SPC, tmp);
- }
- tmp = get_reg(child, PT_CCS) | SBIT_USER;
- put_reg(child, PT_CCS, tmp);
-
- if (!valid_signal(data))
- break;
-
- clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
-
- /* TODO: set some clever breakpoint mechanism... */
-
- child->exit_code = data;
- wake_up_process(child);
- ret = 0;
- break;
-
- }
-
/* Get all GP registers from the child. */
case PTRACE_GETREGS: {
int i;
Index: linux-2.6/arch/cris/include/arch-v32/arch/ptrace.h
===================================================================
--- linux-2.6.orig/arch/cris/include/arch-v32/arch/ptrace.h 2010-02-02 19:25:39.092254418 +0100
+++ linux-2.6/arch/cris/include/arch-v32/arch/ptrace.h 2010-02-02 19:25:46.173254455 +0100
@@ -108,6 +108,7 @@ struct switch_stack {

#ifdef __KERNEL__

+#define arch_has_single_step() (1)
#define user_mode(regs) (((regs)->ccs & (1 << (U_CCS_BITNR + CCS_SHIFT))) != 0)
#define instruction_pointer(regs) ((regs)->erp)
extern void show_regs(struct pt_regs *);

2010-02-02 19:00:25

[permalink] [raw]

Subject: [PATCH 8/14] microblaze: use generic ptrace_resume code

Use the generic ptrace_resume code for PTRACE_SYSCALL, PTRACE_CONT and
PTRACE_KILL. This also makes PTRACE_SINGLESTEP return -EIO while it
previously succeeded despite not actually causing any kind of single
stepping.

Also the TIF_SYSCALL_TRACE thread flag is now cleared on PTRACE_KILL
which it previously wasn't which is consistent with all architectures
using the modern ptrace code.

Signed-off-by: Christoph Hellwig <[email protected]>

Index: linux-2.6/arch/microblaze/kernel/ptrace.c
===================================================================
--- linux-2.6.orig/arch/microblaze/kernel/ptrace.c 2010-02-02 11:23:56.002003861 +0100
+++ linux-2.6/arch/microblaze/kernel/ptrace.c 2010-02-02 19:20:16.997254566 +0100
@@ -110,43 +110,6 @@ long arch_ptrace(struct task_struct *chi
if (rval == 0 && request == PTRACE_PEEKUSR)
rval = put_user(val, (unsigned long *)data);
break;
- /* Continue and stop at next (return from) syscall */
- case PTRACE_SYSCALL:
- pr_debug("PTRACE_SYSCALL\n");
- case PTRACE_SINGLESTEP:
- pr_debug("PTRACE_SINGLESTEP\n");
- /* Restart after a signal. */
- case PTRACE_CONT:
- pr_debug("PTRACE_CONT\n");
- rval = -EIO;
- if (!valid_signal(data))
- break;
-
- if (request == PTRACE_SYSCALL)
- set_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
- else
- clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
-
- child->exit_code = data;
- pr_debug("wakeup_process\n");
- wake_up_process(child);
- rval = 0;
- break;
-
- /*
- * make the child exit. Best I can do is send it a sigkill.
- * perhaps it should be put in the status that it wants to
- * exit.
- */
- case PTRACE_KILL:
- pr_debug("PTRACE_KILL\n");
- rval = 0;
- if (child->exit_state == EXIT_ZOMBIE) /* already dead */
- break;
- child->exit_code = SIGKILL;
- wake_up_process(child);
- break;
-
default:
rval = ptrace_request(child, request, addr, data);
}

2010-02-02 19:01:25

[permalink] [raw]

Subject: [PATCH, RFC 14/14] m32r: use generic ptrace_resume code

Use the generic ptrace_resume code for PTRACE_SYSCALL, PTRACE_CONT,
PTRACE_KILL and PTRACE_SINGLESTEP. This implies defining
arch_has_single_step in <asm/ptrace.h> and implementing the
user_enable_single_step and user_disable_single_step functions, which
also causes the breakpoint information to be cleared on fork, which
could be considered a bug fix.

Also the TIF_SYSCALL_TRACE thread flag is now cleared on PTRACE_KILL
which it previously wasn't, which is consistent with all architectures
using the modern ptrace code.

The old code only disables the breakpoints on PTRACE_KILL, while after
this patch this also happens for PTRACE_CONT and PTRACE_SYSCALL which
matches the behaviour of the other architetures. I think this is a
bugfixes, but please double verify this is correct.

Signed-off-by: Christoph Hellwig <[email protected]>

Index: linux-2.6/arch/m32r/include/asm/ptrace.h
===================================================================
--- linux-2.6.orig/arch/m32r/include/asm/ptrace.h 2010-02-02 19:26:37.456003660 +0100
+++ linux-2.6/arch/m32r/include/asm/ptrace.h 2010-02-02 19:26:49.405264333 +0100
@@ -120,6 +120,8 @@ struct pt_regs {

#include <asm/m32r.h> /* M32R_PSW_BSM, M32R_PSW_BPM */

+#define arch_has_single_step() (1)
+
struct task_struct;
extern void init_debug_traps(struct task_struct *);
#define arch_ptrace_attach(child) \
Index: linux-2.6/arch/m32r/kernel/ptrace.c
===================================================================
--- linux-2.6.orig/arch/m32r/kernel/ptrace.c 2010-02-02 19:26:54.353253780 +0100
+++ linux-2.6/arch/m32r/kernel/ptrace.c 2010-02-02 19:29:39.935253922 +0100
@@ -580,6 +580,35 @@ init_debug_traps(struct task_struct *chi
}
}

+void user_enable_single_step(struct task_struct *child)
+{
+ unsigned long next_pc;
+ unsigned long pc, insn;
+
+ clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
+
+ /* Compute next pc. */
+ pc = get_stack_long(child, PT_BPC);
+
+ if (access_process_vm(child, pc&~3, &insn, sizeof(insn), 0)
+ != sizeof(insn))
+ break;
+
+ compute_next_pc(insn, pc, &next_pc, child);
+ if (next_pc & 0x80000000)
+ break;
+
+ if (embed_debug_trap(child, next_pc))
+ break;
+
+ invalidate_cache();
+}
+
+void user_disable_single_step(struct task_struct *child)
+{
+ unregister_all_debug_traps(child);
+ invalidate_cache();
+}

/*
* Called by kernel/ptrace.c when detaching..
@@ -612,74 +641,6 @@ arch_ptrace(struct task_struct *child, l
ret = ptrace_write_user(child, addr, data);
break;

- /*
- * continue/restart and stop at next (return from) syscall
- */
- case PTRACE_SYSCALL:
- case PTRACE_CONT:
- ret = -EIO;
- if (!valid_signal(data))
- break;
- if (request == PTRACE_SYSCALL)
- set_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
- else
- clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
- child->exit_code = data;
- wake_up_process(child);
- ret = 0;
- break;
-
- /*
- * make the child exit. Best I can do is send it a sigkill.
- * perhaps it should be put in the status that it wants to
- * exit.
- */
- case PTRACE_KILL: {
- ret = 0;
- unregister_all_debug_traps(child);
- invalidate_cache();
- if (child->exit_state == EXIT_ZOMBIE) /* already dead */
- break;
- child->exit_code = SIGKILL;
- wake_up_process(child);
- break;
- }
-
- /*
- * execute single instruction.
- */
- case PTRACE_SINGLESTEP: {
- unsigned long next_pc;
- unsigned long pc, insn;
-
- ret = -EIO;
- if (!valid_signal(data))
- break;
- clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
-
- /* Compute next pc. */
- pc = get_stack_long(child, PT_BPC);
-
- if (access_process_vm(child, pc&~3, &insn, sizeof(insn), 0)
- != sizeof(insn))
- break;
-
- compute_next_pc(insn, pc, &next_pc, child);
- if (next_pc & 0x80000000)
- break;
-
- if (embed_debug_trap(child, next_pc))
- break;
-
- invalidate_cache();
- child->exit_code = data;
-
- /* give it a chance to run. */
- wake_up_process(child);
- ret = 0;
- break;
- }
-
case PTRACE_GETREGS:
ret = ptrace_getregs(child, (void __user *)data);
break;

2010-02-02 19:19:12

by Ralf Baechle

[permalink] [raw]

Subject: Re: [PATCH 9/14] mips: use generic ptrace_resume code

On Tue, Feb 02, 2010 at 07:59:58PM +0100, Christoph Hellwig wrote:

> Use the generic ptrace_resume code for PTRACE_SYSCALL, PTRACE_CONT and
> PTRACE_KILL.
>
> Also the TIF_SYSCALL_TRACE thread flag is now cleared on PTRACE_KILL
> which it previously wasn't which is consistent with all architectures
> using the modern ptrace code.
>
> Signed-off-by: Christoph Hellwig <[email protected]>

Acked-by: Ralf Baechle <[email protected]>

Ralf

2010-02-02 20:30:10

[permalink] [raw]

Subject: Re: [PATCH 5/14] blackfin: use generic ptrace_resume code

On Tue, Feb 2, 2010 at 13:59, Christoph Hellwig wrote:
> Use the generic ptrace_resume code for PTRACE_SYSCALL, PTRACE_CONT,
> PTRACE_KILL and PTRACE_SINGLESTEP. This implies defining
> arch_has_single_step in <asm/ptrace.h> and implementing the
> user_enable_single_step and user_disable_single_step functions, which
> also causes the breakpoint information to be cleared on fork, which
> could be considered a bug fix.
>
> Also the TIF_SYSCALL_TRACE thread flag is now cleared on PTRACE_KILL
> which it previously wasn't which is consistent with all architectures
> using the modern ptrace code.

i have local code in the Blackfin tree that does part of this patch
(and has been tested), so i'll split this patch apart and merge into
my tree. thanks!
-mike

2010-02-03 03:44:17

by Haavard Skinnemoen

[permalink] [raw]

Subject: Re: [PATCH 4/14] avr32: use generic ptrace_resume code

On Tue, 2 Feb 2010 19:58:57 +0100
Christoph Hellwig <[email protected]> wrote:

> Use the generic ptrace_resume code for PTRACE_SYSCALL, PTRACE_CONT,
> PTRACE_KILL and PTRACE_SINGLESTEP. This implies defining
> arch_has_single_step in <asm/ptrace.h> and implementing the
> user_enable_single_step and user_disable_single_step functions, which
> also causes the breakpoint information to be cleared on fork, which
> could be considered a bug fix.
>
> Also the TIF_SYSCALL_TRACE thread flag is now cleared on PTRACE_KILL
> which it previously wasn't which is consistent with all architectures
> using the modern ptrace code.
>
> Currently avr32 doesn't implement any code to disable single stepping
> when one of the non-syscall requests is called which seems wrong,
> but I've left it as-is for now.
>
> Signed-off-by: Christoph Hellwig <[email protected]>

Acked-by: Haavard Skinnemoen <[email protected]>

Sounds like you found three bugs and fixed two of them. Can you tell me
more about the last one, i.e. what should be done to disable
single-stepping?

Haavard

2010-02-03 04:35:45

by Matt Turner

[permalink] [raw]

Subject: Re: [PATCH 2/14] alpha: use generic ptrace_resume code

On Tue, Feb 2, 2010 at 1:58 PM, Christoph Hellwig <[email protected]> wrote:
> Use the generic ptrace_resume code for PTRACE_SYSCALL, PTRACE_CONT,
> PTRACE_KILL and PTRACE_SINGLESTEP. ?This implies defining
> arch_has_single_step in <asm/ptrace.h> and implementing the
> user_enable_single_step and user_disable_single_step functions, which
> also causes the breakpoint information to be cleared on fork, which
> could be considered a bug fix.
>
> Also the TIF_SYSCALL_TRACE thread flag is now cleared on PTRACE_KILL
> which it previously wasn't, which is consistent with all architectures
> using the modern ptrace code.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
>
> Index: linux-2.6/arch/alpha/kernel/ptrace.c
> ===================================================================
> --- linux-2.6.orig/arch/alpha/kernel/ptrace.c ? 2010-02-02 11:05:03.197004058 +0100
> +++ linux-2.6/arch/alpha/kernel/ptrace.c ? ? ? ?2010-02-02 11:07:04.091006019 +0100
> @@ -249,6 +249,17 @@ ptrace_cancel_bpt(struct task_struct * c
> ? ? ? ?return (nsaved != 0);
> ?}
>
> +void user_enable_single_step(struct task_struct *child)
> +{
> + ? ? ? /* Mark single stepping. ?*/
> + ? ? ? task_thread_info(child)->bpt_nsaved = -1;
> +}
> +
> +void user_disable_single_step(struct task_struct *child)
> +{
> + ? ? ? ptrace_cancel_bpt(child);
> +}
> +
> ?/*
> ?* Called by kernel/ptrace.c when detaching..
> ?*
> @@ -256,7 +267,7 @@ ptrace_cancel_bpt(struct task_struct * c
> ?*/
> ?void ptrace_disable(struct task_struct *child)
> ?{
> - ? ? ? ptrace_cancel_bpt(child);
> + ? ? ? user_disable_single_step(child);
> ?}
>
> ?long arch_ptrace(struct task_struct *child, long request, long addr, long data)
> @@ -289,52 +300,6 @@ long arch_ptrace(struct task_struct *chi
> ? ? ? ? ? ? ? ?DBG(DBG_MEM, ("poke $%ld<-%#lx\n", addr, data));
> ? ? ? ? ? ? ? ?ret = put_reg(child, addr, data);
> ? ? ? ? ? ? ? ?break;
> -
> - ? ? ? case PTRACE_SYSCALL:
> - ? ? ? ? ? ? ? /* continue and stop at next (return from) syscall */
> - ? ? ? case PTRACE_CONT: ? ?/* restart after signal. */
> - ? ? ? ? ? ? ? ret = -EIO;
> - ? ? ? ? ? ? ? if (!valid_signal(data))
> - ? ? ? ? ? ? ? ? ? ? ? break;
> - ? ? ? ? ? ? ? if (request == PTRACE_SYSCALL)
> - ? ? ? ? ? ? ? ? ? ? ? set_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
> - ? ? ? ? ? ? ? else
> - ? ? ? ? ? ? ? ? ? ? ? clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
> - ? ? ? ? ? ? ? child->exit_code = data;
> - ? ? ? ? ? ? ? /* make sure single-step breakpoint is gone. */
> - ? ? ? ? ? ? ? ptrace_cancel_bpt(child);
> - ? ? ? ? ? ? ? wake_up_process(child);
> - ? ? ? ? ? ? ? ret = 0;
> - ? ? ? ? ? ? ? break;
> -
> - ? ? ? /*
> - ? ? ? ?* Make the child exit. ?Best I can do is send it a sigkill.
> - ? ? ? ?* perhaps it should be put in the status that it wants to
> - ? ? ? ?* exit.
> - ? ? ? ?*/
> - ? ? ? case PTRACE_KILL:
> - ? ? ? ? ? ? ? ret = 0;
> - ? ? ? ? ? ? ? if (child->exit_state == EXIT_ZOMBIE)
> - ? ? ? ? ? ? ? ? ? ? ? break;
> - ? ? ? ? ? ? ? child->exit_code = SIGKILL;
> - ? ? ? ? ? ? ? /* make sure single-step breakpoint is gone. */
> - ? ? ? ? ? ? ? ptrace_cancel_bpt(child);
> - ? ? ? ? ? ? ? wake_up_process(child);
> - ? ? ? ? ? ? ? break;
> -
> - ? ? ? case PTRACE_SINGLESTEP: ?/* execute single instruction. */
> - ? ? ? ? ? ? ? ret = -EIO;
> - ? ? ? ? ? ? ? if (!valid_signal(data))
> - ? ? ? ? ? ? ? ? ? ? ? break;
> - ? ? ? ? ? ? ? /* Mark single stepping. ?*/
> - ? ? ? ? ? ? ? task_thread_info(child)->bpt_nsaved = -1;
> - ? ? ? ? ? ? ? clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
> - ? ? ? ? ? ? ? child->exit_code = data;
> - ? ? ? ? ? ? ? wake_up_process(child);
> - ? ? ? ? ? ? ? /* give it a chance to run. */
> - ? ? ? ? ? ? ? ret = 0;
> - ? ? ? ? ? ? ? break;
> -
> ? ? ? ?default:
> ? ? ? ? ? ? ? ?ret = ptrace_request(child, request, addr, data);
> ? ? ? ? ? ? ? ?break;
> Index: linux-2.6/arch/alpha/include/asm/ptrace.h
> ===================================================================
> --- linux-2.6.orig/arch/alpha/include/asm/ptrace.h ? ? ?2010-02-02 11:07:29.091023600 +0100
> +++ linux-2.6/arch/alpha/include/asm/ptrace.h ? 2010-02-02 11:07:38.645007248 +0100
> @@ -68,6 +68,7 @@ struct switch_stack {
>
> ?#ifdef __KERNEL__
>
> +#define arch_has_single_step() ? ? ? ? (1)
> ?#define user_mode(regs) (((regs)->ps & 8) != 0)
> ?#define instruction_pointer(regs) ((regs)->pc)
> ?#define profile_pc(regs) instruction_pointer(regs)
>

Acked-by: Matt Turner <[email protected]>

2010-02-03 06:55:07

by Greg Ungerer

[permalink] [raw]

Subject: Re: [PATCH 7/14] m68knommu: use generic ptrace_resume code

Christoph Hellwig wrote:
> Use the generic ptrace_resume code for PTRACE_SYSCALL, PTRACE_CONT,
> PTRACE_KILL and PTRACE_SINGLESTEP. m68knommu already defines the
> nessecary user_enable_single_step and user_disable_single_step functions
> for this.
>
> Also the TIF_SYSCALL_TRACE thread flag is now cleared on PTRACE_KILL
> which it previously wasn't which is consistent with all architectures
> using the modern ptrace code.
>
> Signed-off-by: Christoph Hellwig <[email protected]>

Acked-by: Greg Ungerer <[email protected]>

------------------------------------------------------------------------
Greg Ungerer -- Principal Engineer EMAIL: [email protected]
SnapGear Group, McAfee PHONE: +61 7 3435 2888
8 Gardner Close FAX: +61 7 3217 5323
Milton, QLD, 4064, Australia WEB: http://www.SnapGear.com

2010-02-03 08:36:42

[permalink] [raw]

Subject: Re: [PATCH 4/14] avr32: use generic ptrace_resume code

On Wed, Feb 03, 2010 at 08:47:37AM +0530, Haavard Skinnemoen wrote:
> Sounds like you found three bugs and fixed two of them. Can you tell me
> more about the last one, i.e. what should be done to disable
> single-stepping?

I don't think the flag clearing on PTRACE_KILL matters as the process
is a zombie and almost gone anyway, it's more an artefact of the
generic implementation that I wanted to mention.

It seems like avr32 implement single stepping by setting the
TIF_SINGLE_STEP and TIF_BREAKPOINT thread flags. Simply clearing these
flags in user_disable_single_step and calling that function from
ptrace_disable (which currently clears them itself) should take care
of it.

2010-02-03 08:42:27

[permalink] [raw]

Subject: Re: [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h

On Tue, Feb 2, 2010 at 13:57, Christoph Hellwig wrote:
> While in theory user_enable_single_step/user_disable_single_step/
> user_enable_blockstep could also be provided as an inline or macro there's no
> good reason to do so, and having the prototype in one places keeps code size
> and confusion down.

the only annoying thing here is that we currently have to enable both
user_disable_single_step() and ptrace_disable() that do exactly the
same thing. i avoided this somewhat on Blackfin by cheating:
#define user_disable_single_step(child) ptrace_disable(child)

so now there's no code bloat. perhaps this could be moved into common
linux/ptrace.h too ?

> --- linux-2.6/include/linux/ptrace.h
> +++ linux-2.6/include/linux/ptrace.h
> @@ -238,6 +238,9 @@
> static inline void user_disable_single_step(struct task_struct *task)
> {
> }
> +#else
> +extern void user_enable_single_step(struct task_struct *);
> +extern void user_disable_single_step(struct task_struct *);
> #endif /* arch_has_single_step */

+#define ptrace_disable(child) user_disable_single_step(child)
-mike

2010-02-03 08:58:52

[permalink] [raw]

Subject: Re: [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h

On Wed, Feb 03, 2010 at 03:42:05AM -0500, Mike Frysinger wrote:
> On Tue, Feb 2, 2010 at 13:57, Christoph Hellwig wrote:
> > While in theory user_enable_single_step/user_disable_single_step/
> > user_enable_blockstep could also be provided as an inline or macro there's no
> > good reason to do so, and having the prototype in one places keeps code size
> > and confusion down.
>
> the only annoying thing here is that we currently have to enable both
> user_disable_single_step() and ptrace_disable() that do exactly the
> same thing. i avoided this somewhat on Blackfin by cheating:
> #define user_disable_single_step(child) ptrace_disable(child)
>
> so now there's no code bloat. perhaps this could be moved into common
> linux/ptrace.h too ?

What is done by most architectures is ptrace_disable simply
calling user_disable_single_step. Long-term I expect ptrace_disable to
go away entirely. While a few architectures do more than just
user_disable_single_step in it that seems at least fishy to me, but
I'll wait with the audit until we have everyone actually using
ptrace_resume.

2010-02-03 11:02:36

by Michal Simek

[permalink] [raw]

Subject: Re: [PATCH 8/14] microblaze: use generic ptrace_resume code

Acked-by: Michal Simek <[email protected]>

Christoph Hellwig wrote:
> Use the generic ptrace_resume code for PTRACE_SYSCALL, PTRACE_CONT and
> PTRACE_KILL. This also makes PTRACE_SINGLESTEP return -EIO while it
> previously succeeded despite not actually causing any kind of single
> stepping.
>
> Also the TIF_SYSCALL_TRACE thread flag is now cleared on PTRACE_KILL
> which it previously wasn't which is consistent with all architectures
> using the modern ptrace code.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
>
> Index: linux-2.6/arch/microblaze/kernel/ptrace.c
> ===================================================================
> --- linux-2.6.orig/arch/microblaze/kernel/ptrace.c 2010-02-02 11:23:56.002003861 +0100
> +++ linux-2.6/arch/microblaze/kernel/ptrace.c 2010-02-02 19:20:16.997254566 +0100
> @@ -110,43 +110,6 @@ long arch_ptrace(struct task_struct *chi
> if (rval == 0 && request == PTRACE_PEEKUSR)
> rval = put_user(val, (unsigned long *)data);
> break;
> - /* Continue and stop at next (return from) syscall */
> - case PTRACE_SYSCALL:
> - pr_debug("PTRACE_SYSCALL\n");
> - case PTRACE_SINGLESTEP:
> - pr_debug("PTRACE_SINGLESTEP\n");
> - /* Restart after a signal. */
> - case PTRACE_CONT:
> - pr_debug("PTRACE_CONT\n");
> - rval = -EIO;
> - if (!valid_signal(data))
> - break;
> -
> - if (request == PTRACE_SYSCALL)
> - set_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
> - else
> - clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
> -
> - child->exit_code = data;
> - pr_debug("wakeup_process\n");
> - wake_up_process(child);
> - rval = 0;
> - break;
> -
> - /*
> - * make the child exit. Best I can do is send it a sigkill.
> - * perhaps it should be put in the status that it wants to
> - * exit.
> - */
> - case PTRACE_KILL:
> - pr_debug("PTRACE_KILL\n");
> - rval = 0;
> - if (child->exit_state == EXIT_ZOMBIE) /* already dead */
> - break;
> - child->exit_code = SIGKILL;
> - wake_up_process(child);
> - break;
> -
> default:
> rval = ptrace_request(child, request, addr, data);
> }

--
Michal Simek, Ing. (M.Eng)
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian

2010-02-03 19:23:42

by Oleg Nesterov

[permalink] [raw]

Subject: Re: [PATCH 4/14] avr32: use generic ptrace_resume code

On 02/03, Christoph Hellwig wrote:
>
> I don't think the flag clearing on PTRACE_KILL matters as the process
> is a zombie and almost gone anyway, it's more an artefact of the
> generic implementation that I wanted to mention.

Well, PTRACE_KILL doesn't necessarily kill the tracee. Only if the
tracee reported the signal or syscall, otherwise ->exit_code is
ignored.

OTOH I agree, probably the flag clearing on PTRACE_KILL doesn't
really matter beause PTRACE_KILL shouldn't be used at all.

The whole series looks "obviously good" to me, but of course
this is up to arch maintaners.

Oleg.

2010-02-03 19:29:37

[permalink] [raw]

Subject: Re: [PATCH 4/14] avr32: use generic ptrace_resume code

On Wed, Feb 03, 2010 at 08:22:23PM +0100, Oleg Nesterov wrote:
> On 02/03, Christoph Hellwig wrote:
> >
> > I don't think the flag clearing on PTRACE_KILL matters as the process
> > is a zombie and almost gone anyway, it's more an artefact of the
> > generic implementation that I wanted to mention.
>
> Well, PTRACE_KILL doesn't necessarily kill the tracee. Only if the
> tracee reported the signal or syscall, otherwise ->exit_code is
> ignored.
>
> OTOH I agree, probably the flag clearing on PTRACE_KILL doesn't
> really matter beause PTRACE_KILL shouldn't be used at all.

You're right of course. When writing the above I somehow had a mental
picture of only the EXIT_ZOMBIE case, but we're not even clearing the
flag in that case.

2010-02-03 19:37:14

[permalink] [raw]

Subject: Re: [PATCH 5/14] blackfin: use generic ptrace_resume code

On Tue, Feb 2, 2010 at 15:29, Mike Frysinger wrote:
> On Tue, Feb 2, 2010 at 13:59, Christoph Hellwig wrote:
>> Use the generic ptrace_resume code for PTRACE_SYSCALL, PTRACE_CONT,
>> PTRACE_KILL and PTRACE_SINGLESTEP. This implies defining
>> arch_has_single_step in <asm/ptrace.h> and implementing the
>> user_enable_single_step and user_disable_single_step functions, which
>> also causes the breakpoint information to be cleared on fork, which
>> could be considered a bug fix.
>>
>> Also the TIF_SYSCALL_TRACE thread flag is now cleared on PTRACE_KILL
>> which it previously wasn't which is consistent with all architectures
>> using the modern ptrace code.
>
> i have local code in the Blackfin tree that does part of this patch
> (and has been tested), so i'll split this patch apart and merge into
> my tree. thanks!

i added tracehook support to Blackfin recently, so that covered all
the new functions here. i just had to drop the handling of the
PTRACE_xxx things that common code already does.

when did you want to push through these updates ? i was planning on
sending these ptrace() updates through the Blackfin tree as part of my
2.6.34 queue. i'm guessing you didnt want this stuff in 2.6.33 ...
-mike

2010-02-03 19:43:08

[permalink] [raw]

Subject: Re: [PATCH 5/14] blackfin: use generic ptrace_resume code

On Wed, Feb 03, 2010 at 02:36:47PM -0500, Mike Frysinger wrote:
> i added tracehook support to Blackfin recently, so that covered all
> the new functions here. i just had to drop the handling of the
> PTRACE_xxx things that common code already does.
>
> when did you want to push through these updates ? i was planning on
> sending these ptrace() updates through the Blackfin tree as part of my
> 2.6.34 queue. i'm guessing you didnt want this stuff in 2.6.33 ...

No, it's all .34 material. I'd say send your bits through the
microblaze tree. The double prototype for the two functions won't
hurt during the merge window and we can fix it up later. Just make
sure you really have user_{enable,disable}_single_step implement
as functions and not as a #define for ptrace_disable as in your
earlier version.

2010-02-08 10:53:13

by David Howells

[permalink] [raw]

Subject: Re: [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h

Christoph Hellwig <[email protected]> wrote:

> While in theory user_enable_single_step/user_disable_single_step/
> user_enable_blockstep could also be provided as an inline or macro there's no
> good reason to do so, and having the prototype in one places keeps code size
> and confusion down.
>
> Signed-off-by: Christoph Hellwig <[email protected]>

Acked-by: David Howells <[email protected]>

2010-02-08 19:53:14

[permalink] [raw]

Subject: Re: [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h

The original thought there was that user_enable_single_step() et al might
well be only an instruction or three on a sane machine (as if we have any
of those!), and since there is only one call site inlining would be
beneficial. But I agree that there is no strong reason to care about
inlining it.

As to the arch changes, there is only one thought I'd add to the record.
It was always my thinking that for an arch where PTRACE_SINGLESTEP does
text-modifying breakpoint insertion, user_enable_single_step() should not
be provided. That is, arch_has_single_step()=>true means that there is an
arch facility with "pure" semantics that does not have any unexpected side
effects. Inserting a breakpoint might do very unexpected strange things in
multi-threaded situations. Aside from that, it is a peculiar side effect
that user_{enable,disable}_single_step() should cause COW de-sharing of
text pages and so forth. For PTRACE_SINGLESTEP, all these peculiarities
are the status quo ante for that arch, so having arch_ptrace() itself do
those is one thing. But for building other things in the future, it is
nicer to have a uniform "pure" semantics that arch-independent code can
expect.

OTOH, all such arch issues are really up to the arch maintainer. As of
today, there is nothing but ptrace using user_enable_single_step() et al so
it's a distinction without a practical difference. If/when there are other
facilities that use user_enable_single_step() and might care, the affected
arch's can revisit the question when someone cares about the quality of the
arch support for said new facility.

So, with those caveats preserved for posteriority, all these changes
are OK with me.

Thanks,
Roland

2010-02-10 22:08:45

[permalink] [raw]

Subject: Re: [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h

On Mon, Feb 08, 2010 at 11:51:25AM -0800, Roland McGrath wrote:
> The original thought there was that user_enable_single_step() et al might
> well be only an instruction or three on a sane machine (as if we have any
> of those!), and since there is only one call site inlining would be
> beneficial. But I agree that there is no strong reason to care about
> inlining it.

In fact many of the implementations are small enough to inline them, but
not no architecture so far decided to inline them, and in the end it's
not exactly a fast-path. This at least keeps the prototype the same
everywhere.

> As to the arch changes, there is only one thought I'd add to the record.
> It was always my thinking that for an arch where PTRACE_SINGLESTEP does
> text-modifying breakpoint insertion, user_enable_single_step() should not
> be provided. That is, arch_has_single_step()=>true means that there is an
> arch facility with "pure" semantics that does not have any unexpected side
> effects. Inserting a breakpoint might do very unexpected strange things in
> multi-threaded situations. Aside from that, it is a peculiar side effect
> that user_{enable,disable}_single_step() should cause COW de-sharing of
> text pages and so forth. For PTRACE_SINGLESTEP, all these peculiarities
> are the status quo ante for that arch, so having arch_ptrace() itself do
> those is one thing. But for building other things in the future, it is
> nicer to have a uniform "pure" semantics that arch-independent code can
> expect.

For now the ptrace code is the same for real or software singlestepping.
If we grow users that care we can add a arch_has_hw_single_step macro
or just overload arch_has_single_step with different postitive return
values for the exact same type of single stepping supported.

2010-02-11 09:41:50

[permalink] [raw]

Subject: [PATCH 0/2] Blackfin: use generic ptrace_resume code

These are the changes I'm going to be pushing out for 2.6.34. They aren't
trivial to rebase, so I'm keeping them in my tree rather than pushing as
part of set through another. They aren't dependent on anything pending
anyways (seem to work fine on top of 2.6.32.x).

2010-02-11 09:41:58

[permalink] [raw]

Subject: [PATCH 1/2] Blackfin: initial tracehook support

Signed-off-by: Mike Frysinger <[email protected]>
---
arch/blackfin/Kconfig | 1 +
arch/blackfin/include/asm/ptrace.h | 23 ++++++++
arch/blackfin/include/asm/syscall.h | 96 +++++++++++++++++++++++++++++++++++
arch/blackfin/kernel/ptrace.c | 71 +++++++-------------------
arch/blackfin/kernel/signal.c | 4 +-
arch/blackfin/mach-common/entry.S | 6 ++-
6 files changed, 145 insertions(+), 56 deletions(-)
create mode 100644 arch/blackfin/include/asm/syscall.h

diff --git a/arch/blackfin/Kconfig b/arch/blackfin/Kconfig
index cdd87a0..13d8fe9 100644
--- a/arch/blackfin/Kconfig
+++ b/arch/blackfin/Kconfig
@@ -24,6 +24,7 @@ config RWSEM_XCHGADD_ALGORITHM
config BLACKFIN
def_bool y
select HAVE_ARCH_KGDB
+ select HAVE_ARCH_TRACEHOOK
select HAVE_FUNCTION_GRAPH_TRACER
select HAVE_FUNCTION_TRACER
select HAVE_IDE
diff --git a/arch/blackfin/include/asm/ptrace.h b/arch/blackfin/include/asm/ptrace.h
index b33a448..f3e75fa 100644
--- a/arch/blackfin/include/asm/ptrace.h
+++ b/arch/blackfin/include/asm/ptrace.h
@@ -24,6 +24,8 @@

#ifndef __ASSEMBLY__

+struct task_struct;
+
/* this struct defines the way the registers are stored on the
stack during a system call. */

@@ -101,9 +103,30 @@ struct pt_regs {
master interrupt enable. */
#define user_mode(regs) (!(((regs)->ipend & ~0x10) & (((regs)->ipend & ~0x10) - 1)))
#define instruction_pointer(regs) ((regs)->pc)
+#define user_stack_pointer(regs) ((regs)->usp)
#define profile_pc(regs) instruction_pointer(regs)
extern void show_regs(struct pt_regs *);

+#define arch_has_single_step() (1)
+extern void user_enable_single_step(struct task_struct *);
+extern void user_disable_single_step(struct task_struct *child);
+/* common code demands this function */
+#define ptrace_disable(child) user_disable_single_step(child)
+
+/*
+ * Get the address of the live pt_regs for the specified task.
+ * These are saved onto the top kernel stack when the process
+ * is not running.
+ *
+ * Note: if a user thread is execve'd from kernel space, the
+ * kernel stack will not be empty on entry to the kernel, so
+ * ptracing these tasks will fail.
+ */
+#define task_pt_regs(task) \
+ (struct pt_regs *) \
+ ((unsigned long)task_stack_page(task) + \
+ (THREAD_SIZE - sizeof(struct pt_regs)))
+
#endif /* __KERNEL__ */

#endif /* __ASSEMBLY__ */
diff --git a/arch/blackfin/include/asm/syscall.h b/arch/blackfin/include/asm/syscall.h
new file mode 100644
index 0000000..74e0458
--- /dev/null
+++ b/arch/blackfin/include/asm/syscall.h
@@ -0,0 +1,96 @@
+/*
+ * Magic syscall break down functions
+ *
+ * Copyright 2010 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2 or later.
+ */
+
+#ifndef __ASM_BLACKFIN_SYSCALL_H__
+#define __ASM_BLACKFIN_SYSCALL_H__
+
+/*
+ * Blackfin syscalls are simple:
+ * enter:
+ * p0: syscall number
+ * r{0,1,2,3,4,5}: syscall args 0,1,2,3,4,5
+ * exit:
+ * r0: return/error value
+ */
+
+#include <linux/err.h>
+#include <linux/sched.h>
+#include <asm/ptrace.h>
+
+static inline long
+syscall_get_nr(struct task_struct *task, struct pt_regs *regs)
+{
+ return regs->p0;
+}
+
+static inline void
+syscall_rollback(struct task_struct *task, struct pt_regs *regs)
+{
+ /* was zu tun !? */
+}
+
+static inline long
+syscall_get_error(struct task_struct *task, struct pt_regs *regs)
+{
+ return IS_ERR_VALUE(regs->r0) ? regs->r0 : 0;
+}
+
+static inline long
+syscall_get_return_value(struct task_struct *task, struct pt_regs *regs)
+{
+ return regs->r0;
+}
+
+static inline void
+syscall_set_return_value(struct task_struct *task, struct pt_regs *regs,
+ int error, long val)
+{
+ regs->r0 = error ? -error : val;
+}
+
+static inline void
+syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
+ unsigned int i, unsigned int n, unsigned long *args)
+{
+ /* wtf is "i" ? */
+ BUG_ON(i);
+
+ switch (n) {
+ case 6: args[5] = regs->r5;
+ case 5: args[4] = regs->r4;
+ case 4: args[3] = regs->r3;
+ case 3: args[2] = regs->r2;
+ case 2: args[1] = regs->r1;
+ case 1: args[0] = regs->r0;
+ break;
+ default:
+ BUG();
+ }
+}
+
+static inline void
+syscall_set_arguments(struct task_struct *task, struct pt_regs *regs,
+ unsigned int i, unsigned int n, const unsigned long *args)
+{
+ /* wtf is "i" ? */
+ BUG_ON(i);
+
+ switch (n) {
+ case 6: regs->r5 = args[5];
+ case 5: regs->r4 = args[4];
+ case 4: regs->r3 = args[3];
+ case 3: regs->r2 = args[2];
+ case 2: regs->r1 = args[1];
+ case 1: regs->r0 = args[0];
+ break;
+ default:
+ BUG();
+ }
+}
+
+#endif
diff --git a/arch/blackfin/kernel/ptrace.c b/arch/blackfin/kernel/ptrace.c
index 65567dc..f6e94ad 100644
--- a/arch/blackfin/kernel/ptrace.c
+++ b/arch/blackfin/kernel/ptrace.c
@@ -13,6 +13,7 @@
#include <linux/ptrace.h>
#include <linux/user.h>
#include <linux/signal.h>
+#include <linux/tracehook.h>
#include <linux/uaccess.h>

#include <asm/page.h>
@@ -37,32 +38,13 @@
/* sets the trace bits. */
#define TRACE_BITS 0x0001

-/* Find the stack offset for a register, relative to thread.esp0. */
-#define PT_REG(reg) ((long)&((struct pt_regs *)0)->reg)
-
-/*
- * Get the address of the live pt_regs for the specified task.
- * These are saved onto the top kernel stack when the process
- * is not running.
- *
- * Note: if a user thread is execve'd from kernel space, the
- * kernel stack will not be empty on entry to the kernel, so
- * ptracing these tasks will fail.
- */
-static inline struct pt_regs *get_user_regs(struct task_struct *task)
-{
- return (struct pt_regs *)
- ((unsigned long)task_stack_page(task) +
- (THREAD_SIZE - sizeof(struct pt_regs)));
-}
-
/*
* Get all user integer registers.
*/
static inline int ptrace_getregs(struct task_struct *tsk, void __user *uregs)
{
struct pt_regs regs;
- memcpy(&regs, get_user_regs(tsk), sizeof(regs));
+ memcpy(&regs, task_pt_regs(tsk), sizeof(regs));
regs.usp = tsk->thread.usp;
return copy_to_user(uregs, &regs, sizeof(struct pt_regs)) ? -EFAULT : 0;
}
@@ -78,10 +60,8 @@ static inline int ptrace_getregs(struct task_struct *tsk, void __user *uregs)
static inline long get_reg(struct task_struct *task, int regno)
{
unsigned char *reg_ptr;
+ struct pt_regs *regs = task_pt_regs(task);

- struct pt_regs *regs =
- (struct pt_regs *)((unsigned long)task_stack_page(task) +
- (THREAD_SIZE - sizeof(struct pt_regs)));
reg_ptr = (char *)regs;

switch (regno) {
@@ -104,10 +84,8 @@ static inline int
put_reg(struct task_struct *task, int regno, unsigned long data)
{
char *reg_ptr;
+ struct pt_regs *regs = task_pt_regs(task);

- struct pt_regs *regs =
- (struct pt_regs *)((unsigned long)task_stack_page(task) +
- (THREAD_SIZE - sizeof(struct pt_regs)));
reg_ptr = (char *)regs;

switch (regno) {
@@ -160,7 +138,7 @@ static inline int is_user_addr_valid(struct task_struct *child,
return -EIO;
}

-void ptrace_enable(struct task_struct *child)
+void user_enable_single_step(struct task_struct *child)
{
unsigned long tmp;
tmp = get_reg(child, PT_SYSCFG) | (TRACE_BITS);
@@ -169,10 +147,8 @@ void ptrace_enable(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)
+void user_disable_single_step(struct task_struct *child)
{
unsigned long tmp;
/* make sure the single step bit is not set. */
@@ -366,7 +342,7 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
else
clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
child->exit_code = data;
- ptrace_disable(child);
+ user_disable_single_step(child);
pr_debug("ptrace: before wake_up_process\n");
wake_up_process(child);
ret = 0;
@@ -382,7 +358,7 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
if (child->exit_state == EXIT_ZOMBIE) /* already dead */
break;
child->exit_code = SIGKILL;
- ptrace_disable(child);
+ user_disable_single_step(child);
wake_up_process(child);
break;

@@ -392,7 +368,7 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
if (!valid_signal(data))
break;
clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
- ptrace_enable(child);
+ user_enable_single_step(child);
child->exit_code = data;
wake_up_process(child);
ret = 0;
@@ -417,27 +393,18 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
return ret;
}

-asmlinkage void syscall_trace(void)
+asmlinkage int syscall_trace_enter(struct pt_regs *regs)
{
- if (!test_thread_flag(TIF_SYSCALL_TRACE))
- return;
+ int ret = 0;

- if (!(current->ptrace & PT_PTRACED))
- return;
+ if (test_thread_flag(TIF_SYSCALL_TRACE))
+ ret = tracehook_report_syscall_entry(regs);

- /* the 0x80 provides a way for the tracing parent to distinguish
- * between a syscall stop and SIGTRAP delivery
- */
- ptrace_notify(SIGTRAP | ((current->ptrace & PT_TRACESYSGOOD)
- ? 0x80 : 0));
+ return ret;
+}

- /*
- * this isn't the same as continuing with a signal, but it will do
- * for normal use. strace only continues with a signal if the
- * stopping signal is not SIGTRAP. -brl
- */
- if (current->exit_code) {
- send_sig(current->exit_code, current, 1);
- current->exit_code = 0;
- }
+asmlinkage void syscall_trace_leave(struct pt_regs *regs)
+{
+ if (test_thread_flag(TIF_SYSCALL_TRACE))
+ tracehook_report_syscall_exit(regs, 0);
}
diff --git a/arch/blackfin/kernel/signal.c b/arch/blackfin/kernel/signal.c
index e0fd63e..1f28852 100644
--- a/arch/blackfin/kernel/signal.c
+++ b/arch/blackfin/kernel/signal.c
@@ -1,5 +1,5 @@
/*
- * Copyright 2004-2009 Analog Devices Inc.
+ * Copyright 2004-2010 Analog Devices Inc.
*
* Licensed under the GPL-2 or later
*/
@@ -213,7 +213,7 @@ setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t * info,
*/
if (regs->syscfg & TRACE_BITS) {
regs->syscfg &= ~TRACE_BITS;
- ptrace_notify(SIGTRAP);
+ tracehook_signal_handler(sig, info, ka, regs, 1);
}

return 0;
diff --git a/arch/blackfin/mach-common/entry.S b/arch/blackfin/mach-common/entry.S
index 16abeb8..365d506 100644
--- a/arch/blackfin/mach-common/entry.S
+++ b/arch/blackfin/mach-common/entry.S
@@ -736,7 +736,8 @@ ENDPROC(_system_call)
* this symbol need not be global anyways, so ...
*/
_sys_trace:
- pseudo_long_call _syscall_trace, p5;
+ r0 = sp;
+ pseudo_long_call _syscall_trace_enter, p5;

/* Execute the appropriate system call */

@@ -760,7 +761,8 @@ _sys_trace:
SP += 24;
[sp + PT_R0] = r0;

- pseudo_long_call _syscall_trace, p5;
+ r0 = sp;
+ pseudo_long_call _syscall_trace_leave, p5;
jump .Lresume_userspace;
ENDPROC(_sys_trace)

--
1.6.6.1

2010-02-11 09:41:57

[permalink] [raw]

Subject: [PATCH 2/2] Blackfin: use generic ptrace_resume code

From: Christoph Hellwig <[email protected]>

Use the generic ptrace_resume code for PTRACE_SYSCALL, PTRACE_CONT,
PTRACE_KILL and PTRACE_SINGLESTEP. This implies defining
arch_has_single_step in <asm/ptrace.h> and implementing the
user_enable_single_step and user_disable_single_step functions, which
also causes the breakpoint information to be cleared on fork, which
could be considered a bug fix.

Also the TIF_SYSCALL_TRACE thread flag is now cleared on PTRACE_KILL
which it previously wasn't which is consistent with all architectures
using the modern ptrace code.

Signed-off-by: Christoph Hellwig <[email protected]>
Signed-off-by: Mike Frysinger <[email protected]>
---
arch/blackfin/kernel/ptrace.c | 44 -----------------------------------------
1 files changed, 0 insertions(+), 44 deletions(-)

diff --git a/arch/blackfin/kernel/ptrace.c b/arch/blackfin/kernel/ptrace.c
index f6e94ad..09fb04e 100644
--- a/arch/blackfin/kernel/ptrace.c
+++ b/arch/blackfin/kernel/ptrace.c
@@ -330,50 +330,6 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
ret = put_reg(child, addr, data);
break;

- case PTRACE_SYSCALL: /* continue and stop at next (return from) syscall */
- case PTRACE_CONT: /* restart after signal. */
- pr_debug("ptrace: syscall/cont\n");
-
- ret = -EIO;
- if (!valid_signal(data))
- break;
- if (request == PTRACE_SYSCALL)
- set_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
- else
- clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
- child->exit_code = data;
- user_disable_single_step(child);
- pr_debug("ptrace: before wake_up_process\n");
- wake_up_process(child);
- ret = 0;
- break;
-
- /*
- * make the child exit. Best I can do is send it a sigkill.
- * perhaps it should be put in the status that it wants to
- * exit.
- */
- case PTRACE_KILL:
- ret = 0;
- if (child->exit_state == EXIT_ZOMBIE) /* already dead */
- break;
- child->exit_code = SIGKILL;
- user_disable_single_step(child);
- wake_up_process(child);
- break;
-
- case PTRACE_SINGLESTEP: /* set the trap flag. */
- pr_debug("ptrace: single step\n");
- ret = -EIO;
- if (!valid_signal(data))
- break;
- clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
- user_enable_single_step(child);
- child->exit_code = data;
- wake_up_process(child);
- ret = 0;
- break;
-
case PTRACE_GETREGS:
/* Get all gp regs from the child. */
ret = ptrace_getregs(child, datap);
--
1.6.6.1

2010-02-11 20:47:41

[permalink] [raw]

Subject: Re: [PATCH 1/2] Blackfin: initial tracehook support

> config BLACKFIN
> def_bool y
> select HAVE_ARCH_KGDB
> + select HAVE_ARCH_TRACEHOOK

Don't define this until you have all its constituents as listed in the
arch/Kconfig comment. I don't see user_regset support.

> +static inline void
> +syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
> + unsigned int i, unsigned int n, unsigned long *args)
> +{
> + /* wtf is "i" ? */
> + BUG_ON(i);

i is the starting number. args[0] gets the i'th argument,
args[n - 1] gets the i+n-1'th argument.

> +asmlinkage void syscall_trace_leave(struct pt_regs *regs)
> +{
> + if (test_thread_flag(TIF_SYSCALL_TRACE))
> + tracehook_report_syscall_exit(regs, 0);
> }

Is it in fact true that single-step reports still come normally after a
syscall instruction?

> @@ -213,7 +213,7 @@ setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t * info,
> */
> if (regs->syscfg & TRACE_BITS) {
> regs->syscfg &= ~TRACE_BITS;
> - ptrace_notify(SIGTRAP);
> + tracehook_signal_handler(sig, info, ka, regs, 1);
> }

This call should be made unconditionally, and it should be made after the
signal mask changes have been made (i.e. at the end of handle_signal). I
think it's wrong to clear the single-step flag here. Instead, pass
(regs->syscfg & TRACE_BITS) as the last argument.

With ptrace, it makes no difference one way or the other because it will
always either explicitly clear or explicitly set single-step before it
resumes. But in future, it will matter.

Thanks,
Roland

2010-02-11 23:54:27

[permalink] [raw]

Subject: Re: [PATCH 1/2] Blackfin: initial tracehook support

On Thu, Feb 11, 2010 at 15:46, Roland McGrath wrote:
>> config BLACKFIN
>> def_bool y
>> select HAVE_ARCH_KGDB
>> + select HAVE_ARCH_TRACEHOOK
>
> Don't define this until you have all its constituents as listed in the
> arch/Kconfig comment. I don't see user_regset support.

where is user_regset actually used ? i only see it in fs/binfmt_elf.c
and core dumps, neither of which work on nommu systems (or at least on
Blackfin systems).

>> +static inline void
>> +syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
>> + unsigned int i, unsigned int n, unsigned long *args)
>> +{
>> + /* wtf is "i" ? */
>> + BUG_ON(i);
>
> i is the starting number. args[0] gets the i'th argument,
> args[n - 1] gets the i+n-1'th argument.

i dont see anyone calling syscall_get_arguments() with i!=0, and a few
other arches are doing the BUG_ON(i) thing too.

but should be easy to implement this with memory walking code ...

>> +asmlinkage void syscall_trace_leave(struct pt_regs *regs)
>> +{
>> + if (test_thread_flag(TIF_SYSCALL_TRACE))
>> + tracehook_report_syscall_exit(regs, 0);
>> }
>
> Is it in fact true that single-step reports still come normally after a
> syscall instruction?

this is unchanged from the previous Blackfin behavior, and it's how
most arches behaved in 2.6.32. but looking in latest mainline, it
seems people are changing to:
if (test_thread_flag(TIF_SINGLESTEP) || test_thread_flag(TIF_SYSCALL_TRACE))
tracehook_report_syscall_exit(regs, 0);

so changing Blackfin too should be straightforward i guess

>> @@ -213,7 +213,7 @@
>> */
>> if (regs->syscfg & TRACE_BITS) {
>> regs->syscfg &= ~TRACE_BITS;
>> - ptrace_notify(SIGTRAP);
>> + tracehook_signal_handler(sig, info, ka, regs, 1);
>> }
>
> This call should be made unconditionally, and it should be made after the
> signal mask changes have been made (i.e. at the end of handle_signal). I
> think it's wrong to clear the single-step flag here. Instead, pass
> (regs->syscfg & TRACE_BITS) as the last argument.
>
> With ptrace, it makes no difference one way or the other because it will
> always either explicitly clear or explicitly set single-step before it
> resumes. But in future, it will matter.

sounds like this issue is unrelated to tracehook and how we've been
doing signal/ptrace stuff has always been a little broken ...

i'll move it to how most arches seem to do it -- in do_signal after a
successful call to handle_signal and after clearing
TIF_RESTORE_SIGMASK.

thanks for the review
-mike

2010-02-12 03:24:34

[permalink] [raw]

Subject: Re: [PATCH 1/2] Blackfin: initial tracehook support

> where is user_regset actually used ? i only see it in fs/binfmt_elf.c
> and core dumps, neither of which work on nommu systems (or at least on
> Blackfin systems).

The core dump code in binfmt_elf_fdpic.c appears identical to an old
version of the binfmt_elf.c code. That file appears to have been made with
the "copy and paste" school of code sharing, of which I am a detractor.
The ELF core dump code can be shared between those two, and really should
be. Once that's done, you will want to use the CORE_DUMP_USE_REGSET flavor
of the code and clean out any old core-related cruft you had in asm/elf.h.
In the long run, the non-user_regset version of the core dump code will go
away after every arch has been cleaned up.

The "ptrace: Add support for generic PTRACE_GETREGSET/PTRACE_SETREGSET"
patch making its way through the obstacle course right now will make the
generic ptrace code use user_regset. That use is conditional on
CONFIG_HAVE_ARCH_TRACEHOOK and your kernel will stop building if you have
set that without meeting its requirements.

Moreover, the whole point of CONFIG_HAVE_ARCH_TRACEHOOK is to indicate the
minimum arch requirements that all future generic code can rely on. If you
set it without meeting the documented requirements as arch/Kconfig tells
you to, then your arch will one day be broken by new generic code getting
merged in.

> i dont see anyone calling syscall_get_arguments() with i!=0, and a few
> other arches are doing the BUG_ON(i) thing too.

Someone will, and then they will crash. A few others being half-assed is
no good reason for you to follow suit.

> but should be easy to implement this with memory walking code ...

Good!

> this is unchanged from the previous Blackfin behavior, and it's how
> most arches behaved in 2.6.32. but looking in latest mainline, it
> seems people are changing to:
> if (test_thread_flag(TIF_SINGLESTEP) || test_thread_flag(TIF_SYSCALL_TRACE))
> tracehook_report_syscall_exit(regs, 0);
>
> so changing Blackfin too should be straightforward i guess

You can't blindly follow another arch. All the details that tell you what
is the right thing to do here are arch-specific. I see no arch that does
what you say, and it seems certain they would be wrong if they did.

You should always call tracehook_report_syscall_exit() if TIF_SYSCALL_TRACE
is set. Whether to call it otherwise depends on arch details.

On some machines, single-step into a syscall instruction is no different
from other user instructions, so the normal SIGTRAP will come afterwards
anyway.

On other machines, entering the kernel for the syscall instruction defeats
the normal user-mode effects of single-step being enabled. In that event,
you want to call tracehook_report_syscall_exit() if single-step is enabled.
You must pass a nonzero second argument if your arch code is not going to
generate the normal SIGTRAP associated with having single-stepped into the
syscall instruction.

> sounds like this issue is unrelated to tracehook and how we've been
> doing signal/ptrace stuff has always been a little broken ...

Yes. It's related to tracehook in the sense that the tracehook interfaces
as described by the kerneldoc cover explicitly all the corners of the
semantics that were fuzzy or implicit in the ancient code. Getting all
these corners right for your arch makes sure that any future generic
features that differ from the ancient ptrace muck will behave as intended
on your arch without you having to go fix things up later on.

> i'll move it to how most arches seem to do it -- in do_signal after a
> successful call to handle_signal and after clearing
> TIF_RESTORE_SIGMASK.

That is correct.

Thanks,
Roland

2010-02-12 04:33:45

[permalink] [raw]

Subject: Re: [PATCH 1/2] Blackfin: initial tracehook support

On Thu, Feb 11, 2010 at 22:24, Roland McGrath wrote:
>> where is user_regset actually used ? i only see it in fs/binfmt_elf.c
>> and core dumps, neither of which work on nommu systems (or at least on
>> Blackfin systems).
>
> The core dump code in binfmt_elf_fdpic.c appears identical to an old
> version of the binfmt_elf.c code. That file appears to have been made with
> the "copy and paste" school of code sharing, of which I am a detractor.
> The ELF core dump code can be shared between those two, and really should
> be. Once that's done, you will want to use the CORE_DUMP_USE_REGSET flavor
> of the code and clean out any old core-related cruft you had in asm/elf.h.
> In the long run, the non-user_regset version of the core dump code will go
> away after every arch has been cleaned up.

cant say we've had anything to do with this ;). nor does Blackfin
support coredumps on FDPIC ELF (and FLAT has never supported
coredumps), and i'm pretty sure we havent looked at anything in gdb
along these lines. no one has ever asked, so we've never looked.

> The "ptrace: Add support for generic PTRACE_GETREGSET/PTRACE_SETREGSET"
> patch making its way through the obstacle course right now will make the
> generic ptrace code use user_regset. That use is conditional on
> CONFIG_HAVE_ARCH_TRACEHOOK and your kernel will stop building if you have
> set that without meeting its requirements.
>
> Moreover, the whole point of CONFIG_HAVE_ARCH_TRACEHOOK is to indicate the
> minimum arch requirements that all future generic code can rely on. If you
> set it without meeting the documented requirements as arch/Kconfig tells
> you to, then your arch will one day be broken by new generic code getting
> merged in.

i dont have a problem with implementing user_regsets ... my point was
more that if there's no code using these interfaces, then i could slap
together a whole lot of crap and never know if it's correct since i
cant test it. Blackfin doesnt currently implement
PTRACE_{G,S}ETREGSET, so even with that patch i dont think i have any
(straight forward) test vectors.

>> i dont see anyone calling syscall_get_arguments() with i!=0, and a few
>> other arches are doing the BUG_ON(i) thing too.
>
> Someone will, and then they will crash. A few others being half-assed is
> no good reason for you to follow suit.

the original reason was like the comment said -- i had no idea what
the "i" was for and the few arches that did implement it didnt exactly
have clear code/documentation. ive added some nice kerneldoc stuff to
the Blackfin version in case anyone looks at this.

>> this is unchanged from the previous Blackfin behavior, and it's how
>> most arches behaved in 2.6.32. but looking in latest mainline, it
>> seems people are changing to:
>> if (test_thread_flag(TIF_SINGLESTEP) || test_thread_flag(TIF_SYSCALL_TRACE))
>> tracehook_report_syscall_exit(regs, 0);
>>
>> so changing Blackfin too should be straightforward i guess
>
> You can't blindly follow another arch. All the details that tell you what
> is the right thing to do here are arch-specific. I see no arch that does
> what you say, and it seems certain they would be wrong if they did.

i missed the passing of TIF_SINGLESTEP in as the second arg. i
thought you were taking issue with the if(...) portions.

the way we do it on Blackfin atm is that syscall_trace_{enter,leave}
are only called when TIF_SYSCALL_TRACE is set (in the low level
assembly), so any checking of thread_flags here is pointless. i guess
the low level code needs updating to check a mask so that we can add
TIF_SYSCALL_TRACEPOINT easier.

> You should always call tracehook_report_syscall_exit() if TIF_SYSCALL_TRACE
> is set. Whether to call it otherwise depends on arch details.

this is all set for us

> On some machines, single-step into a syscall instruction is no different
> from other user instructions, so the normal SIGTRAP will come afterwards
> anyway.
>
> On other machines, entering the kernel for the syscall instruction defeats
> the normal user-mode effects of single-step being enabled. In that event,
> you want to call tracehook_report_syscall_exit() if single-step is enabled.
> You must pass a nonzero second argument if your arch code is not going to
> generate the normal SIGTRAP associated with having single-stepped into the
> syscall instruction.

so tracehook_report_syscall_exit() checking TIF_SINGLESTEP only makes
sense when the arch doesnt support hardware single stepping in user
mode ? the Blackfin processor does support hardware single stepping
(and we utilize it in Linux).

also, in reading the kerneldocs for tracehook_report_syscall_exit(),
it says "an attempted system call". should system calls greater than
NR_syscall (-ENOSYS) also get traced ? we dont currently do this on
the Blackfin port, but going by `strace` differences between my
desktop and the board, i guess the answer is "the Blackfin code is
currently broken".
-mike

2010-02-12 15:25:14

by Oleg Nesterov

[permalink] [raw]

Subject: Re: [PATCH 1/2] Blackfin: initial tracehook support

On 02/11, Mike Frysinger wrote:
>
> On Thu, Feb 11, 2010 at 22:24, Roland McGrath wrote:
>
> > On some machines, single-step into a syscall instruction is no different
> > from other user instructions, so the normal SIGTRAP will come afterwards
> > anyway.
> >
> > On other machines, entering the kernel for the syscall instruction defeats
> > the normal user-mode effects of single-step being enabled. ?In that event,
> > you want to call tracehook_report_syscall_exit() if single-step is enabled.
> > You must pass a nonzero second argument if your arch code is not going to
> > generate the normal SIGTRAP associated with having single-stepped into the
> > syscall instruction.
>
> so tracehook_report_syscall_exit() checking TIF_SINGLESTEP only makes
> sense when the arch doesnt support hardware single stepping in user
> mode ? the Blackfin processor does support hardware single stepping
> (and we utilize it in Linux).

I'd like to know the answer too ;)

But, even if x86 supports hardware single stepping, it does check
TIF_SINGLESTEP and pass it to tracehook_report_syscall_exit(step).

Consider PTRACE_SINGLESTEP which follows the syscall-entry stop.
The tracee gets the trap before return to user-mode. Otherwise,
if we just return with X86_EFLAGS_TF, it gets the trap after the
next instruction after syscall insn. But I don't know whether
blackfin should follow this logic.

> also, in reading the kerneldocs for tracehook_report_syscall_exit(),
> it says "an attempted system call". should system calls greater than
> NR_syscall (-ENOSYS) also get traced ?

I'd say yes, but let's wait for Roland's reply.

Oleg.

2010-02-12 20:45:44

[permalink] [raw]

Subject: Re: [PATCH 1/2] Blackfin: initial tracehook support

> cant say we've had anything to do with this ;). nor does Blackfin
> support coredumps on FDPIC ELF (and FLAT has never supported
> coredumps), and i'm pretty sure we havent looked at anything in gdb
> along these lines. no one has ever asked, so we've never looked.

Ok.

> i dont have a problem with implementing user_regsets ... my point was
> more that if there's no code using these interfaces, then i could slap
> together a whole lot of crap and never know if it's correct since i
> cant test it. Blackfin doesnt currently implement
> PTRACE_{G,S}ETREGSET, so even with that patch i dont think i have any
> (straight forward) test vectors.

The point of PTRACE_{G,S}ETREGSET is that no arch code implements them.
The generic ptrace code does. The patch for that is in tip/master (I think).

Moreover, the usual cleanup is to make your arch_ptrace() use
copy_regset_from_user() and copy_regset_to_user() to implement existing
calls ike PTRACE_GETREGS. That way, existing ptrace users (strace, gdb)
become tests of the user_regset paths (some of them).

> i missed the passing of TIF_SINGLESTEP in as the second arg. i
> thought you were taking issue with the if(...) portions.

I was trying to explain how you apply your arch knowledge to decide what is
exactly right for your arch code.

> the way we do it on Blackfin atm is that syscall_trace_{enter,leave}
> are only called when TIF_SYSCALL_TRACE is set (in the low level
> assembly), so any checking of thread_flags here is pointless. i guess
> the low level code needs updating to check a mask so that we can add
> TIF_SYSCALL_TRACEPOINT easier.

What happens today when PTRACE_SINGLESTEP is used with the PC sitting at a
syscall instruction? If that gets a single-step report (SIGTRAP) after the
syscall is done, with the PC sitting at the instruction immediately
following the syscall instruction, then you are already doing the right
thing. On some other machines, making that happen involves arch assembly
code making sure it gets to its syscall_trace_leave() for this case.

> so tracehook_report_syscall_exit() checking TIF_SINGLESTEP only makes
> sense when the arch doesnt support hardware single stepping in user
> mode ? the Blackfin processor does support hardware single stepping
> (and we utilize it in Linux).

No, that is not what I said at all. It is necessary if and only if the way
that the hardware single-step interacts with the syscall entry/exit paths
on your machine means that the completion of the user-mode syscall
instruction does not result in a single-step trap like any other single
user-mode instruction would. This is the case on machines such as x86 and
powerpc (I don't know others in detail). There, the hardware single-step
feature works such that the real hardware trap either comes in kernel mode
right at syscall entry, or doesn't come at all. Hence, the arch code has
to notice when single-step was enabled at syscall entry time and simulate
the hardware trap before returning to user mode.

> also, in reading the kerneldocs for tracehook_report_syscall_exit(),
> it says "an attempted system call". should system calls greater than
> NR_syscall (-ENOSYS) also get traced ? we dont currently do this on
> the Blackfin port, but going by `strace` differences between my
> desktop and the board, i guess the answer is "the Blackfin code is
> currently broken".

Yes. It's any syscall attempt. Inside tracehook_report_syscall_entry(),
all the registers can be changed (via user_regset, i.e. ptrace) so that
what starts as an entry with a bogus syscall number becomes an entry with a
different syscall number and/or arguments. So you can't decide before
tracehook_report_syscall_entry() whether it is "real" or not. For every
tracehook_report_syscall_entry() call, there must be a corresponding
tracehook_report_syscall_exit() call (unless TIF_SYSCALL_TRACE was cleared
in the interim). At that exit point, the registers can again be changed.

For example, an "expected" use (that some things do today via ptrace) is to
catch the entry, abort the syscall by changing the syscall number register
to something bogus like -1, resume so it diagnoses -ENOSYS and gets to
syscall exit, then catch the exit and reset the return value registers to
pretend some particular syscall results happened.

Thanks,
Roland

2010-02-13 09:41:57

[permalink] [raw]

Subject: Re: [PATCH 1/2] Blackfin: initial tracehook support

On Fri, Feb 12, 2010 at 15:44, Roland McGrath wrote:
> Moreover, the usual cleanup is to make your arch_ptrace() use
> copy_regset_from_user() and copy_regset_to_user() to implement existing
> calls ike PTRACE_GETREGS. That way, existing ptrace users (strace, gdb)
> become tests of the user_regset paths (some of them).

OK, this should be doable. are there any guidelines for what should
be in a specific regset ? the Blackfin processor does not have a FPU,
so the only set i have defined atm is the "general" set and that is
exactly the same as the current set of ptrace registers. this is also
what the current PTRACE_{G,S}ETREGS operates on (struct pt_regs).

there are more pseudo regs as required by FDPIC, but they arent in the
pt_regs layout ...

> What happens today when PTRACE_SINGLESTEP is used with the PC sitting at a
> syscall instruction? If that gets a single-step report (SIGTRAP) after the
> syscall is done, with the PC sitting at the instruction immediately
> following the syscall instruction, then you are already doing the right
> thing. On some other machines, making that happen involves arch assembly
> code making sure it gets to its syscall_trace_leave() for this case.

i believe the single step exception is taken first and then the
syscall exception, but i'll have to do some hardware tests on the
hardware to confirm

>> also, in reading the kerneldocs for tracehook_report_syscall_exit(),
>> it says "an attempted system call". should system calls greater than
>> NR_syscall (-ENOSYS) also get traced ? we dont currently do this on
>> the Blackfin port, but going by `strace` differences between my
>> desktop and the board, i guess the answer is "the Blackfin code is
>> currently broken".
>
> Yes. It's any syscall attempt. Inside tracehook_report_syscall_entry(),
> all the registers can be changed (via user_regset, i.e. ptrace) so that
> what starts as an entry with a bogus syscall number becomes an entry with a
> different syscall number and/or arguments. So you can't decide before
> tracehook_report_syscall_entry() whether it is "real" or not. For every
> tracehook_report_syscall_entry() call, there must be a corresponding
> tracehook_report_syscall_exit() call (unless TIF_SYSCALL_TRACE was cleared
> in the interim). At that exit point, the registers can again be changed.
>
> For example, an "expected" use (that some things do today via ptrace) is to
> catch the entry, abort the syscall by changing the syscall number register
> to something bogus like -1, resume so it diagnoses -ENOSYS and gets to
> syscall exit, then catch the exit and reset the return value registers to
> pretend some particular syscall results happened.

i fixed the Blackfin code to do NR checking after tracing and now
`strace` shows the bad syscall

as for register munging, i didnt realize this was accepted practice
and something that should actively be supported. i had been toying
around locally with optimizing some of the syscall paths by breaking
this behavior, so i'll add some comments to prevent any further
mucking here.

the Blackfin code when traced now does:
call tracehook_report_syscall_entry(regs)
reload syscall NR and all 6 args from regs
if syscall NR is valid, call it
if syscall NR is invalid, set return value to -ENOSYS
store return value into regs
call tracehook_report_syscall_exit(regs)
-mike

2010-02-15 07:37:19

[permalink] [raw]

Subject: Re: [PATCH 1/2] Blackfin: initial tracehook support

On Sat, Feb 13, 2010 at 04:41, Mike Frysinger wrote:
> On Fri, Feb 12, 2010 at 15:44, Roland McGrath wrote:
>> Moreover, the usual cleanup is to make your arch_ptrace() use
>> copy_regset_from_user() and copy_regset_to_user() to implement existing
>> calls ike PTRACE_GETREGS. That way, existing ptrace users (strace, gdb)
>> become tests of the user_regset paths (some of them).

unfortunately the Blackfin ports of both gdb and strace do not use the
PTRAGE_{G,S}ETREGS interfaces. so i had to port both in order to test
out the new code.

> OK, this should be doable. are there any guidelines for what should
> be in a specific regset ? the Blackfin processor does not have a FPU,
> so the only set i have defined atm is the "general" set and that is
> exactly the same as the current set of ptrace registers. this is also
> what the current PTRACE_{G,S}ETREGS operates on (struct pt_regs).

going by the gdb code, all i really need to worry about is the
"general" set and have that be the same as pt_regs today

i have one or two small things to check out, but i think we should be
all set now thanks to your help
-mike

2010-02-15 20:08:13