[PATCH v2] ARM: ptrace: remove single-step emulation code

Nicolas Pitre nico at fluxnic.net
Wed Feb 9 14:28:41 EST 2011


On Wed, 9 Feb 2011, Will Deacon wrote:

> PTRACE_SINGLESTEP is a ptrace request designed to offer single-stepping
> support to userspace when the underlying architecture has hardware
> support for this operation.
> 
> On ARM, we set arch_has_single_step() to 1 and attempt to emulate hardware
> single-stepping by disassembling the current instruction to determine the
> next pc and placing a software breakpoint on that location.
> 
> Unfortunately this has the following problems:
> 
> 1.) Only a subset of ARMv7 instructions are supported
> 2.) Thumb-2 is unsupported
> 3.) The code is not SMP safe
> 
> We could try to fix this code, but it turns out that because of the above
> issues it is rarely used in practice.  GDB, for example, uses PTRACE_POKETEXT
> and PTRACE_PEEKTEXT to manage breakpoints itself and does not require any
> kernel assistance.
> 
> This patch removes the single-step emulation code from ptrace meaning that
> the PTRACE_SINGLESTEP request will return -EIO on ARM. Portable code must
> check the return value from a ptrace call and handle the failure gracefully.
> 
> Signed-off-by: Will Deacon <will.deacon at arm.com>

Acked-by: Nicolas Pitre <nicolas.pitre at linaro.org>

