Re: utrace_report_syscall_entry() finish_report()
On 11/11, Roland McGrath wrote: I saw you have utrace-syscall-resumed branch but never looked at it. For those not concerned with its purpose, there is one thing you must know. Every report_syscall_entry callback must do: if (utrace_syscall_action(action) == UTRACE_SYSCALL_RESUMED) return UTRACE_RESUME; to avoid being confused by the new extra callbacks. OK, thanks. Then I fix utrace-ptrace. I _think_ this will be easy too. ptrace_report_syscall_exit() can notice ctx-resume == UTRACE_SINGLESTEP and send the trap if utrace_control() doesn't set TIF_SINGLESTEP synchronously. (And this means with utrace tracehook_report_syscall_exit() can do nothing except utrace_report_syscall_exit()). Today we don't call into utrace unless UTRACE_EVENT(SYSCALL_EXIT) is set. So if nothing changes in the utrace layer, this means that ptrace will enable UTRACE_EVENT(SYSCALL_EXIT) (i.e. TIF_SYSCALL) for PTRACE_SINGLE*. Is that what you had in mind? Yes, exactly. There are two possible issues with doing it that way. The first is purely from a ptrace perspective. Today PTRACE_SINGLE* does not set TIF_SYSCALL. On x86 this doesn't really make a difference in the underlying code, because TIF_SINGLESTEP takes the same path. So probably this is fine. But on other machines I'm not entirely sure that TIF_SYSCALL won't take a slower path than single-step alone caused already for syscall entries. This surely pales in comparison to the overhead of the ptrace stop and context switch and all, but still, it could be a change. On x86 I'm not sure there's any reason not to optimize that better in the future, I see. The other issue comes from the utrace API perspective. Here it just seems entirely wrong to require the engine (ptrace or any other) to do any syscall-exit magic of its own to make UTRACE_SINGLESTEP work on a syscall insn like it does on other insns. The utrace layer should make that trap appear transparently like the normal traps seen for UTRACE_SINGLESTEP. So, we could have tracehook_report_syscall_exit send the signal as it does now upstream. But probably better is to do it all in the utrace layer, Indeed! i.e. pass the step flag to utrace_report_syscall_exit. The problem is, we can't use step flag. step == TIF_SINGLESTEP, and it can be unset despite the fact ptrace (or other engine) did utrace_control(UTRACE_SINGLESTEP) to resume. Simple example. The tracee stopped in syscall-entry. the tracer does PTRACE_SINGLESTEP. With the recent changes in utrace-cleanup utrace_control() doesn't set TIF_SINGLESTEP, and the tracee passes syscall_trace_leave() without TIF_SINGLESTEP. But I guess utrace_report_syscall_exit() can look at utrace-resume and notice UTRACE_SINGLESTEP ? That approximates the check that yesterday's x86 code does: if (test_thread_flag(TIF_SINGLESTEP) tracehook_consider_fatal_signal(current, SIGTRAP)) send_sigtrap(current, regs, 0, TRAP_BRKPT); But still we are queuing a real signal here. That is no different from the real hardware trap case, so perhaps that is the right thing to do for now. (Eventually we want to have a way for all these traps to avoid becoming real signals at all, but that is a subject for another day.) Like a real signal, that has the problem of still being queued if the tracer detaches immediately thereafter (so it gets delivered to the user). We could avoid that for this fake signal by just setting some flag that tells utrace_get_signal to synthesize it before dequeuing real signals. It's probably not worth bothering with that complexity now. Yes, I think we have more important problems ;) Oleg.
Re: utrace_report_syscall_entry() finish_report()
Simple example. The tracee stopped in syscall-entry. the tracer does PTRACE_SINGLESTEP. With the recent changes in utrace-cleanup utrace_control() doesn't set TIF_SINGLESTEP, and the tracee passes syscall_trace_leave() without TIF_SINGLESTEP. Ah, yes. Well, the point of the arch/tracehook tweaks was that the utrace layer has all the information and can decide what to do. But I guess utrace_report_syscall_exit() can look at utrace-resume and notice UTRACE_SINGLESTEP ? Yes. Unlike ptrace, at the utrace layer we can have both syscall-exit and single-step going on at the same time (even by unrelated engines). If some engine doesn't care about syscall-exit, it will still get report_quiesce there and should reassert UTRACE_SINGLESTEP with its return value. But that makes it a bit unclear whether a UTRACE_SINGLESTEP return from report_syscall_exit wants a fresh step after the next insn, or wants a synthetic trap immediately after the syscall insn. So I wonder if what might be right is to have utrace_report_syscall_exit look at -resume before its reporting. If it's single/block-step it can synthesize the trap there, so the signal is queued before the syscall-exit report is made. Thanks, Roland
Re: utrace_report_syscall_entry() finish_report()
On 11/10, Roland McGrath wrote: Ok. I realize it's largely separate, but I think we want to hash through this along with the other set of after-report-behavior problems. Also, it doesn't seem sensible to fiddle with utrace_report_syscall_entry separate from resolving UTRACE_SYSCALL_RESUMED change to the API. There was not much feedback about that change, but IIRC nobody was against it. So I've decided to merge the old utrace-syscall-resumed branch into utrace-cleanup. I saw you have utrace-syscall-resumed branch but never looked at it. I'll try to follow these changes once I fix utrace-ptrace. Now that upstream problems with syscall_exit trap are hopefully resolved this should be easy. Today I was distracted by workqueue bug (bluetooth driver actually), will try tomorrow. Roland, I guess utrace-cleanup becomes the master branch, yes? I mean, utrace-ptrace should be updated according to the recent changes in utrace-cleanup, and this is what you are going to send upstream. This means you should also merge utrace-cleanup into utrace-ptrace? Then I fix utrace-ptrace. I _think_ this will be easy too. ptrace_report_syscall_exit() can notice ctx-resume == UTRACE_SINGLESTEP and send the trap if utrace_control() doesn't set TIF_SINGLESTEP synchronously. (And this means with utrace tracehook_report_syscall_exit() can do nothing except utrace_report_syscall_exit()). Oleg.
Re: utrace_report_syscall_entry() finish_report()
I saw you have utrace-syscall-resumed branch but never looked at it.
For those not concerned with its purpose, there is one thing you must
know. Every report_syscall_entry callback must do:
if (utrace_syscall_action(action) == UTRACE_SYSCALL_RESUMED)
return UTRACE_RESUME;
to avoid being confused by the new extra callbacks.
Roland, I guess utrace-cleanup becomes the master branch, yes?
I mean, utrace-ptrace should be updated according to the recent
changes in utrace-cleanup, and this is what you are going to send
upstream.
Right. I was waiting to be sure you didn't object. But AFAICT we now seem
to be happy with this branch modulo more necessary fixes.
This means you should also merge utrace-cleanup into utrace-ptrace?
Right.
Then I fix utrace-ptrace. I _think_ this will be easy too.
ptrace_report_syscall_exit() can notice ctx-resume == UTRACE_SINGLESTEP
and send the trap if utrace_control() doesn't set TIF_SINGLESTEP
synchronously.
(And this means with utrace tracehook_report_syscall_exit() can do
nothing except utrace_report_syscall_exit()).
Today we don't call into utrace unless UTRACE_EVENT(SYSCALL_EXIT) is set.
So if nothing changes in the utrace layer, this means that ptrace will
enable UTRACE_EVENT(SYSCALL_EXIT) (i.e. TIF_SYSCALL) for PTRACE_SINGLE*.
Is that what you had in mind?
There are two possible issues with doing it that way.
The first is purely from a ptrace perspective. Today PTRACE_SINGLE* does
not set TIF_SYSCALL. On x86 this doesn't really make a difference in the
underlying code, because TIF_SINGLESTEP takes the same path. So probably
this is fine. But on other machines I'm not entirely sure that TIF_SYSCALL
won't take a slower path than single-step alone caused already for syscall
entries. This surely pales in comparison to the overhead of the ptrace
stop and context switch and all, but still, it could be a change. On x86
I'm not sure there's any reason not to optimize that better in the future,
too. Even in the most optimal circumstance imaginable, the whole point is
to handle a SIGTRAP signal after the syscall, so that too probably dwarfs
the overhead difference of the faster entry path.
The other issue comes from the utrace API perspective. Here it just seems
entirely wrong to require the engine (ptrace or any other) to do any
syscall-exit magic of its own to make UTRACE_SINGLESTEP work on a syscall
insn like it does on other insns. The utrace layer should make that trap
appear transparently like the normal traps seen for UTRACE_SINGLESTEP.
So, we could have tracehook_report_syscall_exit send the signal as it does
now upstream. But probably better is to do it all in the utrace layer,
i.e. pass the step flag to utrace_report_syscall_exit. Since today
single-step generates a plain (forced) SIGTRAP, we can synthesize that.
There are at least two options here. One is to do:
if (step (task-utrace_flags UTRACE_SIGNAL_CORE)) {
force_sig_info(...);
}
That approximates the check that yesterday's x86 code does:
if (test_thread_flag(TIF_SINGLESTEP)
tracehook_consider_fatal_signal(current, SIGTRAP))
send_sigtrap(current, regs, 0, TRAP_BRKPT);
But still we are queuing a real signal here. That is no different from the
real hardware trap case, so perhaps that is the right thing to do for now.
(Eventually we want to have a way for all these traps to avoid becoming
real signals at all, but that is a subject for another day.) Like a real
signal, that has the problem of still being queued if the tracer detaches
immediately thereafter (so it gets delivered to the user). We could avoid
that for this fake signal by just setting some flag that tells
utrace_get_signal to synthesize it before dequeuing real signals.
It's probably not worth bothering with that complexity now.
Thanks,
Roland
Re: utrace_report_syscall_entry() finish_report()
Ok. I realize it's largely separate, but I think we want to hash through this along with the other set of after-report-behavior problems. Also, it doesn't seem sensible to fiddle with utrace_report_syscall_entry separate from resolving UTRACE_SYSCALL_RESUMED change to the API. There was not much feedback about that change, but IIRC nobody was against it. So I've decided to merge the old utrace-syscall-resumed branch into utrace-cleanup. We can consider the resume-action handling details given that code. I'll try to look at all this with, as you say, a fresh head, on Wednesday. Thanks, Roland
