Re: [PATCH 0/1] Was: utrace-cleanup branch
On 11/12, Oleg Nesterov wrote: On 11/12, Roland McGrath wrote: I did some tweaks that I think address the several things you've raised. But I didn't try to reply point by point. I've merged everything up now, so the utrace-cleanup branch is gone. Please review the current code and post about anything we still need to fix. Great, thanks. I will definitely read the changes in utrace.c, if nothing else this is interesting to me. But for now, until utrace-ptrace is updated/finished, I will assume it is correct and doesn't need any further changes. (except the SIGTRAP changes in utrace_report_syscall_exit() we are discussing in another thread) Roland, I pulled the last changes in your tree (utrace-cleanup merged in utrace-ptrace) and did some testing. In short: utrace-ptrace does not work at all. It fails a lot (if not most) of tests, in particular sa-resethand-on-cont-signal hangs and clone-multi-ptrace silently crashes the kernel. The quick investigation didn't help, utrace.c has too much changes, I will read the code tomorrow from the very beginning. Today I am going to pretend utrace works and do the discussed changes in utrace-ptrace, then try to fix utrace. Oleg.
Re: [PATCH 127] move ptrace_resume()-send_sigtrap() logic into ptrace_report_signal()
Nothing wrong (I think), but this is more complex and implies more unnecessary work. You mean the code path taken is longer. But the code's logic remains simple. If ptrace_report_signal() sends the trap, we actually send the signal twice. Firstly ptrace_resume() does utrace_control(UTRACE_INTERRUPT) (ok, this is not the signal sending but sort of), then -report_signal() notices we need the trap and send the real signal, then we return to get_signal_to_deliver() which calls utrace_get_signal() again, it dequeues SIGTRAP and calls ptrace_report_signal() again to finally report SIGTRAP to the tracer. I don't think this extra step is particuarly costly given the context. But: I am not sure yet, but I think the current code is not optimal for that, it was written to report *info without force_sig_info(). I don't really follow that bit. But I'll just see what new code you come up with. So, lets revert this change for now to be compatible. This change is simple and can be done again at any time, probably along with upstream change. Ok. Thanks, Roland
[PATCH 130] revert turn PTRACE_EVENT_SIGTRAP into PTRACE_EVENT_SIGNAL
By the previous discussion, revert 603e19c41ba5c97e48a25543c63c081c5fe64137
for now.
But keep WARN_ON(resume != XXX_STEP), it may help.
---
kernel/ptrace.c | 14 ++
1 file changed, 6 insertions(+), 8 deletions(-)
--- PU/kernel/ptrace.c~130_REVERT_KILL_PTRACE_EVENT_SIGTRAP 2009-11-02
06:54:49.0 +0100
+++ PU/kernel/ptrace.c 2009-11-13 22:34:23.0 +0100
@@ -73,7 +73,8 @@ struct ptrace_context {
#define PTRACE_EVENT_SYSCALL_ENTRY (1 16)
#define PTRACE_EVENT_SYSCALL_EXIT (2 16)
-#define PTRACE_EVENT_SIGNAL(3 16)
+#define PTRACE_EVENT_SIGTRAP (3 16)
+#define PTRACE_EVENT_SIGNAL(4 16)
/* events visible to user-space */
#define PTRACE_EVENT_MASK 0x
@@ -526,10 +527,10 @@ static u32 ptrace_report_signal(u32 acti
if (resume != UTRACE_RESUME) {
WARN_ON(resume != UTRACE_BLOCKSTEP
resume != UTRACE_SINGLESTEP);
- WARN_ON(ctx-signr);
- ctx-signr = SIGTRAP;
+
+ set_stop_code(ctx, PTRACE_EVENT_SIGTRAP);
+ return UTRACE_STOP | UTRACE_SIGNAL_IGN;
}
- /* fallthrough */
case UTRACE_SIGNAL_REPORT:
if (!ctx-siginfo) {
@@ -537,10 +538,7 @@ static u32 ptrace_report_signal(u32 acti
return resume | UTRACE_SIGNAL_IGN;
WARN_ON(ctx-signr != SIGTRAP);
- /*
-* set by ptrace_resume(PTRACE_EVENT_SYSCALL_EXIT) or
-* by UTRACE_SIGNAL_HANDLER above.
-*/
+ /* set by ptrace_resume(PTRACE_EVENT_SYSCALL_EXIT) */
fill_sigtrap_info(task, info);
break;
}
[PATCH 131] don't send the unnecessary SIGTRAP after SYSCALL_EXIT
PTRACE_SINGLESTEP after syscall-exit shouldn't trigger the trap before
return to user-mode. This was the x86 spicific oddity which is already
fixed in -mm by
ptrace-x86-change-syscall_trace_leave-to-rely-on-tracehook-when-stepping.patch
---
kernel/ptrace.c |6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
--- PU/kernel/ptrace.c~131_SYSCALL_EXIT_DONT_TRAP 2009-11-13
22:34:23.0 +0100
+++ PU/kernel/ptrace.c 2009-11-13 22:35:40.0 +0100
@@ -940,10 +940,7 @@ static int ptrace_resume(struct task_str
do_ptrace_notify_stop(ctx, tracee);
return 0;
}
- /* fallthrough, but suppress send_sig_info() below */
- data = 0;
- case PTRACE_EVENT_SYSCALL_EXIT:
if (action != UTRACE_RESUME) {
/*
* single-stepping. UTRACE_SIGNAL_REPORT will
@@ -952,8 +949,9 @@ static int ptrace_resume(struct task_str
ctx-signr = SIGTRAP;
action = UTRACE_INTERRUPT;
}
- /* fallthrough */
+ break;
+ case PTRACE_EVENT_SYSCALL_EXIT:
case PTRACE_EVENT_SYSCALL_ENTRY:
if (data)
send_sig_info(data, SEND_SIG_PRIV, tracee);
[PATCH 132] change ptrace_report_signal() to use user_single_step_siginfo()
Change ptrace_report_signal() to report SIGTRAP via
user_single_step_siginfo() + force_sig_info().
---
kernel/ptrace.c | 24 +---
1 file changed, 9 insertions(+), 15 deletions(-)
--- PU/kernel/ptrace.c~132_USER_SINGLE_STEP_INFO2009-11-13
22:35:40.0 +0100
+++ PU/kernel/ptrace.c 2009-11-13 22:44:10.0 +0100
@@ -451,16 +451,10 @@ static u32 ptrace_report_exec(enum utrac
return UTRACE_STOP;
}
-static void fill_sigtrap_info(struct task_struct *task, siginfo_t *info)
+static void ptrace_send_sigtrap(struct task_struct *task, siginfo_t *info)
{
-#ifdef CONFIG_X86
- task-thread.trap_no = 1;
- task-thread.error_code = 0;
-#endif
- memset(info, 0, sizeof(*info));
- info-si_signo = SIGTRAP;
- info-si_code = TRAP_BRKPT;
- info-si_addr = (void __user*)KSTK_EIP(task);
+ user_single_step_siginfo(task, task_pt_regs(task), info);
+ force_sig_info(SIGTRAP, info, task);
}
static enum utrace_signal_action resume_signal(struct task_struct *task,
@@ -534,13 +528,13 @@ static u32 ptrace_report_signal(u32 acti
case UTRACE_SIGNAL_REPORT:
if (!ctx-siginfo) {
- if (!ctx-signr)
- return resume | UTRACE_SIGNAL_IGN;
+ if (ctx-signr) {
+ /* set by ptrace_resume(SYSCALL_EXIT) */
+ WARN_ON(ctx-signr != SIGTRAP);
+ ptrace_send_sigtrap(task, info);
+ }
- WARN_ON(ctx-signr != SIGTRAP);
- /* set by ptrace_resume(PTRACE_EVENT_SYSCALL_EXIT) */
- fill_sigtrap_info(task, info);
- break;
+ return resume | UTRACE_SIGNAL_IGN;
}
if (WARN_ON(ctx-siginfo != info))
