utrace_report_syscall_entry() && finish_report()

[Oleg Nesterov]

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.