ARM: ptrace: remove single-step emulation code

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.

---
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

index 67357ba..b439b41 100644
@@ -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

index 783d50f..a8ff22b 100644
@@ -130,8 +130,6 @@ struct pt_regs {
 
 #ifdef __KERNEL__
 
-#define arch_has_single_step()	(1)
-
 #define user_mode(regs)	\
 	(((regs)->ARM_cpsr & 0xf) == 0)
 

index 1b960d5..f90756d 100644
@@ -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;
 

deleted file mode 100644
index 3926605..0000000
@@ -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);
-}

index 907d5a6..7709668 100644
@@ -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

index ee57640..1f3f0e8 100644
@@ -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

Have you checked whether strace and ltrace use single stepping?

Hi Russell,

strace works fine with this patch applied and, looking at the sources, it doesn't use the SINGLESTEP request. As for ltrace, it *does* use SINGLESTEP but it can use PTRACE_SYSCALL instead (indeed, it does this for sparc, ia64 and mips). ltrace doesn't have code for checking the ptrace return value so I'd say it's their bug.

Will

Hi,

afair, the current way to prevent SINGLESTEP usage in ltrace is to modify some #ifdef. So, while I agree that not checking ptrace return value is not nice, it has nothing to do with SINGLESTEP removal as this call will not get compiled in. What matters is rather to know if things are still working once the #ifdef are changed and if they're not, finding if it's a bug in ltrace or kernel.

Arnaud

Arnaud,

Not true. A PTRACE_SINGLESTEP request will simply return -EIO. Userspace programs should check the return value anyway because it could fail and then handle the failure gracefully (in the case of ltrace, by trying a PTRACE_SYSCALL request). This also means that ltrace is currently broken on Thumb, where PTRACE_SINGLESTEP will return an error code.

Will

Very few people use it since it does not work :-)

I tried to use PTRACE_SINGLESTEP on ARM last year but pretty soon noticed that it doesn't work with user helpers. I sent a patch

http://lists.infradead.org/pipermail/linux-arm-kernel/2010-November/030832.html

and added ARM support to my really simple instruction level execution tracing tool

http://iki.fi/lindi/git/itrace.git/

originally written mainly for my own debugging needs. The major limitation of valgrind is that it can't attach to an already running process and I guess it's too resource intensive to run my processes constantly under valgrind on an embedded system.

I agree that decoding ARM instruction in kernel space is really funky. Perhaps my best be would be to copy the old kernel code to my own userland tool and use PTRACE_POKETEXT to set breakpoints? The only drawbacks I see are:

1) I need more syscalls per instruction: PTRACE_GETREGS + PTRACE_SINGLESTEP vs. PTRACE_GETREGS + PTRACE_PEEKTEXT + PTRACE_POKETEXT * (number of potential branch targets) + PTRACE_CONTINUE but I guess I can live with this.

2) itrace does not know where user helpers are. Parsing /proc/config.gz at runtime for CONFIG_VECTORS_BASE is probably not a good idea. If this location does not change often it is not a problem to hardcode it in itrace.

I was going to say that GDB does not work either with user helpers but it seems that in

commit 52d6c8167d4e91d89bc5c26cf0bacc2200272f96

Author: Julian Brown <[email protected]>
Date:   Thu Jul 30 23:05:00 2009 +0000

the function arm_catch_kernel_helper_return was added to GDB. They hard code 0xffff0000 but I guess that is ok?

Hi Timo,

You could copy the old code, but it may need updating to support ARMv7. It also doesn't handle Thumb instructions as it stands.

Ok.

I think it's always mapped at the high address if you have an MMU, so you might need to detect the uclinux case.

Will