> IOW, I am asking you to apply my patch for now and revert your
> change to have the working tree, then discuss the "right" fix.
Ok, done.
> Once again. The tracee sleeps in SYSCALL_ENTER. The tracer resumes
> the tracee via utrace_control(UTRACE_SINGLESTEP).
Right. This is another strange corner. It doesn't necessarily make
especially good sense intrinsically for single-step to have this meaning
from this place. But it's the status quo in the ptrace interface.
In the pure utrace context, stepping and syscall tracing are independent
moving parts as they aren't in ptrace. So one could say it makes sense for
a single-step that precedes the syscall-entry report to apply to "that
instruction", meaning the syscall instruction. This clearly makes sense
when it's one engine doing the step and another unrelated one doing the
syscall tracing. But then the utrace idiom is that the stepping engine
gets report_quiesce(SYSCALL_ENTRY) and returns UTRACE_SINGLESTEP to
preserve its "what to do next" choice from before--and this has the right
results anyway.
> By the time the resumed tracee passes tracehook_report_syscall_exit()
> step == F, utrace_control() does not set TIF_SINGLESTEP.
In old ptrace, a PTRACE_SINGLESTEP resuming from the syscall-entry stop
does do user_enable_single_step() just like any other resume. So today we
do get there with that flag set. The utrace equivalent would be
finish_resume_report() in utrace_report_syscall_entry(). But that would
have no other use except for this quirk (none that I can see).
> So, I think we should do something like
>
> tracehook_report_syscall_exit(step)
>
> // do not use step at all
> if (task_utrace_flags() != 0)
> utrace_report_syscall_exit();
The trouble here is that the arch code looks like this:
step = test_thread_flag(TIF_SINGLESTEP);
if (step || test_thread_flag(TIF_SYSCALL_TRACE))
tracehook_report_syscall_exit(regs, step);
i.e. unless syscall tracing is enabled, we won't even get there at all.
> utrace_report_syscall_exit()
>
> if (UTRACE_EVENT(SYSCALL_EXIT))
> REPORT(report_syscall_exit);
>
> if (utrace->resume == UTRACE_SINGLESTEP ||
> utrace->resume == UTRACE_BLOCKSTEP)
> send_sigtrap();
>
> What do you think?
This can't work in the REPORT case. There start_report() will have reset
utrace->resume, so you are only seeing the value set by report_syscall_exit
callbacks just made. That has the behavior of the old x86 code rather than
what we want now. You'd have to cache the value before REPORT,
e.g. start_report() return value as in utrace_resume(), and use that.
But if (as now) we need the step flag to be set in these cases to be sure
we got called at all, then might as well just use that. So what about this?
(This would go with un-reverting f19442c and reverting 3bd4be9.)
Thanks,
Roland
=====
diff --git a/kernel/utrace.c b/kernel/utrace.c
index 5af0f94..0000000 100644
--- a/kernel/utrace.c
+++ b/kernel/utrace.c
@@ -1584,20 +1584,25 @@ static inline u32 do_report_syscall_entr
UTRACE_EVENT(SYSCALL_ENTRY), report_syscall_entry,
resume_report | report->result | report->action,
engine, current, regs);
- finish_report(task, utrace, report, false);
+
+ /*
+ * Now we'll stop if asked. If we're proceeding with the syscall,
+ * now we'll first enable single-step if asked. That leaves the
+ * task in "stepping" state so tracehook_report_syscall_exit() and
+ * the arch code that calls it will know.
+ */
+ finish_resume_report(task, utrace, report);
if (report->action != UTRACE_STOP)
return 0;
- utrace_stop(task, utrace, report->resume_action);
-
if (fatal_signal_pending(task)) {
/*
* We are continuing despite UTRACE_STOP because of a
* SIGKILL. Don't let the system call actually proceed.
*/
report->result = UTRACE_SYSCALL_ABORT;
- } else if (utrace->resume == UTRACE_REPORT) {
+ } else if (utrace->resume <= UTRACE_REPORT) {
/*
* If we've been asked for another report after our stop,
* go back to report (and maybe stop) again before we run