> ---
> 
> The comments I received about v1 suggest that:
> 
>  - If emulation is required, it is plausible to do it from userspace
>  - ltrace uses the SINGLESTEP call (conditionally at compile-time since
>    other architectures, such as mips, do not support this request) but
>    does not check the return value from ptrace. This is a bug in ltrace.
>  - strace does not use SINGLESTEP
> 
> Further feedback welcome.
> 
>  arch/arm/include/asm/processor.h |   12 --
>  arch/arm/include/asm/ptrace.h    |    2 -
>  arch/arm/include/asm/traps.h     |    1 +
>  arch/arm/kernel/ptrace.c         |  383 +-------------------------------------
>  arch/arm/kernel/ptrace.h         |   37 ----
>  arch/arm/kernel/signal.c         |    9 -
>  arch/arm/kernel/traps.c          |    2 +-
>  7 files changed, 3 insertions(+), 443 deletions(-)
>  delete mode 100644 arch/arm/kernel/ptrace.h
> 
> diff --git a/arch/arm/include/asm/processor.h b/arch/arm/include/asm/processor.h
> index 67357ba..b439b41 100644
> --- a/arch/arm/include/asm/processor.h
> +++ b/arch/arm/include/asm/processor.h
> @@ -29,19 +29,7 @@
>  #define STACK_TOP_MAX	TASK_SIZE
>  #endif
>  
> -union debug_insn {
> -	u32	arm;
> -	u16	thumb;
> -};
> -
> -struct debug_entry {
> -	u32			address;
> -	union debug_insn	insn;
> -};
> -
>  struct debug_info {
> -	int			nsaved;
> -	struct debug_entry	bp[2];
>  #ifdef CONFIG_HAVE_HW_BREAKPOINT
>  	struct perf_event	*hbp[ARM_MAX_HBP_SLOTS];
>  #endif
> diff --git a/arch/arm/include/asm/ptrace.h b/arch/arm/include/asm/ptrace.h
> index 783d50f..a8ff22b 100644
> --- a/arch/arm/include/asm/ptrace.h
> +++ b/arch/arm/include/asm/ptrace.h
> @@ -130,8 +130,6 @@ struct pt_regs {
>  
>  #ifdef __KERNEL__
>  
> -#define arch_has_single_step()	(1)
> -
>  #define user_mode(regs)	\
>  	(((regs)->ARM_cpsr & 0xf) == 0)
>  
> diff --git a/arch/arm/include/asm/traps.h b/arch/arm/include/asm/traps.h
> index 1b960d5..f90756d 100644
> --- a/arch/arm/include/asm/traps.h
> +++ b/arch/arm/include/asm/traps.h
> @@ -45,6 +45,7 @@ static inline int in_exception_text(unsigned long ptr)
>  
>  extern void __init early_trap_init(void);
>  extern void dump_backtrace_entry(unsigned long where, unsigned long from, unsigned long frame);
> +extern void ptrace_break(struct task_struct *tsk, struct pt_regs *regs);
>  
>  extern void *vectors_page;
>  
> diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
> index 19c6816..eace844 100644
> --- a/arch/arm/kernel/ptrace.c
> +++ b/arch/arm/kernel/ptrace.c
> @@ -26,8 +26,6 @@
>  #include <asm/system.h>
>  #include <asm/traps.h>
>  
> -#include "ptrace.h"
> -
>  #define REG_PC	15
>  #define REG_PSR	16
>  /*
> @@ -184,389 +182,12 @@ put_user_reg(struct task_struct *task, int offset, long data)
>  	return ret;
>  }
>  
> -static inline int
> -read_u32(struct task_struct *task, unsigned long addr, u32 *res)
> -{
> -	int ret;
> -
> -	ret = access_process_vm(task, addr, res, sizeof(*res), 0);
> -
> -	return ret == sizeof(*res) ? 0 : -EIO;
> -}
> -
> -static inline int
> -read_instr(struct task_struct *task, unsigned long addr, u32 *res)
> -{
> -	int ret;
> -
> -	if (addr & 1) {
> -		u16 val;
> -		ret = access_process_vm(task, addr & ~1, &val, sizeof(val), 0);
> -		ret = ret == sizeof(val) ? 0 : -EIO;
> -		*res = val;
> -	} else {
> -		u32 val;
> -		ret = access_process_vm(task, addr & ~3, &val, sizeof(val), 0);
> -		ret = ret == sizeof(val) ? 0 : -EIO;
> -		*res = val;
> -	}
> -	return ret;
> -}
> -
> -/*
> - * Get value of register `rn' (in the instruction)
> - */
> -static unsigned long
> -ptrace_getrn(struct task_struct *child, unsigned long insn)
> -{
> -	unsigned int reg = (insn >> 16) & 15;
> -	unsigned long val;
> -
> -	val = get_user_reg(child, reg);
> -	if (reg == 15)
> -		val += 8;
> -
> -	return val;
> -}
> -
> -/*
> - * Get value of operand 2 (in an ALU instruction)
> - */
> -static unsigned long
> -ptrace_getaluop2(struct task_struct *child, unsigned long insn)
> -{
> -	unsigned long val;
> -	int shift;
> -	int type;
> -
> -	if (insn & 1 << 25) {
> -		val = insn & 255;
> -		shift = (insn >> 8) & 15;
> -		type = 3;
> -	} else {
> -		val = get_user_reg (child, insn & 15);
> -
> -		if (insn & (1 << 4))
> -			shift = (int)get_user_reg (child, (insn >> 8) & 15);
> -		else
> -			shift = (insn >> 7) & 31;
> -
> -		type = (insn >> 5) & 3;
> -	}
> -
> -	switch (type) {
> -	case 0:	val <<= shift;	break;
> -	case 1:	val >>= shift;	break;
> -	case 2:
> -		val = (((signed long)val) >> shift);
> -		break;
> -	case 3:
> - 		val = (val >> shift) | (val << (32 - shift));
> -		break;
> -	}
> -	return val;
> -}
> -
> -/*
> - * Get value of operand 2 (in a LDR instruction)
> - */
> -static unsigned long
> -ptrace_getldrop2(struct task_struct *child, unsigned long insn)
> -{
> -	unsigned long val;
> -	int shift;
> -	int type;
> -
> -	val = get_user_reg(child, insn & 15);
> -	shift = (insn >> 7) & 31;
> -	type = (insn >> 5) & 3;
> -
> -	switch (type) {
> -	case 0:	val <<= shift;	break;
> -	case 1:	val >>= shift;	break;
> -	case 2:
> -		val = (((signed long)val) >> shift);
> -		break;
> -	case 3:
> - 		val = (val >> shift) | (val << (32 - shift));
> -		break;
> -	}
> -	return val;
> -}
> -
> -#define OP_MASK	0x01e00000
> -#define OP_AND	0x00000000
> -#define OP_EOR	0x00200000
> -#define OP_SUB	0x00400000
> -#define OP_RSB	0x00600000
> -#define OP_ADD	0x00800000
> -#define OP_ADC	0x00a00000
> -#define OP_SBC	0x00c00000
> -#define OP_RSC	0x00e00000
> -#define OP_ORR	0x01800000
> -#define OP_MOV	0x01a00000
> -#define OP_BIC	0x01c00000
> -#define OP_MVN	0x01e00000
> -
> -static unsigned long
> -get_branch_address(struct task_struct *child, unsigned long pc, unsigned long insn)
> -{
> -	u32 alt = 0;
> -
> -	switch (insn & 0x0e000000) {
> -	case 0x00000000:
> -	case 0x02000000: {
> -		/*
> -		 * data processing
> -		 */
> -		long aluop1, aluop2, ccbit;
> -
> -	        if ((insn & 0x0fffffd0) == 0x012fff10) {
> -		        /*
> -			 * bx or blx
> -			 */
> -			alt = get_user_reg(child, insn & 15);
> -			break;
> -		}
> -
> -
> -		if ((insn & 0xf000) != 0xf000)
> -			break;
> -
> -		aluop1 = ptrace_getrn(child, insn);
> -		aluop2 = ptrace_getaluop2(child, insn);
> -		ccbit  = get_user_reg(child, REG_PSR) & PSR_C_BIT ? 1 : 0;
> -
> -		switch (insn & OP_MASK) {
> -		case OP_AND: alt = aluop1 & aluop2;		break;
> -		case OP_EOR: alt = aluop1 ^ aluop2;		break;
> -		case OP_SUB: alt = aluop1 - aluop2;		break;
> -		case OP_RSB: alt = aluop2 - aluop1;		break;
> -		case OP_ADD: alt = aluop1 + aluop2;		break;
> -		case OP_ADC: alt = aluop1 + aluop2 + ccbit;	break;
> -		case OP_SBC: alt = aluop1 - aluop2 + ccbit;	break;
> -		case OP_RSC: alt = aluop2 - aluop1 + ccbit;	break;
> -		case OP_ORR: alt = aluop1 | aluop2;		break;
> -		case OP_MOV: alt = aluop2;			break;
> -		case OP_BIC: alt = aluop1 & ~aluop2;		break;
> -		case OP_MVN: alt = ~aluop2;			break;
> -		}
> -		break;
> -	}
> -
> -	case 0x04000000:
> -	case 0x06000000:
> -		/*
> -		 * ldr
> -		 */
> -		if ((insn & 0x0010f000) == 0x0010f000) {
> -			unsigned long base;
> -
> -			base = ptrace_getrn(child, insn);
> -			if (insn & 1 << 24) {
> -				long aluop2;
> -
> -				if (insn & 0x02000000)
> -					aluop2 = ptrace_getldrop2(child, insn);
> -				else
> -					aluop2 = insn & 0xfff;
> -
> -				if (insn & 1 << 23)
> -					base += aluop2;
> -				else
> -					base -= aluop2;
> -			}
> -			read_u32(child, base, &alt);
> -		}
> -		break;
> -
> -	case 0x08000000:
> -		/*
> -		 * ldm
> -		 */
> -		if ((insn & 0x00108000) == 0x00108000) {
> -			unsigned long base;
> -			unsigned int nr_regs;
> -
> -			if (insn & (1 << 23)) {
> -				nr_regs = hweight16(insn & 65535) << 2;
> -
> -				if (!(insn & (1 << 24)))
> -					nr_regs -= 4;
> -			} else {
> -				if (insn & (1 << 24))
> -					nr_regs = -4;
> -				else
> -					nr_regs = 0;
> -			}
> -
> -			base = ptrace_getrn(child, insn);
> -
> -			read_u32(child, base + nr_regs, &alt);
> -			break;
> -		}
> -		break;
> -
> -	case 0x0a000000: {
> -		/*
> -		 * bl or b
> -		 */
> -		signed long displ;
> -		/* It's a branch/branch link: instead of trying to
> -		 * figure out whether the branch will be taken or not,
> -		 * we'll put a breakpoint at both locations.  This is
> -		 * simpler, more reliable, and probably not a whole lot
> -		 * slower than the alternative approach of emulating the
> -		 * branch.
> -		 */
> -		displ = (insn & 0x00ffffff) << 8;
> -		displ = (displ >> 6) + 8;
> -		if (displ != 0 && displ != 4)
> -			alt = pc + displ;
> -	    }
> -	    break;
> -	}
> -
> -	return alt;
> -}
> -
> -static int
> -swap_insn(struct task_struct *task, unsigned long addr,
> -	  void *old_insn, void *new_insn, int size)
> -{
> -	int ret;
> -
> -	ret = access_process_vm(task, addr, old_insn, size, 0);
> -	if (ret == size)
> -		ret = access_process_vm(task, addr, new_insn, size, 1);
> -	return ret;
> -}
> -
> -static void
> -add_breakpoint(struct task_struct *task, struct debug_info *dbg, unsigned long addr)
> -{
> -	int nr = dbg->nsaved;
> -
> -	if (nr < 2) {
> -		u32 new_insn = BREAKINST_ARM;
> -		int res;
> -
> -		res = swap_insn(task, addr, &dbg->bp[nr].insn, &new_insn, 4);
> -
> -		if (res == 4) {
> -			dbg->bp[nr].address = addr;
> -			dbg->nsaved += 1;
> -		}
> -	} else
> -		printk(KERN_ERR "ptrace: too many breakpoints\n");
> -}
> -
> -/*
> - * Clear one breakpoint in the user program.  We copy what the hardware
> - * does and use bit 0 of the address to indicate whether this is a Thumb
> - * breakpoint or an ARM breakpoint.
> - */
> -static void clear_breakpoint(struct task_struct *task, struct debug_entry *bp)
> -{
> -	unsigned long addr = bp->address;
> -	union debug_insn old_insn;
> -	int ret;
> -
> -	if (addr & 1) {
> -		ret = swap_insn(task, addr & ~1, &old_insn.thumb,
> -				&bp->insn.thumb, 2);
> -
> -		if (ret != 2 || old_insn.thumb != BREAKINST_THUMB)
> -			printk(KERN_ERR "%s:%d: corrupted Thumb breakpoint at "
> -				"0x%08lx (0x%04x)\n", task->comm,
> -				task_pid_nr(task), addr, old_insn.thumb);
> -	} else {
> -		ret = swap_insn(task, addr & ~3, &old_insn.arm,
> -				&bp->insn.arm, 4);
> -
> -		if (ret != 4 || old_insn.arm != BREAKINST_ARM)
> -			printk(KERN_ERR "%s:%d: corrupted ARM breakpoint at "
> -				"0x%08lx (0x%08x)\n", task->comm,
> -				task_pid_nr(task), addr, old_insn.arm);
> -	}
> -}
> -
> -void ptrace_set_bpt(struct task_struct *child)
> -{
> -	struct pt_regs *regs;
> -	unsigned long pc;
> -	u32 insn;
> -	int res;
> -
> -	regs = task_pt_regs(child);
> -	pc = instruction_pointer(regs);
> -
> -	if (thumb_mode(regs)) {
> -		printk(KERN_WARNING "ptrace: can't handle thumb mode\n");
> -		return;
> -	}
> -
> -	res = read_instr(child, pc, &insn);
> -	if (!res) {
> -		struct debug_info *dbg = &child->thread.debug;
> -		unsigned long alt;
> -
> -		dbg->nsaved = 0;
> -
> -		alt = get_branch_address(child, pc, insn);
> -		if (alt)
> -			add_breakpoint(child, dbg, alt);
> -
> -		/*
> -		 * Note that we ignore the result of setting the above
> -		 * breakpoint since it may fail.  When it does, this is
> -		 * not so much an error, but a forewarning that we may
> -		 * be receiving a prefetch abort shortly.
> -		 *
> -		 * If we don't set this breakpoint here, then we can
> -		 * lose control of the thread during single stepping.
> -		 */
> -		if (!alt || predicate(insn) != PREDICATE_ALWAYS)
> -			add_breakpoint(child, dbg, pc + 4);
> -	}
> -}
> -
> -/*
> - * Ensure no single-step breakpoint is pending.  Returns non-zero
> - * value if child was being single-stepped.
> - */
> -void ptrace_cancel_bpt(struct task_struct *child)
> -{
> -	int i, nsaved = child->thread.debug.nsaved;
> -
> -	child->thread.debug.nsaved = 0;
> -
> -	if (nsaved > 2) {
> -		printk("ptrace_cancel_bpt: bogus nsaved: %d!\n", nsaved);
> -		nsaved = 2;
> -	}
> -
> -	for (i = 0; i < nsaved; i++)
> -		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)
>  {
> -	user_disable_single_step(child);
> +	/* Nothing to do. */
>  }
>  
>  /*
> @@ -576,8 +197,6 @@ void ptrace_break(struct task_struct *tsk, struct pt_regs *regs)
>  {
>  	siginfo_t info;
>  
> -	ptrace_cancel_bpt(tsk);
> -
>  	info.si_signo = SIGTRAP;
>  	info.si_errno = 0;
>  	info.si_code  = TRAP_BRKPT;
> diff --git a/arch/arm/kernel/ptrace.h b/arch/arm/kernel/ptrace.h
> deleted file mode 100644
> index 3926605..0000000
> --- a/arch/arm/kernel/ptrace.h
> +++ /dev/null
> @@ -1,37 +0,0 @@
> -/*
> - *  linux/arch/arm/kernel/ptrace.h
> - *
> - *  Copyright (C) 2000-2003 Russell King
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - */
> -#include <linux/ptrace.h>
> -
> -extern void ptrace_cancel_bpt(struct task_struct *);
> -extern void ptrace_set_bpt(struct task_struct *);
> -extern void ptrace_break(struct task_struct *, struct pt_regs *);
> -
> -/*
> - * Send SIGTRAP if we're single-stepping
> - */
> -static inline void single_step_trap(struct task_struct *task)
> -{
> -	if (task->ptrace & PT_SINGLESTEP) {
> -		ptrace_cancel_bpt(task);
> -		send_sig(SIGTRAP, task, 1);
> -	}
> -}
> -
> -static inline void single_step_clear(struct task_struct *task)
> -{
> -	if (task->ptrace & PT_SINGLESTEP)
> -		ptrace_cancel_bpt(task);
> -}
> -
> -static inline void single_step_set(struct task_struct *task)
> -{
> -	if (task->ptrace & PT_SINGLESTEP)
> -		ptrace_set_bpt(task);
> -}
> diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> index 907d5a6..7709668 100644
> --- a/arch/arm/kernel/signal.c
> +++ b/arch/arm/kernel/signal.c
> @@ -20,7 +20,6 @@
>  #include <asm/unistd.h>
>  #include <asm/vfp.h>
>  
> -#include "ptrace.h"
>  #include "signal.h"
>  
>  #define _BLOCKABLE (~(sigmask(SIGKILL) | sigmask(SIGSTOP)))
> @@ -348,8 +347,6 @@ asmlinkage int sys_sigreturn(struct pt_regs *regs)
>  	if (restore_sigframe(regs, frame))
>  		goto badframe;
>  
> -	single_step_trap(current);
> -
>  	return regs->ARM_r0;
>  
>  badframe:
> @@ -383,8 +380,6 @@ asmlinkage int sys_rt_sigreturn(struct pt_regs *regs)
>  	if (do_sigaltstack(&frame->sig.uc.uc_stack, NULL, regs->ARM_sp) == -EFAULT)
>  		goto badframe;
>  
> -	single_step_trap(current);
> -
>  	return regs->ARM_r0;
>  
>  badframe:
> @@ -704,8 +699,6 @@ static void do_signal(struct pt_regs *regs, int syscall)
>  	if (try_to_freeze())
>  		goto no_signal;
>  
> -	single_step_clear(current);
> -
>  	signr = get_signal_to_deliver(&info, &ka, regs, NULL);
>  	if (signr > 0) {
>  		sigset_t *oldset;
> @@ -724,7 +717,6 @@ static void do_signal(struct pt_regs *regs, int syscall)
>  			if (test_thread_flag(TIF_RESTORE_SIGMASK))
>  				clear_thread_flag(TIF_RESTORE_SIGMASK);
>  		}
> -		single_step_set(current);
>  		return;
>  	}
>  
> @@ -770,7 +762,6 @@ static void do_signal(struct pt_regs *regs, int syscall)
>  			sigprocmask(SIG_SETMASK, &current->saved_sigmask, NULL);
>  		}
>  	}
> -	single_step_set(current);
>  }
>  
>  asmlinkage void
> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> index ee57640..439e107 100644
> --- a/arch/arm/kernel/traps.c
> +++ b/arch/arm/kernel/traps.c
> @@ -23,6 +23,7 @@
>  #include <linux/kexec.h>
>  #include <linux/delay.h>
>  #include <linux/init.h>
> +#include <linux/sched.h>
>  
>  #include <asm/atomic.h>
>  #include <asm/cacheflush.h>
> @@ -32,7 +33,6 @@
>  #include <asm/unwind.h>
>  #include <asm/tls.h>
>  
> -#include "ptrace.h"
>  #include "signal.h"
>  
>  static const char *handler[]= { "prefetch abort", "data abort", "address exception", "interrupt" };
> -- 
> 1.7.0.4
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 



More information about the linux-arm-kernel mailing list