> If we always set the single step bit in the PTRACE_SINGLESTEP call and
> have a signal to deliver (data!=0) then the we will do a SIGTRAP and at
> the same time the single step bit would be set. To make it perfect we
> should clear the bit in this case again. I'd probably add the
> ptrace_notify(SIGTRAP) + clear_single_step calls to handle_signal /
> handle_signal32.
I don't see any reason to clear per_info.single_step in this case.
The thread is stopped at the handler entry point. Resuming it will
always explicitly set or clear single-step, i.e. PTRACE_SINGLESTEP
sets and PTRACE_CONT et al (including ptrace_disable for sudden
disconnection) clear it. It's no different from a normal single-step
stop--the bit remains set then too, doesn't it?
Since you already have the TIF_SINGLE_STEP behavior on the way back to
user mode, I think it's preferable just to set TIF_SINGLE_STEP in
handle_signal. (ptrace_notify is a bit of a kludge, and by setting
TIF_SINGLE_STEP you are making it exactly like a normal single-step
trap from the userland perspective.)
> Somelike like the patch below ?
That patch doesn't make sense to me. It tests TIF_SINGLE_STEP in
handler setup, but that won't be set for the case of PTRACE_SINGLESTEP
with a handled signal. I think what handler setup wants is:
if (current->thread.per_info.single_step)
set_thread_flag(TIF_SINGLE_STEP);
If I'm not mistaken, the patch below (wholly untested) is about right.
I renamed the functions to the (new) canonical names and added the
arch_has_single_step macro so that after my ptrace_request changes
(now in -mm) land, s390 can drop its PTRACE_{CONT,SINGLESTEP,etc} code
altogether and use the new generic code in ptrace_request.
Thanks,
Roland
---
[PATCH] s390 single-step cleanup
Signed-off-by: Roland McGrath <roland@xxxxxxxxxx>
---
diff --git a/arch/s390/kernel/ptrace.c b/arch/s390/kernel/ptrace.c
index 1d81bf9..0000000 100644
--- a/arch/s390/kernel/ptrace.c
+++ b/arch/s390/kernel/ptrace.c
@@ -86,13 +86,13 @@ FixPerRegisters(struct task_struct *task
per_info->control_regs.bits.storage_alt_space_ctl = 0;
}
-static void set_single_step(struct task_struct *task)
+void user_enable_single_step(struct task_struct *task)
{
task->thread.per_info.single_step = 1;
FixPerRegisters(task);
}
-static void clear_single_step(struct task_struct *task)
+void user_disable_single_step(struct task_struct *task)
{
task->thread.per_info.single_step = 0;
FixPerRegisters(task);
@@ -107,7 +107,7 @@ void
ptrace_disable(struct task_struct *child)
{
/* make sure the single step bit is not set. */
- clear_single_step(child);
+ user_disable_single_step(child);
}
#ifndef CONFIG_64BIT
@@ -651,7 +651,7 @@ do_ptrace(struct task_struct *child, lon
clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
child->exit_code = data;
/* make sure the single step bit is not set. */
- clear_single_step(child);
+ user_disable_single_step(child);
wake_up_process(child);
return 0;
@@ -665,7 +665,7 @@ do_ptrace(struct task_struct *child, lon
return 0;
child->exit_code = SIGKILL;
/* make sure the single step bit is not set. */
- clear_single_step(child);
+ user_disable_single_step(child);
wake_up_process(child);
return 0;
@@ -675,10 +675,7 @@ do_ptrace(struct task_struct *child, lon
return -EIO;
clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
child->exit_code = data;
- if (data)
- set_tsk_thread_flag(child, TIF_SINGLE_STEP);
- else
- set_single_step(child);
+ user_enable_single_step(child);
/* give it a chance to run. */
wake_up_process(child);
return 0;
diff --git a/arch/s390/kernel/signal.c b/arch/s390/kernel/signal.c
index d264671..0000000 100644
--- a/arch/s390/kernel/signal.c
+++ b/arch/s390/kernel/signal.c
@@ -471,6 +471,7 @@ void do_signal(struct pt_regs *regs)
if (signr > 0) {
/* Whee! Actually deliver the signal. */
+ int ret;
#ifdef CONFIG_COMPAT
if (test_thread_flag(TIF_31BIT)) {
extern int handle_signal32(unsigned long sig,
@@ -478,15 +479,12 @@ void do_signal(struct pt_regs *regs)
siginfo_t *info,
sigset_t *oldset,
struct pt_regs *regs);
- if (handle_signal32(
- signr, &ka, &info, oldset, regs) == 0) {
- if (test_thread_flag(TIF_RESTORE_SIGMASK))
- clear_thread_flag(TIF_RESTORE_SIGMASK);
- }
- return;
+ ret = handle_signal32(signr, &ka, &info, oldset, regs);
}
+ else
#endif
- if (handle_signal(signr, &ka, &info, oldset, regs) == 0) {
+ ret = handle_signal(signr, &ka, &info, oldset, regs);
+ if (!ret) {
/*
* A signal was successfully delivered; the saved
* sigmask will have been stored in the signal frame,
@@ -495,6 +493,14 @@ void do_signal(struct pt_regs *regs)
*/
if (test_thread_flag(TIF_RESTORE_SIGMASK))
clear_thread_flag(TIF_RESTORE_SIGMASK);
+
+ /*
+ * If we would have taken a single-step trap
+ * for a normal instruction, act like we took
+ * one for the handler setup.
+ */
+ if (current->thread.per_info.single_step)
+ set_thread_flag(TIF_SINGLE_STEP);
}
return;
}
diff --git a/include/asm-s390/ptrace.h b/include/asm-s390/ptrace.h
index 332ee73..0000000 100644
--- a/include/asm-s390/ptrace.h
+++ b/include/asm-s390/ptrace.h
@@ -465,6 +465,13 @@ struct user_regs_struct
#ifdef __KERNEL__
#define __ARCH_SYS_PTRACE 1
+/*
+ * 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 *);
+
#define user_mode(regs) (((regs)->psw.mask & PSW_MASK_PSTATE) != 0)
#define instruction_pointer(regs) ((regs)->psw.addr & PSW_ADDR_INSN)
#define regs_return_value(regs)((regs)->gprs[2])
-
To unsubscribe from this list: send the line "unsubscribe linux-s390" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
