Sorry for late reply.
I delayed it because I hoped I will see how to solve this problem
cleanly during the rewrite.
No. I don't see how we can fix this without some changes in utrace.
Just in case, let me remind you what is the problem. For simplicity,
let's ignore SIGKILL issues, and let's discuss this particular case:
a PTRACE_O_TRACESYSGOOD tracee reports SYSCALL_ENTRY and stops.
the tracer does
ptrace(PTRACE_SYSCALL);
// we must have SYSCALL_EXIT report
waitpid(&state);
assert(state == 0x857F); <--- 1
assert(ptrace(PTRACE_GETEVENTMSG) == 0); <--- 2
This must always work, both 1 and 2 asserts are valid. (but, for the
moment, please forget about 2).
However, with utrace-ptrace the tracee can notice ->group_stop_count and
call do_signal_stop() before it has a chance to do finish_resume_report()
and actually stop. This breaks the 1st assert.
On 09/18, Roland McGrath wrote:
>
> I'll point out that the only symptom that matters to ptrace is ->exit_code
> and that issue disappears if you stop overloading it for ptrace purposes,
Firstly, I agree of course, in the long term ptrace should not use
task_struct->exit_code. But this doesn't matter at all, please see
below.
> In this scenario, the event has already happened and that's externally
> visible (the child exists). (In the equivalent case for exec, you can
> tell from /proc/pid/exe, the effects of setuid have happened, etc.)
> We don't have the option of saying that the group-stop happened first.
> If that were so, this event would not have happened yet, but it's
> already been both reported via utrace and become observable by
> userland. So it should appear as if the other thread started its
> group-stop after we were already in TASK_TRACED.
Agreed, I thought about this too. IOW, we can just silently drop this
JCTL stop event and report SYSCALL_EXIT instead.
So, we add ptrace_report_jctl(notify) which does
if (notify == CLD_STOPPED) {
if (get_stop_event(context)) {
// pretend we we already stopped when another
// thread initiated this group-stop, set the
// correct ->exit_code and return, our tracer
// will be notified by do_signal_stop()
task->exit_code = context->stop_code &
PTRACE_EVENT_MASK;
return;
}
}
(yes, this can't work because ->exit_code will be overwritten by
do_signal_stop() but this is very trivial to fix).
Unfortunately, this doesn't help. Because the tracee will stop in
TASK_STOPPED state, and this means SIGCONT can break the 2nd assert
above.
This is the problem. Since do_signal_stop() sets TASK_STOPPED, we
can't report anything but JCTL event. And it is not possible to
hack do_signal_stop() so that it sets TASK_TRACED if the debugger
has changed task->exit_code (or context->exit_code or whatever).
> In other versions of this race, we'll already be in utrace_stop() or
> on the way to it, and hit "we must participate in the bookkeeping."
> But the bookkeeping is not really enough. In the case where we were
> the last thread we do set SIGNAL_STOP_STOPPED, but we swallow the
> parent's notification.
Yes, but this is another story. And the current (upstream) implementation
is not better, lets forget about this problem for now.
> Hmm, perhaps ->exit_code is only ever set now
> because of ptrace, at least since your task_stopped_code() changes?
> Perhaps we should just nix setting it in do_signal_stop() too (after
> new ptrace).
Yes. But: please don't forget the tracer can attach after the tracee
does do_signal_stop(). Again, this is another story, lets discuss
this later.
> Now, back to your case, a different ordering of that same race.
>
> There is nothing really wrong with having utrace's hook be before the
> group_stop_count check in get_signal_to_deliver(). But that would
> require changing the tracehook interface there, and the existing
> tracehook_get_signal() seems about as clean as we can make it.
Please see the patches. They are very simple.
Perhaps, it makes sense to do more. We can introduce the new
UTRACE_SIGNAL_JCTL utrace_signal_action, but I don't think it is
really needed.
Yes, the debugger can, say, fill *info and return UTRACE_SIGNAL_DELIVER
while ->group_stop_count != 0, but a) I think this is harmless and b)
can happen anyway (see the changelog for 2nd patch).
What do you think?
> We can
> change tracehook_notify_jctl()
> ...
> If (after the report_jctl
> pass if any) anybody wants UTRACE_STOP, then we handle it with
> utrace_stop() right there
Yes, I considered this option too. Imho, not nice.
This needs more changes and afaics doesn't have any advantages compared
to the patch I am sending. But more importantly, I believe this change will
complicate the interaction between do_signal_stop() and utrace, and this
interaction becomes even more unobvious.
Oleg.
