move user_enable_single_step & co prototypes to linux/ptrace.h
Subsystems (14)
PATCH 1/14 move user_enable_single_step & co prototypes to linux/ptrace.h
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.
arch/frv/include/asm/ptrace.h +0 -2
@@ -84,8 +84,6 @@ extern void show_regs(struct pt_regs *);#define task_pt_regs (task) ((task)->thread.frame0)#define arch_has_single_step () (1) -extern void user_enable_single_step (struct task_struct *); -extern void user_disable_single_step (struct task_struct *);#endif /* !__ASSEMBLY__ */ #endif /* __KERNEL__ */
arch/ia64/include/asm/ptrace.h +0 -4
@@ -319,11 +319,7 @@ static inline unsigned long user_stack_p ptrace_attach_sync_user_rbs(child)#define arch_has_single_step () (1) -extern void user_enable_single_step (struct task_struct *); -extern void user_disable_single_step (struct task_struct *); -#define arch_has_block_step () (1) -extern void user_enable_block_step (struct task_struct *);#endif /* !__KERNEL__ */
arch/m68k/include/asm/ptrace.h +0 -8
@@ -85,18 +85,10 @@ struct switch_stack {#define profile_pc (regs) instruction_pointer(regs)extern void show_regs (struct pt_regs *); -/* -* These are defined as per linux/ptrace.h. -*/ -struct task_struct ; -#define arch_has_single_step () (1) -extern void user_enable_single_step (struct task_struct *); -extern void user_disable_single_step (struct task_struct *);#ifdef CONFIG_MMU #define arch_has_block_step () (1) -extern void user_enable_block_step (struct task_struct *);#endif #endif /* __KERNEL__ */
arch/mn10300/include/asm/ptrace.h +0 -2
@@ -99,8 +99,6 @@ struct task_struct;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 *);#endif /* !__ASSEMBLY */
arch/parisc/include/asm/ptrace.h +0 -5
@@ -47,13 +47,8 @@ struct pt_regs {#define task_regs (task) ((struct pt_regs *) ((char *)(task) + TASK_REGS)) -struct task_struct ;#define arch_has_single_step () 1 -void user_disable_single_step (struct task_struct *task); -void user_enable_single_step (struct task_struct *task); -#define arch_has_block_step () 1 -void user_enable_block_step (struct task_struct *task);/* XXX should we use iaoq[1] or iaoq[0] ? */ #define user_mode (regs) (((regs)->iaoq[0] & 3) ? 1 : 0)
arch/powerpc/include/asm/ptrace.h +0 -7
@@ -131,15 +131,8 @@ do { \ } while (0)#endif /* __powerpc64__ */ -/* -* These are defined as per linux/ptrace.h, which see. -*/ #define arch_has_single_step () (1)#define arch_has_block_step () (!cpu_has_feature(CPU_FTR_601)) -extern void user_enable_single_step (struct task_struct *); -extern void user_enable_block_step (struct task_struct *); -extern void user_disable_single_step (struct task_struct *); -#define ARCH_HAS_USER_SINGLE_STEP_INFO #endif /* __ASSEMBLY__ */
arch/s390/include/asm/ptrace.h +0 -4
@@ -489,10 +489,6 @@ struct user_regs_struct* These are defined as per linux/ptrace.h, which see. */ #define arch_has_single_step () (1) -struct task_struct ; -extern void user_enable_single_step (struct task_struct *); -extern void user_disable_single_step (struct task_struct *); -#define user_mode (regs) (((regs)->psw.mask & PSW_MASK_PSTATE) != 0)#define instruction_pointer (regs) ((regs)->psw.addr & PSW_ADDR_INSN)#define user_stack_pointer (regs)((regs)->gprs[15])
arch/score/include/asm/ptrace.h +1 -2
@@ -90,8 +90,7 @@ extern int read_tsk_short(struct task_st unsigned short *);#define arch_has_single_step () (1) -extern void user_enable_single_step (struct task_struct *); -extern void user_disable_single_step (struct task_struct *); +#endif /* __KERNEL__ */ #endif /* _ASM_SCORE_PTRACE_H */
arch/sh/include/asm/ptrace.h +0 -2
@@ -121,8 +121,6 @@ extern void show_regs(struct pt_regs *);struct task_struct ;#define arch_has_single_step () (1) -extern void user_enable_single_step (struct task_struct *); -extern void user_disable_single_step (struct task_struct *);#define task_pt_regs (task) \ ((struct pt_regs *) (task_stack_page(task) + THREAD_SIZE) - 1)
arch/x86/include/asm/ptrace.h +0 -7
@@ -278,14 +278,7 @@ static inline unsigned long regs_get_kerextern unsigned long regs_get_argument_nth (struct pt_regs *regs,unsigned int n); -/* -* These are defined as per linux/ptrace.h, which see. -*/ #define arch_has_single_step () (1) -extern void user_enable_single_step (struct task_struct *); -extern void user_disable_single_step (struct task_struct *); - -extern void user_enable_block_step (struct task_struct *);#ifdef CONFIG_X86_DEBUGCTLMSR #define arch_has_block_step () (1)#else
include/linux/ptrace.h +5 -0
@@ -238,6 +238,9 @@ static inline void user_enable_single_ststatic 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 */ #ifndef arch_has_block_step
@@ -265,6 +268,8 @@ static inline void user_enable_block_ste {BUG ();/* This can never be called. */ } +#else +extern void user_enable_block_step (struct task_struct *);#endif /* arch_has_block_step */ #ifdef ARCH_HAS_USER_SINGLE_STEP_INFO
PATCH 2/14 alpha: use generic ptrace_resume code
Use the generic ptrace_resume code for PTRACE_SYSCALL, PTRACE_CONT, PTRACE_KILL and PTRACE_SINGLESTEP. This implies defining arch_has_single_step in <asm/ptrace.h> and implementing the user_enable_single_step and user_disable_single_step functions, which also causes the breakpoint information to be cleared on fork, which could be considered a bug fix.
Also the TIF_SYSCALL_TRACE thread flag is now cleared on PTRACE_KILL which it previously wasn't, which is consistent with all architectures using the modern ptrace code.
arch/alpha/kernel/ptrace.c +12 -47
@@ -249,6 +249,17 @@ ptrace_cancel_bpt(struct task_struct * creturn (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 *chiDBG (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 ;
arch/alpha/include/asm/ptrace.h +1 -0
@@ -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)
Re: [PATCH 2/14] alpha: use generic ptrace_resume code
Quoted 103 lines · Christoph Hellwig
Use the generic
ptrace_resumecode forPTRACE_SYSCALL,PTRACE_CONT,PTRACE_KILLandPTRACE_SINGLESTEP. This implies definingarch_has_single_stepin <asm/ptrace.h> and implementing theuser_enable_single_stepanduser_disable_single_stepfunctions, which also causes the breakpoint information to be cleared on fork, which could be considered a bug fix.Also the
TIF_SYSCALL_TRACEthread flag is now cleared onPTRACE_KILLwhich it previously wasn't, which is consistent with all architectures using the modern ptrace code.Signed-off-by Christoph Hellwig <[email protected]>Index:
linux-2.6/arch/alpha/kernel/ptrace.c
===================================================================arch/alpha/kernel/ptrace.c 2010-02-02 11:07:04.091006019 +0100 renamed from arch/alpha/kernel/ptrace.c 2010-02-02 11:05:03.197004058 +0100 +12
-47@@ -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
===================================================================
arch/alpha/include/asm/ptrace.h 2010-02-02 11:07:38.645007248 +0100 struct switch_stack { · line 74
Quoted 7 lines
arch/alpha/include/asm/ptrace.h 2010-02-02 11:07:38.645007248 +0100 renamed from arch/alpha/include/asm/ptrace.h 2010-02-02 11:07:29.091023600 +0100 +1
-0@@ -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)
PATCH 3/14 arm: use generic ptrace_resume code
In reply to Christoph HellwigUse 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.
arch/arm/kernel/ptrace.c +12 -48
@@ -452,12 +452,23 @@ void ptrace_cancel_bpt(struct task_strucclear_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 ;
arch/arm/kernel/ptrace.h +0 -14
@@ -14,20 +14,6 @@ extern void ptrace_set_bpt(struct task_sextern 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)
arch/arm/include/asm/ptrace.h +2 -0
@@ -128,6 +128,8 @@ struct pt_regs {#ifdef __KERNEL__ +#define arch_has_single_step () (1) +#define user_mode (regs) \ (((regs)->ARM_cpsr & 0xf) == 0)
PATCH 4/14 avr32: use generic ptrace_resume code
In reply to Christoph HellwigUse 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.
arch/avr32/include/asm/ptrace.h +2 -0
@@ -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)
arch/avr32/kernel/ptrace.c +7 -46
72 lines not on this page · raw message
Re: [PATCH 4/14] avr32: use generic ptrace_resume code
Quoted 11 lines · On Tue, 2 Feb 2010 19:58:57 +0100 Christoph Hellwig <[email protected]> wrote:
Use the generic
ptrace_resumecode forPTRACE_SYSCALL,PTRACE_CONT,PTRACE_KILLandPTRACE_SINGLESTEP. This implies definingarch_has_single_stepin <asm/ptrace.h> and implementing theuser_enable_single_stepanduser_disable_single_stepfunctions, which also causes the breakpoint information to be cleared on fork, which could be considered a bug fix.Also the
TIF_SYSCALL_TRACEthread flag is now cleared onPTRACE_KILLwhich it previously wasn't which is consistent with all architectures using the modern ptrace code.
Quoted 5 lines
Currently avr32 doesn't implement any code to disable single stepping when one of the non-syscall requests is called which seems wrong, but I've left it as-is for now.
Signed-off-by Christoph Hellwig <[email protected]>
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
Re: [PATCH 4/14] avr32: use generic ptrace_resume code
Quoted 3 lines · Haavard Skinnemoen
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.
Oleg Nesterov
maintainerRe: [PATCH 4/14] avr32: use generic ptrace_resume code
Quoted 4 lines · Christoph Hellwig
I don't think the flag clearing on
PTRACE_KILLmatters 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.
Re: [PATCH 4/14] avr32: use generic ptrace_resume code
Quoted 6 lines · Oleg Nesterov
Quoted 4 lines · Christoph Hellwig
I don't think the flag clearing on
PTRACE_KILLmatters as the process is a zombie and almost gone anyway, it's more an artefact of the generic implementation that I wanted to mention.
Quoted 6 lines
Well,
PTRACE_KILLdoesn'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_KILLdoesn't really matter beausePTRACE_KILLshouldn'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.
PATCH 5/14 blackfin: use generic ptrace_resume code
In reply to Christoph HellwigUse 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.
arch/blackfin/kernel/ptrace.c +10 -49
@@ -160,7 +160,15 @@ static inline int is_user_addr_valid(strreturn -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);
arch/blackfin/include/asm/ptrace.h +2 -0
@@ -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)))
Re: [PATCH 5/14] blackfin: use generic ptrace_resume code
Quoted 10 lines · Christoph Hellwig
Use the generic
ptrace_resumecode forPTRACE_SYSCALL,PTRACE_CONT,PTRACE_KILLandPTRACE_SINGLESTEP. This implies definingarch_has_single_stepin <asm/ptrace.h> and implementing theuser_enable_single_stepanduser_disable_single_stepfunctions, which also causes the breakpoint information to be cleared on fork, which could be considered a bug fix.Also the
TIF_SYSCALL_TRACEthread flag is now cleared onPTRACE_KILLwhich 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
Re: [PATCH 5/14] blackfin: use generic ptrace_resume code
Quoted 8 lines · Mike Frysinger
Quoted 7 lines · Christoph Hellwig
Use the generic
ptrace_resumecode forPTRACE_SYSCALL,PTRACE_CONT,PTRACE_KILLandPTRACE_SINGLESTEP. This implies definingarch_has_single_stepin <asm/ptrace.h> and implementing theuser_enable_single_stepanduser_disable_single_stepfunctions, which also causes the breakpoint information to be cleared on fork, which could be considered a bug fix.
Quoted 7 lines
Quoted 3 lines
Also the
TIF_SYSCALL_TRACEthread flag is now cleared onPTRACE_KILLwhich 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
Re: [PATCH 5/14] blackfin: use generic ptrace_resume code
Quoted 7 lines · Mike Frysinger
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_xxxthings 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.
cover Blackfin: use generic ptrace_resume code
In reply to Christoph HellwigThese 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).
PATCH 1/2 Blackfin: initial tracehook support
In reply to Christoph Hellwig--- 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
arch/blackfin/Kconfig +1 -0
@@ -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
arch/blackfin/include/asm/ptrace.h +23 -0
@@ -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__ */
arch/blackfin/include/asm/syscall.h new file +96 -0
96 lines not on this page · raw message
arch/blackfin/kernel/ptrace.c +19 -52
142 lines not on this page · raw message
arch/blackfin/kernel/signal.c +2 -2
@@ -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 ;
arch/blackfin/mach-common/entry.S +4 -2
@@ -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)
Signature · 1 line
1.6.6.1
Re: [PATCH 1/2] Blackfin: initial tracehook support
arch/blackfin/Kconfig config RWSEM_XCHGADD_ALGORITHM · line 27
Quoted 4 lines
config BLACKFIN
arch/blackfin/Kconfig +1
-0def_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.
arch/blackfin/include/asm/syscall.h line 61
Quoted 6 lines
arch/blackfin/include/asm/syscall.h new file +6
-0+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.
arch/blackfin/kernel/ptrace.c long arch_ptrace(struct task_struct *child, long request, long addr, long data) · line 410
Quoted 5 lines
arch/blackfin/kernel/ptrace.c +4
-0+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?
arch/blackfin/kernel/signal.c setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t * info, · line 217
Quoted 7 lines
arch/blackfin/kernel/signal.c +1
-1@@ -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
Re: [PATCH 1/2] Blackfin: initial tracehook support
Quoted 7 lines · Roland McGrath
Quoted 4 lines
config BLACKFIN
def_booly
selectHAVE_ARCH_KGDB
+ selectHAVE_ARCH_TRACEHOOKDon't define this until you have all its constituents as listed in the
arch/Kconfigcomment. I don't seeuser_regsetsupport.
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).
Quoted 9 lines
arch/blackfin/include/asm/syscall.h line 61
Quoted 6 lines
arch/blackfin/include/asm/syscall.h new file +6
-0+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 ...
Quoted 8 lines
Quoted 5 lines
arch/blackfin/kernel/ptrace.c +4
-0+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
Quoted 13 lines
arch/blackfin/kernel/signal.c setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t * info, · line 217
Quoted 7 lines
arch/blackfin/kernel/signal.c +1
-1@@ -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.
Quoted 3 lines
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
Re: [PATCH 1/2] Blackfin: initial tracehook support
Quoted 3 lines
where is
user_regsetactually used ? i only see it infs/binfmt_elf.cand 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.
Quoted 2 lines
i dont see anyone calling
syscall_get_arguments()with i!=0, and a few other arches are doing theBUG_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.
Quoted 1 line
but should be easy to implement this with memory walking code ...
Good!
Quoted 7 lines
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.
Quoted 2 lines
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.
Quoted 3 lines
i'll move it to how most arches seem to do it -- in
do_signalafter a successful call tohandle_signaland after clearingTIF_RESTORE_SIGMASK.
That is correct.
Thanks,
Roland
Re: [PATCH 1/2] Blackfin: initial tracehook support
Quoted 4 lines · Roland McGrath
Quoted 3 lines
where is
user_regsetactually used ? i only see it infs/binfmt_elf.cand core dumps, neither of which work on nommu systems (or at least on Blackfin systems).
Quoted 8 lines
The core dump code in
binfmt_elf_fdpic.cappears identical to an old version of thebinfmt_elf.ccode. 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 theCORE_DUMP_USE_REGSETflavor of the code and clean out any old core-related cruft you had inasm/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.
Quoted 11 lines
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 onCONFIG_HAVE_ARCH_TRACEHOOKand your kernel will stop building if you have set that without meeting its requirements.Moreover, the whole point of
CONFIG_HAVE_ARCH_TRACEHOOKis to indicate the minimum arch requirements that all future generic code can rely on. If you set it without meeting the documented requirements asarch/Kconfigtells 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.
Quoted 5 lines
Quoted 2 lines
i dont see anyone calling
syscall_get_arguments()with i!=0, and a few other arches are doing theBUG_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.
Quoted 11 lines
Quoted 7 lines
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.
Quoted 2 lines
You should always call
tracehook_report_syscall_exit()ifTIF_SYSCALL_TRACEis set. Whether to call it otherwise depends on arch details.
this is all set for us
Quoted 10 lines
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
Oleg Nesterov
maintainerRe: [PATCH 1/2] Blackfin: initial tracehook support
Quoted 14 lines · Mike Frysinger
Quoted 10 lines · Roland McGrath
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.
Quoted 4 lines
so
tracehook_report_syscall_exit()checkingTIF_SINGLESTEPonly 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.
Quoted 3 lines
also, in reading the kerneldocs for
tracehook_report_syscall_exit(), it says "an attempted system call". should system calls greater thanNR_syscall(-ENOSYS) also get traced ?
I'd say yes, but let's wait for Roland's reply.
Oleg.
Re: [PATCH 1/2] Blackfin: initial tracehook support
In reply to Mike FrysingerQuoted 4 lines
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.
Quoted 6 lines
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).
Quoted 2 lines
i missed the passing of
TIF_SINGLESTEPin 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.
Quoted 5 lines
the way we do it on Blackfin atm is that syscall_trace_{enter,leave} are only called when
TIF_SYSCALL_TRACEis set (in the low level assembly), so any checking ofthread_flagshere is pointless. i guess the low level code needs updating to check a mask so that we can addTIF_SYSCALL_TRACEPOINTeasier.
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.
Quoted 4 lines
so
tracehook_report_syscall_exit()checkingTIF_SINGLESTEPonly 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.
Quoted 6 lines
also, in reading the kerneldocs for
tracehook_report_syscall_exit(), it says "an attempted system call". should system calls greater thanNR_syscall(-ENOSYS) also get traced ? we dont currently do this on the Blackfin port, but going bystracedifferences 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
Re: [PATCH 1/2] Blackfin: initial tracehook support
Quoted 4 lines · Roland McGrath
Moreover, the usual cleanup is to make your
arch_ptrace()usecopy_regset_from_user()andcopy_regset_to_user()to implement existing calls ikePTRACE_GETREGS. That way, existing ptrace users (strace, gdb) become tests of theuser_regsetpaths (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 ...
Quoted 6 lines
What happens today when
PTRACE_SINGLESTEPis 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 itssyscall_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
Quoted 16 lines
Quoted 6 lines
also, in reading the kerneldocs for
tracehook_report_syscall_exit(), it says "an attempted system call". should system calls greater thanNR_syscall(-ENOSYS) also get traced ? we dont currently do this on the Blackfin port, but going bystracedifferences 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 (viauser_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 beforetracehook_report_syscall_entry()whether it is "real" or not. For everytracehook_report_syscall_entry()call, there must be a correspondingtracehook_report_syscall_exit()call (unlessTIF_SYSCALL_TRACEwas cleared in the interim). At that exit point, the registers can again be changed.
Quoted 5 lines
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
-ENOSYSand 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
Re: [PATCH 1/2] Blackfin: initial tracehook support
Quoted 5 lines · Mike Frysinger
Quoted 4 lines · Roland McGrath
Moreover, the usual cleanup is to make your
arch_ptrace()usecopy_regset_from_user()andcopy_regset_to_user()to implement existing calls ikePTRACE_GETREGS. That way, existing ptrace users (strace, gdb) become tests of theuser_regsetpaths (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.
Quoted 5 lines
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
Re: [PATCH 1/2] Blackfin: initial tracehook support
In reply to Mike FrysingerQuoted 5 lines
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.
Quoted 2 lines
there are more pseudo regs as required by FDPIC, but they arent in the
pt_regslayout ...
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.
Quoted 3 lines
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.
Quoted 2 lines
i fixed the Blackfin code to do NR checking after tracing and now
straceshows the bad syscall
Good!
Quoted 2 lines
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).
Quoted 3 lines
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.
Quoted 7 lines
the Blackfin code when traced now does:
calltracehook_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-ENOSYSstore return value into regs
calltracehook_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
PATCH 2/2 Blackfin: use generic ptrace_resume code
In reply to Christoph HellwigFrom: Christoph Hellwig <[email protected]>
Use the generic ptrace_resume code for PTRACE_SYSCALL, PTRACE_CONT, PTRACE_KILL and PTRACE_SINGLESTEP. This implies defining arch_has_single_step in <asm/ptrace.h> and implementing the user_enable_single_step and user_disable_single_step functions, which also causes the breakpoint information to be cleared on fork, which could be considered a bug fix.
Also the TIF_SYSCALL_TRACE thread flag is now cleared on PTRACE_KILL which it previously wasn't which is consistent with all architectures using the modern ptrace code.
--- arch/blackfin/kernel/ptrace.c | 44 ----------------------------------------- 1 files changed, 0 insertions(+), 44 deletions(-)
arch/blackfin/kernel/ptrace.c +0 -44
@@ -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);
Signature · 1 line
1.6.6.1
PATCH 6/14 h8300: use generic ptrace_resume code
In reply to Christoph HellwigUse 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.
arch/h8300/kernel/ptrace.c +6 -46
@@ -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 tavoid 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;
arch/h8300/platform/h8300h/ptrace_h8300h.c +3 -3
@@ -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);
arch/h8300/platform/h8s/ptrace_h8s.c +2 -2
@@ -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 ; }
arch/h8300/include/asm/ptrace.h +2 -0
@@ -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)
PATCH 7/14 m68knommu: use generic ptrace_resume code
In reply to Christoph HellwigUse 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.
arch/m68knommu/kernel/ptrace.c +0 -56
@@ -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;
Re: [PATCH 7/14] m68knommu: use generic ptrace_resume code
Quoted 10 lines · Christoph Hellwig
Use the generic
ptrace_resumecode forPTRACE_SYSCALL,PTRACE_CONT,PTRACE_KILLandPTRACE_SINGLESTEP. m68knommu already defines the nessecaryuser_enable_single_stepanduser_disable_single_stepfunctions for this.Also the
TIF_SYSCALL_TRACEthread flag is now cleared onPTRACE_KILLwhich it previously wasn't which is consistent with all architectures using the modern ptrace code.Signed-off-by Christoph Hellwig <[email protected]>
------------------------------------------------------------------------
Greg Ungerer -- Principal Engineer EMAIL: [email protected] SnapGear Group, McAfee PHONE: +61 7 3435 2888 8 Gardner Close FAX: +61 7 3217 5323 Milton, QLD, 4064, Australia WEB: http://www.SnapGear.com
PATCH 8/14 microblaze: use generic ptrace_resume code
In reply to Christoph HellwigUse 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.
arch/microblaze/kernel/ptrace.c +0 -37
@@ -110,43 +110,6 @@ long arch_ptrace(struct task_struct *chiif (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); }
Re: [PATCH 8/14] microblaze: use generic ptrace_resume code
Quoted 59 lines · Christoph Hellwig
Use the generic
ptrace_resumecode forPTRACE_SYSCALL,PTRACE_CONTandPTRACE_KILL. This also makesPTRACE_SINGLESTEPreturn-EIOwhile it previously succeeded despite not actually causing any kind of single stepping.Also the
TIF_SYSCALL_TRACEthread flag is now cleared onPTRACE_KILLwhich it previously wasn't which is consistent with all architectures using the modern ptrace code.Signed-off-by Christoph Hellwig <[email protected]>Index:
linux-2.6/arch/microblaze/kernel/ptrace.c
===================================================================arch/microblaze/kernel/ptrace.c +0
-37@@ -110,43 +110,6 @@ long arch_ptrace(struct task_struct *chiif (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); }
Signature · 4 lines
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
PATCH 9/14 mips: use generic ptrace_resume code
In reply to Christoph HellwigUse 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.
arch/mips/kernel/ptrace.c +0 -30
@@ -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);
Re: [PATCH 9/14] mips: use generic ptrace_resume code
Quoted 8 lines · Christoph Hellwig
Use the generic
ptrace_resumecode forPTRACE_SYSCALL,PTRACE_CONTandPTRACE_KILL.Also the
TIF_SYSCALL_TRACEthread flag is now cleared onPTRACE_KILLwhich it previously wasn't which is consistent with all architectures using the modern ptrace code.Signed-off-by Christoph Hellwig <[email protected]>
Ralf
PATCH 10/14 um: use generic ptrace_resume code
In reply to Christoph HellwigUse 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.
arch/um/kernel/ptrace.c +16 -54
@@ -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 )) {
arch/um/include/asm/ptrace-generic.h +2 -0
@@ -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)
PATCH 11/14 xtensa: use generic ptrace_resume code
In reply to Christoph HellwigUse 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.
arch/xtensa/kernel/ptrace.c +11 -45
@@ -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 ;
arch/xtensa/include/asm/ptrace.h +1 -0
@@ -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)
PATCH 12/14 cris arch-v10: use generic ptrace_resume code
In reply to Christoph HellwigUse 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.
arch/cris/arch-v10/kernel/ptrace.c +0 -51
@@ -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;
RFC PATCH 13/14 cris arch-v32: use generic ptrace_resume code
In reply to Christoph HellwigUse 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.
arch/cris/arch-v32/kernel/ptrace.c +30 -79
@@ -78,6 +78,35 @@ int put_reg(struct task_struct *task, unreturn 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 *childunsigned 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;
arch/cris/include/arch-v32/arch/ptrace.h +1 -0
@@ -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 *);
RFC PATCH 14/14 m32r: use generic ptrace_resume code
In reply to Christoph HellwigUse 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.
arch/m32r/include/asm/ptrace.h +2 -0
@@ -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) \
arch/m32r/kernel/ptrace.c +29 -68
109 lines not on this page · raw message
Quoted 4 lines · Christoph Hellwig
While in theory user_enable_single_step/user_disable_single_step/
user_enable_blockstepcould 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 ?
include/linux/ptrace.h line 244
Quoted 10 lines
include/linux/ptrace.h +3
-0@@ -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
Quoted 6 lines · Mike Frysinger
Quoted 4 lines · Christoph Hellwig
While in theory user_enable_single_step/user_disable_single_step/
user_enable_blockstepcould 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.
Quoted 7 lines
the only annoying thing here is that we currently have to enable both
user_disable_single_step()andptrace_disable()that do exactly the same thing. i avoided this somewhat on Blackfin by cheating:
#defineuser_disable_single_step(child)ptrace_disable(child)so now there's no code bloat. perhaps this could be moved into common
linux/ptrace.htoo ?
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.
Quoted 6 lines · Christoph Hellwig
While in theory user_enable_single_step/user_disable_single_step/
user_enable_blockstepcould also be provided as an inline or macro there's no good reason to do so, and having the prototype in one places keeps code size and confusion down.Signed-off-by Christoph Hellwig <[email protected]>
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
Quoted 5 lines · Roland McGrath
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.
Quoted 5 lines
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_SINGLESTEPdoes 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
Quoted 8 lines
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 havingarch_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.