Re: upstream/utrace: copy_process() TIF_SINGLESTEP
You are right. I added step-fork to ptrace-tests for this. The place that should do this is arch/*:copy_thread. TIF_* bits are arch implementation details. x86 and powerpc both have a TIF_SINGLESTEP that should be cleared, and others might too. Each arch maintainer should check if their implementation has this issue. This is a pure arch bug that has nothing in particular to do with utrace. It should impact us no differently than it does upstream. So just take it there as an arch-specific issue for all arch maintainers to check on. Thanks, Roland
Re: [PATCH 127] move ptrace_resume()-send_sigtrap() logic into ptrace_report_signal()
- it sets task-thread.trap_no/error_code under CONFIG_X86, what should it do in the #else case? This can't be this way. It has to be a proper arch hook of some kind. - it sets info-si_addr = KSTK_EIP() which doesn't check user_mode_vm(). Hopefully this is OK? Ditto. The si_code/si_addr settings are altogether arch-specific. As long as you are using x86 details with #ifdef for hack drafts, just copy the exact details used in x86's send_sigtrap. - with or without this patch utrace-ptrace assumes that PTRACE_SINGLESTEP after PTRACE_EVENT_SYSCALL_EXIT needs fill_sigtrap_info()'ed signal, this may break or fix !x86 machines, and I don't know how to check. Perhaps ptrace_resume(PTRACE_EVENT_SYSCALL_EXIT) needs ifdef's. It should be easy enough to add a ptrace-tests case that distinguishes the two behaviors. I think we can then call the non-x86 behaviors broken and harmonize on the x86 behavior to make that test pass everywhere. - unlike send_sigtrap()-force_sig_info() we don't unblock SIGTRAP or reset the handler This is nicer for debugger things actually, but we don't have such niceness for real traps and won't soon. IMHO it is best to start with doing exactly what the old x86 code does, which is force_sig_info--which is also making all machines uniformly do what a real single-step trap does and is thus consistent in this PTRACE_SINGLESTEP case matching all others, which we have been saying is a sensible principle. these changes looks good (but see the next patch). However, any user-visible change is dangerous. Let's not roll it in. As well as it just being bad form to roll that into implementation detail changes, it makes things less consistent rather than more (on x86, anyway, and arguably across the board). Thanks, Roland
Sensational sales
Great variety of little helpers for your health. http://pef.pharmlydon43.com/
[PATCH 0/1] Was: utrace-cleanup branch
On 10/28, Roland McGrath wrote:
I've made a new branch, utrace-cleanup.
This forks from utrace-indirect and has:
26fefca utrace: sticky resume action
28b2774 utrace: remove -stopped field
I am not sure I understand the new code in details - too much changes.
Anyway, I can never understand the code after the first reading. At
first glance, imho this change is right in general but:
1. This breaks the current implementation of utrace-ptrace.
I guess I misunderstood you when we discussed this before, I thought
that the tracee should handle UTRACE_SINGLESTEP right after resume
and call user_enable_single_step(). However, enable_step() is called
at utrace_resume/utrace_get_signal time, this is too late for ptrace.
I guess this branch is the code which will be sent to lkml, right?
In this case utrace-cleanup should be merged into utrace-ptrace right
now, then I need to fix ptrace. Basically, ptrace_report_quiesce() and
ptrace_report_interrupt() should detect the case when the tracee was
resumed by PTRACE_XXXSTEP and then it passed syscall_trace_leave()
without TIF_SINGLESTEP, and synthesize a trap.
The main problem is that this is not consistent across the different
arch'es :[
2. Cosmetic, but the usage of utrace_task_alloc() looks a bit strange.
Why it returns bool, not struct utrace * ?
3. Now that we have utrace-resume, can't we kill report-resume_action ?
4. One of the changes in utrace_get_signal() doesn't look exactly right.
if (utrace-resume UTRACE_RESUME || utrace-signal_handler) {
...
if (resume UTRACE_REPORT) {
report.action = resume;
finish_resume_report(report);
}
Yes, we need finish_resume_report() here, but since report-takers
is not set, finish_report_reset() will always call utrace_reset().
OTOH, we can't set -takers before finish_resume_report(), we can
miss UTRACE_DETACH request. utrace_control(DETACH)-utrace_do_stop()
does not change utrace-resume != UTRACE_RESUME.
And since utrace_do_stop() never upgrades utrace-resume, we have
another problem: UTRACE_STOP request can be lost here.
5. utrace_resume() has the same problems. If report-action != REPORT
we do not call callbacks and finish_resume_report() is called with
-takers == F.
6. But! I think utrace_resume() was always wrong here. Because it
calls start_callback(events = 0) and thus we nevet set -takers.
7. utrace_attach_task() has an implicit wmb() between -utrace = new_utrace
and -utrace_flags = REAP, this is good.
But, for example, tracehook_force_sigpending() does not have rmb(),
this means utrace_interrupt_pending() can OOPS in theory.
8. Completely off-topic, but utrace_control() has a very strange comment
under case UTRACE_INTERRUPT,
* When it's stopped, we know it's always going
* through utrace_get_signal() and will recalculate.
can't recall if it were ever true, but surely it is not now?
Oleg.
[PATCH 1/1] re-introduce utrace_finish_stop() to fix the race with SIGKILL
A killed tracee should do nothing until the tracer drops utrace-lock.
Signed-off-by: Oleg Nesterov o...@redhat.com
---
include/linux/tracehook.h |2 ++
include/linux/utrace.h|2 ++
kernel/utrace.c | 16 +++-
3 files changed, 19 insertions(+), 1 deletion(-)
--- UTRACE/include/linux/tracehook.h~1_FINISH_STOP 2009-11-03
01:38:22.0 +0100
+++ UTRACE/include/linux/tracehook.h2009-11-03 05:38:49.0 +0100
@@ -531,6 +531,8 @@ static inline int tracehook_notify_jctl(
*/
static inline void tracehook_finish_jctl(void)
{
+ if (task_utrace_flags(current))
+ utrace_finish_stop();
}
#define DEATH_REAP -1
--- UTRACE/include/linux/utrace.h~1_FINISH_STOP 2009-11-03 01:38:22.0
+0100
+++ UTRACE/include/linux/utrace.h 2009-11-03 05:39:41.0 +0100
@@ -98,6 +98,8 @@ bool utrace_interrupt_pending(void)
__attribute__((weak));
void utrace_resume(struct task_struct *, struct pt_regs *)
__attribute__((weak));
+void utrace_finish_stop(void)
+ __attribute__((weak));
int utrace_get_signal(struct task_struct *, struct pt_regs *,
siginfo_t *, struct k_sigaction *)
__attribute__((weak));
--- UTRACE/kernel/utrace.c~1_FINISH_STOP2009-11-03 01:38:22.0
+0100
+++ UTRACE/kernel/utrace.c 2009-11-03 06:10:10.0 +0100
@@ -701,7 +701,7 @@ static bool utrace_do_stop(struct task_s
if (task_is_stopped(target)) {
/*
* Stopped is considered quiescent; when it wakes up, it will
-* go through utrace_finish_jctl() before doing anything else.
+* go through utrace_finish_stop() before doing anything else.
*/
spin_lock_irq(target-sighand-siglock);
if (likely(task_is_stopped(target)))
@@ -809,6 +809,18 @@ static bool utrace_reset(struct task_str
return !flags;
}
+void utrace_finish_stop(void)
+{
+ /*
+* If we were task_is_traced() and then SIGKILL'ed, make
+* sure we do nothing until the tracer drops utrace-lock.
+*/
+ if (unlikely(__fatal_signal_pending(current))) {
+ struct utrace *utrace = task_utrace_struct(current);
+ spin_unlock_wait(utrace-lock);
+ }
+}
+
/*
* Perform %UTRACE_STOP, i.e. block in TASK_TRACED until woken up.
* @task == current, @utrace == current-utrace, which is not locked.
@@ -875,6 +887,8 @@ relock:
schedule();
+ utrace_finish_stop();
+
/*
* While in TASK_TRACED, we were considered frozen enough.
* Now that we woke up, it's crucial if we're supposed to be
