* [PATCH 2/14] alpha: use generic ptrace_resume code
2010-02-02 18:57 [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h Christoph Hellwig
@ 2010-02-02 18:58 ` Christoph Hellwig
2010-02-03 4:35 ` Matt Turner
2010-02-02 18:58 ` [PATCH 3/14] arm: " Christoph Hellwig
` (14 subsequent siblings)
15 siblings, 1 reply; 46+ messages in thread
From: Christoph Hellwig @ 2010-02-02 18:58 UTC (permalink / raw)
To: roland, oleg, akpm, mattst88, ink, rth; +Cc: linux-kernel, linux-arch
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 <hch@lst.de>
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)
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 2/14] alpha: use generic ptrace_resume code
2010-02-02 18:58 ` [PATCH 2/14] alpha: use generic ptrace_resume code Christoph Hellwig
@ 2010-02-03 4:35 ` Matt Turner
0 siblings, 0 replies; 46+ messages in thread
From: Matt Turner @ 2010-02-03 4:35 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: roland, oleg, akpm, ink, rth, linux-kernel, linux-arch
On Tue, Feb 2, 2010 at 1:58 PM, Christoph Hellwig <hch@lst.de> 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 <hch@lst.de>
>
> 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 <mattst88@gmail.com>
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 3/14] arm: use generic ptrace_resume code
2010-02-02 18:57 [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h Christoph Hellwig
2010-02-02 18:58 ` [PATCH 2/14] alpha: use generic ptrace_resume code Christoph Hellwig
@ 2010-02-02 18:58 ` Christoph Hellwig
2010-02-02 18:58 ` [PATCH 4/14] avr32: " Christoph Hellwig
` (13 subsequent siblings)
15 siblings, 0 replies; 46+ messages in thread
From: Christoph Hellwig @ 2010-02-02 18:58 UTC (permalink / raw)
To: roland, oleg, akpm, linux; +Cc: linux-kernel, linux-arch
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 <hch@lst.de>
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)
^ permalink raw reply [flat|nested] 46+ messages in thread* [PATCH 4/14] avr32: use generic ptrace_resume code
2010-02-02 18:57 [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h Christoph Hellwig
2010-02-02 18:58 ` [PATCH 2/14] alpha: use generic ptrace_resume code Christoph Hellwig
2010-02-02 18:58 ` [PATCH 3/14] arm: " Christoph Hellwig
@ 2010-02-02 18:58 ` Christoph Hellwig
2010-02-03 3:17 ` Haavard Skinnemoen
2010-02-02 18:59 ` [PATCH 5/14] blackfin: " Christoph Hellwig
` (12 subsequent siblings)
15 siblings, 1 reply; 46+ messages in thread
From: Christoph Hellwig @ 2010-02-02 18:58 UTC (permalink / raw)
To: roland, oleg, akpm, hskinnemoen; +Cc: linux-kernel, linux-arch
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 <hch@lst.de>
Index: linux-2.6/arch/avr32/include/asm/ptrace.h
===================================================================
--- linux-2.6.orig/arch/avr32/include/asm/ptrace.h 2010-02-02 11:14:51.257023230 +0100
+++ linux-2.6/arch/avr32/include/asm/ptrace.h 2010-02-02 11:15:00.220025002 +0100
@@ -124,6 +124,8 @@ struct pt_regs {
#include <asm/ocd.h>
+#define arch_has_single_step() (1)
+
#define arch_ptrace_attach(child) ocd_enable(child)
#define user_mode(regs) (((regs)->sr & MODE_MASK) == MODE_USER)
Index: linux-2.6/arch/avr32/kernel/ptrace.c
===================================================================
--- linux-2.6.orig/arch/avr32/kernel/ptrace.c 2010-02-02 11:14:51.269004225 +0100
+++ linux-2.6/arch/avr32/kernel/ptrace.c 2010-02-02 11:15:40.045282945 +0100
@@ -28,9 +28,9 @@ static struct pt_regs *get_user_regs(str
THREAD_SIZE - sizeof(struct pt_regs));
}
-static void ptrace_single_step(struct task_struct *tsk)
+static void user_enable_single_step(struct task_struct *tsk)
{
- pr_debug("ptrace_single_step: pid=%u, PC=0x%08lx, SR=0x%08lx\n",
+ pr_debug("user_enable_single_step: pid=%u, PC=0x%08lx, SR=0x%08lx\n",
tsk->pid, task_pt_regs(tsk)->pc, task_pt_regs(tsk)->sr);
/*
@@ -49,6 +49,11 @@ static void ptrace_single_step(struct ta
set_tsk_thread_flag(tsk, TIF_SINGLE_STEP);
}
+void user_disable_single_step(struct task_struct *child)
+{
+ /* XXX(hch): a no-op here seems wrong.. */
+}
+
/*
* Called by kernel/ptrace.c when detaching
*
@@ -156,50 +161,6 @@ long arch_ptrace(struct task_struct *chi
ret = ptrace_write_user(child, addr, data);
break;
- /* continue and stop at next (return from) syscall */
- case PTRACE_SYSCALL:
- /* restart after signal */
- 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;
- /* XXX: Are we sure no breakpoints are active here? */
- 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;
- wake_up_process(child);
- break;
-
- /*
- * execute single instruction.
- */
- case PTRACE_SINGLESTEP:
- ret = -EIO;
- if (!valid_signal(data))
- break;
- clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
- ptrace_single_step(child);
- child->exit_code = data;
- wake_up_process(child);
- ret = 0;
- break;
-
case PTRACE_GETREGS:
ret = ptrace_getregs(child, (void __user *)data);
break;
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 4/14] avr32: use generic ptrace_resume code
2010-02-02 18:58 ` [PATCH 4/14] avr32: " Christoph Hellwig
@ 2010-02-03 3:17 ` Haavard Skinnemoen
2010-02-03 8:36 ` Christoph Hellwig
0 siblings, 1 reply; 46+ messages in thread
From: Haavard Skinnemoen @ 2010-02-03 3:17 UTC (permalink / raw)
To: Christoph Hellwig
Cc: roland, oleg, akpm, hskinnemoen, linux-kernel, linux-arch
On Tue, 2 Feb 2010 19:58:57 +0100
Christoph Hellwig <hch@lst.de> 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 <hch@lst.de>
Acked-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
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
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 4/14] avr32: use generic ptrace_resume code
2010-02-03 3:17 ` Haavard Skinnemoen
@ 2010-02-03 8:36 ` Christoph Hellwig
2010-02-03 19:22 ` Oleg Nesterov
0 siblings, 1 reply; 46+ messages in thread
From: Christoph Hellwig @ 2010-02-03 8:36 UTC (permalink / raw)
To: Haavard Skinnemoen
Cc: Christoph Hellwig, roland, oleg, akpm, hskinnemoen, linux-kernel,
linux-arch
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.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 4/14] avr32: use generic ptrace_resume code
2010-02-03 8:36 ` Christoph Hellwig
@ 2010-02-03 19:22 ` Oleg Nesterov
2010-02-03 19:28 ` Christoph Hellwig
0 siblings, 1 reply; 46+ messages in thread
From: Oleg Nesterov @ 2010-02-03 19:22 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Haavard Skinnemoen, roland, akpm, hskinnemoen, linux-kernel, linux-arch
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.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 4/14] avr32: use generic ptrace_resume code
2010-02-03 19:22 ` Oleg Nesterov
@ 2010-02-03 19:28 ` Christoph Hellwig
0 siblings, 0 replies; 46+ messages in thread
From: Christoph Hellwig @ 2010-02-03 19:28 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Christoph Hellwig, Haavard Skinnemoen, roland, akpm, hskinnemoen,
linux-kernel, linux-arch
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.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 5/14] blackfin: use generic ptrace_resume code
2010-02-02 18:57 [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h Christoph Hellwig
` (2 preceding siblings ...)
2010-02-02 18:58 ` [PATCH 4/14] avr32: " Christoph Hellwig
@ 2010-02-02 18:59 ` Christoph Hellwig
2010-02-02 20:29 ` Mike Frysinger
` (3 more replies)
2010-02-02 18:59 ` [PATCH 6/14] h8300: " Christoph Hellwig
` (11 subsequent siblings)
15 siblings, 4 replies; 46+ messages in thread
From: Christoph Hellwig @ 2010-02-02 18:59 UTC (permalink / raw)
To: roland, oleg, akpm, vapier; +Cc: linux-kernel, linux-arch
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 <hch@lst.de>
Index: linux-2.6/arch/blackfin/kernel/ptrace.c
===================================================================
--- linux-2.6.orig/arch/blackfin/kernel/ptrace.c 2010-02-02 11:00:49.917253804 +0100
+++ linux-2.6/arch/blackfin/kernel/ptrace.c 2010-02-02 11:18:07.673254085 +0100
@@ -160,7 +160,15 @@ static inline int is_user_addr_valid(str
return -EIO;
}
-void ptrace_enable(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. */
+ tmp = get_reg(child, PT_SYSCFG) & ~TRACE_BITS;
+ put_reg(child, PT_SYSCFG, tmp);
+}
+
+void user_enable_single_step(struct task_struct *child)
{
unsigned long tmp;
tmp = get_reg(child, PT_SYSCFG) | (TRACE_BITS);
@@ -174,10 +182,7 @@ void ptrace_enable(struct task_struct *c
*/
void ptrace_disable(struct task_struct *child)
{
- unsigned long tmp;
- /* make sure the single step bit is not set. */
- tmp = get_reg(child, PT_SYSCFG) & ~TRACE_BITS;
- put_reg(child, PT_SYSCFG, tmp);
+ user_disable_single_step(child);
}
long arch_ptrace(struct task_struct *child, long request, long addr, long data)
@@ -354,50 +359,6 @@ long arch_ptrace(struct task_struct *chi
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;
- ptrace_disable(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;
- ptrace_disable(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);
- ptrace_enable(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);
Index: linux-2.6/arch/blackfin/include/asm/ptrace.h
===================================================================
--- linux-2.6.orig/arch/blackfin/include/asm/ptrace.h 2010-02-02 11:16:35.673022299 +0100
+++ linux-2.6/arch/blackfin/include/asm/ptrace.h 2010-02-02 11:18:14.244006444 +0100
@@ -97,6 +97,8 @@ struct pt_regs {
#ifdef __KERNEL__
+#define arch_has_single_step() (1)
+
/* user_mode returns true if only one bit is set in IPEND, other than the
master interrupt enable. */
#define user_mode(regs) (!(((regs)->ipend & ~0x10) & (((regs)->ipend & ~0x10) - 1)))
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 5/14] blackfin: use generic ptrace_resume code
2010-02-02 18:59 ` [PATCH 5/14] blackfin: " Christoph Hellwig
@ 2010-02-02 20:29 ` Mike Frysinger
2010-02-03 19:36 ` Mike Frysinger
2010-02-11 9:43 ` [PATCH 0/2] Blackfin: " Mike Frysinger
` (2 subsequent siblings)
3 siblings, 1 reply; 46+ messages in thread
From: Mike Frysinger @ 2010-02-02 20:29 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: roland, oleg, akpm, vapier, linux-kernel, linux-arch
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
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 5/14] blackfin: use generic ptrace_resume code
2010-02-02 20:29 ` Mike Frysinger
@ 2010-02-03 19:36 ` Mike Frysinger
2010-02-03 19:42 ` Christoph Hellwig
0 siblings, 1 reply; 46+ messages in thread
From: Mike Frysinger @ 2010-02-03 19:36 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: roland, oleg, akpm, linux-kernel, linux-arch
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
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/14] blackfin: use generic ptrace_resume code
2010-02-03 19:36 ` Mike Frysinger
@ 2010-02-03 19:42 ` Christoph Hellwig
0 siblings, 0 replies; 46+ messages in thread
From: Christoph Hellwig @ 2010-02-03 19:42 UTC (permalink / raw)
To: Mike Frysinger
Cc: Christoph Hellwig, roland, oleg, akpm, linux-kernel, linux-arch
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.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 0/2] Blackfin: use generic ptrace_resume code
2010-02-02 18:59 ` [PATCH 5/14] blackfin: " Christoph Hellwig
2010-02-02 20:29 ` Mike Frysinger
@ 2010-02-11 9:43 ` Mike Frysinger
2010-02-11 9:43 ` [PATCH 1/2] Blackfin: initial tracehook support Mike Frysinger
2010-02-11 9:43 ` Mike Frysinger
3 siblings, 0 replies; 46+ messages in thread
From: Mike Frysinger @ 2010-02-11 9:43 UTC (permalink / raw)
To: Christoph Hellwig
Cc: roland, oleg, Andrew Morton, linux-kernel, linux-arch,
uclinux-dist-devel
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).
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 1/2] Blackfin: initial tracehook support
2010-02-02 18:59 ` [PATCH 5/14] blackfin: " Christoph Hellwig
2010-02-02 20:29 ` Mike Frysinger
2010-02-11 9:43 ` [PATCH 0/2] Blackfin: " Mike Frysinger
@ 2010-02-11 9:43 ` Mike Frysinger
2010-02-11 20:46 ` Roland McGrath
2010-02-11 9:43 ` Mike Frysinger
3 siblings, 1 reply; 46+ messages in thread
From: Mike Frysinger @ 2010-02-11 9:43 UTC (permalink / raw)
To: Christoph Hellwig
Cc: roland, oleg, Andrew Morton, linux-kernel, linux-arch,
uclinux-dist-devel
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
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(®s, get_user_regs(tsk), sizeof(regs));
+ memcpy(®s, task_pt_regs(tsk), sizeof(regs));
regs.usp = tsk->thread.usp;
return copy_to_user(uregs, ®s, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH 1/2] Blackfin: initial tracehook support
2010-02-11 9:43 ` [PATCH 1/2] Blackfin: initial tracehook support Mike Frysinger
@ 2010-02-11 20:46 ` Roland McGrath
2010-02-11 23:54 ` Mike Frysinger
0 siblings, 1 reply; 46+ messages in thread
From: Roland McGrath @ 2010-02-11 20:46 UTC (permalink / raw)
To: Mike Frysinger
Cc: Christoph Hellwig, oleg, Andrew Morton, linux-kernel, linux-arch,
uclinux-dist-devel
> 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
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 1/2] Blackfin: initial tracehook support
2010-02-11 20:46 ` Roland McGrath
@ 2010-02-11 23:54 ` Mike Frysinger
2010-02-12 3:24 ` Roland McGrath
0 siblings, 1 reply; 46+ messages in thread
From: Mike Frysinger @ 2010-02-11 23:54 UTC (permalink / raw)
To: Roland McGrath
Cc: Christoph Hellwig, oleg, Andrew Morton, linux-kernel, linux-arch,
uclinux-dist-devel
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
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 1/2] Blackfin: initial tracehook support
2010-02-11 23:54 ` Mike Frysinger
@ 2010-02-12 3:24 ` Roland McGrath
2010-02-12 4:33 ` Mike Frysinger
0 siblings, 1 reply; 46+ messages in thread
From: Roland McGrath @ 2010-02-12 3:24 UTC (permalink / raw)
To: Mike Frysinger
Cc: Christoph Hellwig, oleg, Andrew Morton, linux-kernel, linux-arch,
uclinux-dist-devel
> 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
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 1/2] Blackfin: initial tracehook support
2010-02-12 3:24 ` Roland McGrath
@ 2010-02-12 4:33 ` Mike Frysinger
2010-02-12 15:24 ` Oleg Nesterov
2010-02-12 20:44 ` Roland McGrath
0 siblings, 2 replies; 46+ messages in thread
From: Mike Frysinger @ 2010-02-12 4:33 UTC (permalink / raw)
To: Roland McGrath
Cc: Christoph Hellwig, oleg, Andrew Morton, linux-kernel, linux-arch,
uclinux-dist-devel
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
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 1/2] Blackfin: initial tracehook support
2010-02-12 4:33 ` Mike Frysinger
@ 2010-02-12 15:24 ` Oleg Nesterov
2010-02-12 20:44 ` Roland McGrath
1 sibling, 0 replies; 46+ messages in thread
From: Oleg Nesterov @ 2010-02-12 15:24 UTC (permalink / raw)
To: Mike Frysinger
Cc: Roland McGrath, Christoph Hellwig, Andrew Morton, linux-kernel,
linux-arch, uclinux-dist-devel
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.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 1/2] Blackfin: initial tracehook support
@ 2010-02-12 20:44 ` Roland McGrath
0 siblings, 0 replies; 46+ messages in thread
From: Roland McGrath @ 2010-02-12 20:44 UTC (permalink / raw)
To: Mike Frysinger
Cc: Christoph Hellwig, oleg, Andrew Morton, linux-kernel, linux-arch,
uclinux-dist-devel
> 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
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 1/2] Blackfin: initial tracehook support
@ 2010-02-12 20:44 ` Roland McGrath
0 siblings, 0 replies; 46+ messages in thread
From: Roland McGrath @ 2010-02-12 20:44 UTC (permalink / raw)
To: Mike Frysinger
Cc: linux-arch-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, oleg-H+wXaHxf7aLQT0dZR+AlfA,
uclinux-dist-devel-ZG0+EudsQA8dtHy/vicBwGD2FQJk+8+b,
Andrew Morton, Christoph Hellwig
> 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
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 1/2] Blackfin: initial tracehook support
2010-02-12 20:44 ` Roland McGrath
(?)
@ 2010-02-13 9:41 ` Mike Frysinger
2010-02-15 7:36 ` Mike Frysinger
2010-02-15 20:07 ` Roland McGrath
-1 siblings, 2 replies; 46+ messages in thread
From: Mike Frysinger @ 2010-02-13 9:41 UTC (permalink / raw)
To: Roland McGrath
Cc: Christoph Hellwig, oleg, Andrew Morton, linux-kernel, linux-arch,
uclinux-dist-devel
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
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 1/2] Blackfin: initial tracehook support
2010-02-13 9:41 ` Mike Frysinger
@ 2010-02-15 7:36 ` Mike Frysinger
2010-02-15 20:07 ` Roland McGrath
1 sibling, 0 replies; 46+ messages in thread
From: Mike Frysinger @ 2010-02-15 7:36 UTC (permalink / raw)
To: Roland McGrath
Cc: Christoph Hellwig, oleg, Andrew Morton, linux-kernel, linux-arch,
uclinux-dist-devel
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
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 1/2] Blackfin: initial tracehook support
2010-02-13 9:41 ` Mike Frysinger
2010-02-15 7:36 ` Mike Frysinger
@ 2010-02-15 20:07 ` Roland McGrath
1 sibling, 0 replies; 46+ messages in thread
From: Roland McGrath @ 2010-02-15 20:07 UTC (permalink / raw)
To: Mike Frysinger
Cc: Christoph Hellwig, oleg, Andrew Morton, linux-kernel, linux-arch,
uclinux-dist-devel
> 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).
For most machines, the decision was already made implicitly in the formats
used in ELF core dumps (e.g. elf_gregset_t). If you've never had ELF core
dumps, then you are not constrained by any existing userland compatibility.
The fundamental guideline is that you should use the "natural" layouts and
divisions for your arch. Usually what that means is fairly clear to people
well-versed in the particular arch. When you have a PTRACE_GETREGS format
and there is nothing particularly wrong with that layout choice, then that
is the obvious thing to use as the user_regset layout for NT_PRSTATUS.
> there are more pseudo regs as required by FDPIC, but they arent in the
> pt_regs layout ...
IMHO these do not belong in a regset at all. Like the name says, the
regset is for registers. If something is not really an arch-specific
detail, I don't think it belongs in user_regset.
> 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
The ptrace-tests suite (see http://sourceware.org/systemtap/wiki/utrace/tests)
has some tests for various corners of these semantics. You'll have to port
some of the asm bits to blackfin, but that is probably easier and more
complete than what you'd try yourself from scratch.
> i fixed the Blackfin code to do NR checking after tracing and now
> `strace` shows the bad syscall
Good!
> as for register munging, i didnt realize this was accepted practice
> and something that should actively be supported.
Indeed. Try "strace -f" and see that its fork/clone tracing code relies
on fiddling syscall argument registers.
This stop (i.e. inside tracehook_report_syscall_entry) is the one and only
place where syscall_set_arguments() can validly be used.
Moreover, the fundamental rule is that at every tracehook_report_* call
site (aka ptrace stop), full access (including modification) via
user_regset interfaces (aka ptrace) should work and have the same effect as
if userland had set those register values normally before entering the
kernel (or just after exiting it, depending on the site in question).
> 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.
Other machines do have such fast paths for syscalls.
They take the slow paths when TIF_SYSCALL_TRACE is set.
Note that you can still have fast paths when TIF_SYSCALL_AUDIT
is set--those don't have to permit modification.
Note also that syscall_get_arguments() works anywhere.
> 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)
That's correct. Also do whatever reloading is required after so that the
return value register and other registers seen in userland have whatever
values may have been set inside tracehook_report_syscall_exit().
Thanks,
Roland
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 2/2] Blackfin: use generic ptrace_resume code
2010-02-02 18:59 ` [PATCH 5/14] blackfin: " Christoph Hellwig
2010-02-02 20:29 ` Mike Frysinger
@ 2010-02-11 9:43 ` Mike Frysinger
2010-02-11 9:43 ` [PATCH 1/2] Blackfin: initial tracehook support Mike Frysinger
2010-02-11 9:43 ` Mike Frysinger
3 siblings, 0 replies; 46+ messages in thread
From: Mike Frysinger @ 2010-02-11 9:43 UTC (permalink / raw)
To: Christoph Hellwig
Cc: roland, oleg, Andrew Morton, linux-kernel, linux-arch,
uclinux-dist-devel, Christoph Hellwig
From: Christoph Hellwig <hch@lst.de>
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 <hch@lst.de>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
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
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH 2/2] Blackfin: use generic ptrace_resume code
@ 2010-02-11 9:43 ` Mike Frysinger
0 siblings, 0 replies; 46+ messages in thread
From: Mike Frysinger @ 2010-02-11 9:43 UTC (permalink / raw)
To: Christoph Hellwig
Cc: roland, oleg, Andrew Morton, linux-kernel, linux-arch,
uclinux-dist-devel
From: Christoph Hellwig <hch@lst.de>
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 <hch@lst.de>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
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
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH 2/2] Blackfin: use generic ptrace_resume code
@ 2010-02-11 9:43 ` Mike Frysinger
0 siblings, 0 replies; 46+ messages in thread
From: Mike Frysinger @ 2010-02-11 9:43 UTC (permalink / raw)
Cc: roland, oleg, Andrew Morton, linux-kernel, linux-arch,
uclinux-dist-devel, Christoph Hellwig
From: Christoph Hellwig <hch@lst.de>
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 <hch@lst.de>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 6/14] h8300: use generic ptrace_resume code
2010-02-02 18:57 [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h Christoph Hellwig
` (3 preceding siblings ...)
2010-02-02 18:59 ` [PATCH 5/14] blackfin: " Christoph Hellwig
@ 2010-02-02 18:59 ` Christoph Hellwig
2010-02-02 18:59 ` [PATCH 7/14] m68knommu: " Christoph Hellwig
` (10 subsequent siblings)
15 siblings, 0 replies; 46+ messages in thread
From: Christoph Hellwig @ 2010-02-02 18:59 UTC (permalink / raw)
To: roland, oleg, akpm, ysato; +Cc: linux-kernel, linux-arch
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 <hch@lst.de>
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)
^ permalink raw reply [flat|nested] 46+ messages in thread* [PATCH 7/14] m68knommu: use generic ptrace_resume code
2010-02-02 18:57 [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h Christoph Hellwig
` (4 preceding siblings ...)
2010-02-02 18:59 ` [PATCH 6/14] h8300: " Christoph Hellwig
@ 2010-02-02 18:59 ` Christoph Hellwig
2010-02-03 6:54 ` Greg Ungerer
2010-02-02 18:59 ` [PATCH 8/14] microblaze: " Christoph Hellwig
` (9 subsequent siblings)
15 siblings, 1 reply; 46+ messages in thread
From: Christoph Hellwig @ 2010-02-02 18:59 UTC (permalink / raw)
To: roland, oleg, akpm, gerg; +Cc: linux-kernel, linux-arch
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 <hch@lst.de>
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;
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 7/14] m68knommu: use generic ptrace_resume code
2010-02-02 18:59 ` [PATCH 7/14] m68knommu: " Christoph Hellwig
@ 2010-02-03 6:54 ` Greg Ungerer
0 siblings, 0 replies; 46+ messages in thread
From: Greg Ungerer @ 2010-02-03 6:54 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: roland, oleg, akpm, linux-kernel, linux-arch
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 <hch@lst.de>
Acked-by: Greg Ungerer <gerg@uclinux.org>
------------------------------------------------------------------------
Greg Ungerer -- Principal Engineer EMAIL: gerg@snapgear.com
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
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 8/14] microblaze: use generic ptrace_resume code
2010-02-02 18:57 [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h Christoph Hellwig
` (5 preceding siblings ...)
2010-02-02 18:59 ` [PATCH 7/14] m68knommu: " Christoph Hellwig
@ 2010-02-02 18:59 ` Christoph Hellwig
2010-02-03 11:00 ` Michal Simek
2010-02-02 18:59 ` [PATCH 9/14] mips: " Christoph Hellwig
` (8 subsequent siblings)
15 siblings, 1 reply; 46+ messages in thread
From: Christoph Hellwig @ 2010-02-02 18:59 UTC (permalink / raw)
To: roland, oleg, akpm, monstr; +Cc: linux-kernel, linux-arch
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 <hch@lst.de>
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);
}
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 8/14] microblaze: use generic ptrace_resume code
2010-02-02 18:59 ` [PATCH 8/14] microblaze: " Christoph Hellwig
@ 2010-02-03 11:00 ` Michal Simek
0 siblings, 0 replies; 46+ messages in thread
From: Michal Simek @ 2010-02-03 11:00 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: roland, oleg, akpm, linux-kernel, linux-arch
Acked-by: Michal Simek <monstr@monstr.eu>
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 <hch@lst.de>
>
> 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: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 9/14] mips: use generic ptrace_resume code
2010-02-02 18:57 [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h Christoph Hellwig
` (6 preceding siblings ...)
2010-02-02 18:59 ` [PATCH 8/14] microblaze: " Christoph Hellwig
@ 2010-02-02 18:59 ` Christoph Hellwig
2010-02-02 19:19 ` Ralf Baechle
2010-02-02 19:00 ` [PATCH 10/14] um: " Christoph Hellwig
` (7 subsequent siblings)
15 siblings, 1 reply; 46+ messages in thread
From: Christoph Hellwig @ 2010-02-02 18:59 UTC (permalink / raw)
To: roland, oleg, akpm, ralf; +Cc: linux-kernel, linux-arch
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 <hch@lst.de>
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);
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 9/14] mips: use generic ptrace_resume code
2010-02-02 18:59 ` [PATCH 9/14] mips: " Christoph Hellwig
@ 2010-02-02 19:19 ` Ralf Baechle
0 siblings, 0 replies; 46+ messages in thread
From: Ralf Baechle @ 2010-02-02 19:19 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: roland, oleg, akpm, linux-kernel, linux-arch
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 <hch@lst.de>
Acked-by: Ralf Baechle <ralf@linux-mips.org>
Ralf
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 10/14] um: use generic ptrace_resume code
2010-02-02 18:57 [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h Christoph Hellwig
` (7 preceding siblings ...)
2010-02-02 18:59 ` [PATCH 9/14] mips: " Christoph Hellwig
@ 2010-02-02 19:00 ` Christoph Hellwig
2010-02-02 19:00 ` [PATCH 11/14] xtensa: " Christoph Hellwig
` (6 subsequent siblings)
15 siblings, 0 replies; 46+ messages in thread
From: Christoph Hellwig @ 2010-02-02 19:00 UTC (permalink / raw)
To: roland, oleg, akpm, jdike; +Cc: linux-kernel, linux-arch
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.
XXX: I'm not sure arch_has_single_step() is placed in the exactly correct
location, please verify in which of the ptrace headers it should really be.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Index: linux-2.6/arch/um/kernel/ptrace.c
===================================================================
--- linux-2.6.orig/arch/um/kernel/ptrace.c 2010-02-02 11:00:50.048254427 +0100
+++ linux-2.6/arch/um/kernel/ptrace.c 2010-02-02 11:27:05.412275969 +0100
@@ -12,16 +12,25 @@
#endif
#include "skas_ptrace.h"
-static inline void set_singlestepping(struct task_struct *child, int on)
+
+
+void user_enable_single_step(struct task_struct *child)
{
- if (on)
- child->ptrace |= PT_DTRACE;
- else
- child->ptrace &= ~PT_DTRACE;
+ child->ptrace |= PT_DTRACE;
child->thread.singlestep_syscall = 0;
#ifdef SUBARCH_SET_SINGLESTEPPING
- SUBARCH_SET_SINGLESTEPPING(child, on);
+ SUBARCH_SET_SINGLESTEPPING(child, 1);
+#endif
+}
+
+void user_disable_single_step(struct task_struct *child)
+{
+ child->ptrace &= ~PT_DTRACE;
+ child->thread.singlestep_syscall = 0;
+
+#ifdef SUBARCH_SET_SINGLESTEPPING
+ SUBARCH_SET_SINGLESTEPPING(child, 0);
#endif
}
@@ -30,7 +39,7 @@ static inline void set_singlestepping(st
*/
void ptrace_disable(struct task_struct *child)
{
- set_singlestepping(child,0);
+ user_disable_single_step(child);
}
extern int peek_user(struct task_struct * child, long addr, long data);
@@ -57,53 +66,6 @@ long arch_ptrace(struct task_struct *chi
ret = -EIO;
break;
- /* continue and stop at next (return from) syscall */
- case PTRACE_SYSCALL:
- /* restart after signal. */
- case PTRACE_CONT: {
- ret = -EIO;
- if (!valid_signal(data))
- break;
-
- set_singlestepping(child, 0);
- 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;
-
- set_singlestepping(child, 0);
- child->exit_code = SIGKILL;
- 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);
- set_singlestepping(child, 1);
- child->exit_code = data;
- /* give it a chance to run. */
- wake_up_process(child);
- ret = 0;
- break;
- }
-
#ifdef PTRACE_GETREGS
case PTRACE_GETREGS: { /* Get all gp regs from the child. */
if (!access_ok(VERIFY_WRITE, p, MAX_REG_OFFSET)) {
Index: linux-2.6/arch/um/include/asm/ptrace-generic.h
===================================================================
--- linux-2.6.orig/arch/um/include/asm/ptrace-generic.h 2010-02-02 11:27:09.196004106 +0100
+++ linux-2.6/arch/um/include/asm/ptrace-generic.h 2010-02-02 11:28:57.615023962 +0100
@@ -16,6 +16,8 @@ struct pt_regs {
struct uml_pt_regs regs;
};
+#define arch_has_single_step() (1)
+
#define EMPTY_REGS { .regs = EMPTY_UML_PT_REGS }
#define PT_REGS_IP(r) UPT_IP(&(r)->regs)
^ permalink raw reply [flat|nested] 46+ messages in thread* [PATCH 11/14] xtensa: use generic ptrace_resume code
2010-02-02 18:57 [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h Christoph Hellwig
` (8 preceding siblings ...)
2010-02-02 19:00 ` [PATCH 10/14] um: " Christoph Hellwig
@ 2010-02-02 19:00 ` Christoph Hellwig
2010-02-02 19:00 ` [PATCH 12/14] cris arch-v10: " Christoph Hellwig
` (5 subsequent siblings)
15 siblings, 0 replies; 46+ messages in thread
From: Christoph Hellwig @ 2010-02-02 19:00 UTC (permalink / raw)
To: roland, oleg, akpm, chris; +Cc: linux-kernel, linux-arch
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 <hch@lst.de>
Index: linux-2.6/arch/xtensa/kernel/ptrace.c
===================================================================
--- linux-2.6.orig/arch/xtensa/kernel/ptrace.c 2010-02-02 19:18:16.218004699 +0100
+++ linux-2.6/arch/xtensa/kernel/ptrace.c 2010-02-02 19:21:56.730004589 +0100
@@ -30,6 +30,17 @@
#include <asm/elf.h>
#include <asm/coprocessor.h>
+
+void user_enable_single_step(struct task_struct *child)
+{
+ child->ptrace |= PT_SINGLESTEP;
+}
+
+void user_disable_single_step(struct task_struct *child)
+{
+ child->ptrace &= ~PT_SINGLESTEP;
+}
+
/*
* Called by kernel/ptrace.c when detaching to disable single stepping.
*/
@@ -258,51 +269,6 @@ long arch_ptrace(struct task_struct *chi
ret = ptrace_pokeusr(child, addr, data);
break;
- /* continue and stop at next (return from) syscall */
-
- case PTRACE_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 the single step bit is not set. */
- child->ptrace &= ~PT_SINGLESTEP;
- 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;
- child->ptrace &= ~PT_SINGLESTEP;
- wake_up_process(child);
- break;
-
- case PTRACE_SINGLESTEP:
- ret = -EIO;
- if (!valid_signal(data))
- break;
- clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
- child->ptrace |= PT_SINGLESTEP;
- child->exit_code = data;
- wake_up_process(child);
- ret = 0;
- break;
-
case PTRACE_GETREGS:
ret = ptrace_getregs(child, (void __user *) data);
break;
Index: linux-2.6/arch/xtensa/include/asm/ptrace.h
===================================================================
--- linux-2.6.orig/arch/xtensa/include/asm/ptrace.h 2010-02-02 19:18:16.234010089 +0100
+++ linux-2.6/arch/xtensa/include/asm/ptrace.h 2010-02-02 19:21:56.730004589 +0100
@@ -113,6 +113,7 @@ struct pt_regs {
#include <variant/core.h>
+# define arch_has_single_step() (1)
# define task_pt_regs(tsk) ((struct pt_regs*) \
(task_stack_page(tsk) + KERNEL_STACK_SIZE - (XCHAL_NUM_AREGS-16)*4) - 1)
# define user_mode(regs) (((regs)->ps & 0x00000020)!=0)
^ permalink raw reply [flat|nested] 46+ messages in thread* [PATCH 12/14] cris arch-v10: use generic ptrace_resume code
2010-02-02 18:57 [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h Christoph Hellwig
` (9 preceding siblings ...)
2010-02-02 19:00 ` [PATCH 11/14] xtensa: " Christoph Hellwig
@ 2010-02-02 19:00 ` Christoph Hellwig
2010-02-02 19:00 ` [PATCH, RFC 13/14] cris arch-v32: " Christoph Hellwig
` (4 subsequent siblings)
15 siblings, 0 replies; 46+ messages in thread
From: Christoph Hellwig @ 2010-02-02 19:00 UTC (permalink / raw)
To: roland, oleg, akpm, starvik, jesper.nilsson; +Cc: linux-kernel, linux-arch
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 <hch@lst.de>
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;
^ permalink raw reply [flat|nested] 46+ messages in thread* [PATCH, RFC 13/14] cris arch-v32: use generic ptrace_resume code
2010-02-02 18:57 [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h Christoph Hellwig
` (10 preceding siblings ...)
2010-02-02 19:00 ` [PATCH 12/14] cris arch-v10: " Christoph Hellwig
@ 2010-02-02 19:00 ` Christoph Hellwig
2010-02-02 19:00 ` [PATCH, RFC 14/14] m32r: " Christoph Hellwig
` (3 subsequent siblings)
15 siblings, 0 replies; 46+ messages in thread
From: Christoph Hellwig @ 2010-02-02 19:00 UTC (permalink / raw)
To: roland, oleg, akpm, starvik, jesper.nilsson; +Cc: linux-kernel, linux-arch
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 <hch@lst.de>
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 *);
^ permalink raw reply [flat|nested] 46+ messages in thread* [PATCH, RFC 14/14] m32r: use generic ptrace_resume code
2010-02-02 18:57 [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h Christoph Hellwig
` (11 preceding siblings ...)
2010-02-02 19:00 ` [PATCH, RFC 13/14] cris arch-v32: " Christoph Hellwig
@ 2010-02-02 19:00 ` Christoph Hellwig
2010-02-03 8:42 ` Mike Frysinger
` (2 subsequent siblings)
15 siblings, 0 replies; 46+ messages in thread
From: Christoph Hellwig @ 2010-02-02 19:00 UTC (permalink / raw)
To: roland, oleg, akpm, takata; +Cc: linux-kernel, linux-arch
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 <hch@lst.de>
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;
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h
2010-02-02 18:57 [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h Christoph Hellwig
@ 2010-02-03 8:42 ` Mike Frysinger
2010-02-02 18:58 ` [PATCH 3/14] arm: " Christoph Hellwig
` (14 subsequent siblings)
15 siblings, 0 replies; 46+ messages in thread
From: Mike Frysinger @ 2010-02-03 8:42 UTC (permalink / raw)
To: Christoph Hellwig
Cc: roland, oleg, akpm, linux-kernel, linux-arch, mattst88, ink, rth,
linux, hskinnemoen, vapier, starvik, jesper.nilsson, ysato,
takata, gerg, monstr, ralf, jdike, chris
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
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h
@ 2010-02-03 8:42 ` Mike Frysinger
0 siblings, 0 replies; 46+ messages in thread
From: Mike Frysinger @ 2010-02-03 8:42 UTC (permalink / raw)
To: Christoph Hellwig
Cc: roland, oleg, akpm, linux-kernel, linux-arch, mattst88, ink, rth,
linux, hskinnemoen, vapier, starvik, jesper.nilsson, ysato,
takata, gerg, monstr, ralf, jdike, chris
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
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h
2010-02-03 8:42 ` Mike Frysinger
(?)
@ 2010-02-03 8:56 ` Christoph Hellwig
-1 siblings, 0 replies; 46+ messages in thread
From: Christoph Hellwig @ 2010-02-03 8:56 UTC (permalink / raw)
To: Mike Frysinger
Cc: Christoph Hellwig, roland, oleg, akpm, linux-kernel, linux-arch,
mattst88, ink, rth, linux, hskinnemoen, vapier, starvik,
jesper.nilsson, ysato, takata, gerg, monstr, ralf, jdike, chris
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.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h
2010-02-02 18:57 [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h Christoph Hellwig
` (13 preceding siblings ...)
2010-02-03 8:42 ` Mike Frysinger
@ 2010-02-08 10:50 ` David Howells
2010-02-08 19:51 ` Roland McGrath
15 siblings, 0 replies; 46+ messages in thread
From: David Howells @ 2010-02-08 10:50 UTC (permalink / raw)
To: Christoph Hellwig
Cc: dhowells, roland, oleg, akpm, linux-kernel, linux-arch, mattst88,
ink, rth, linux, hskinnemoen, vapier, starvik, jesper.nilsson,
ysato, takata, gerg, monstr, ralf, jdike, chris
Christoph Hellwig <hch@lst.de> 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 <hch@lst.de>
Acked-by: David Howells <dhowells@redhat.com>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h
2010-02-02 18:57 [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h Christoph Hellwig
` (14 preceding siblings ...)
2010-02-08 10:50 ` David Howells
@ 2010-02-08 19:51 ` Roland McGrath
2010-02-10 22:03 ` Christoph Hellwig
15 siblings, 1 reply; 46+ messages in thread
From: Roland McGrath @ 2010-02-08 19:51 UTC (permalink / raw)
To: Christoph Hellwig
Cc: oleg, akpm, linux-kernel, linux-arch, mattst88, ink, rth, linux,
hskinnemoen, vapier, starvik, jesper.nilsson, ysato, takata,
gerg, monstr, ralf, jdike, chris
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
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH 1/14] move user_enable_single_step & co prototypes to linux/ptrace.h
2010-02-08 19:51 ` Roland McGrath
@ 2010-02-10 22:03 ` Christoph Hellwig
0 siblings, 0 replies; 46+ messages in thread
From: Christoph Hellwig @ 2010-02-10 22:03 UTC (permalink / raw)
To: Roland McGrath
Cc: Christoph Hellwig, oleg, akpm, linux-kernel, linux-arch,
mattst88, ink, rth, linux, hskinnemoen, vapier, starvik,
jesper.nilsson, ysato, takata, gerg, monstr, ralf, jdike, chris
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.
^ permalink raw reply [flat|nested] 46+ messages in thread