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

Will Deacon will.deacon at arm.com
Wed Jan 19 10:07:15 EST 2011


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 nobody uses it anyway.
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.

Signed-off-by: Will Deacon <will.deacon at arm.com>
---
I'm posting this as an RFC to see if anybody has a good reason to keep this
code around. There's also a chance I've missed an opportunity to remove some
related code, but I think I found everything. Tested on a Versatile Express,
single-stepping in GDB worked fine.

 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          |    1 -
 7 files changed, 2 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..1f3f0e8 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -32,7 +32,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





More information about the linux-arm-kernel mailing list