utrace_control(,,UTRACE_SINGLESTEP)

[Oleg Nesterov]

On 10/27, Roland McGrath wrote:
>
> > And yes, while I don't pretend I know what should be done, I agree the
> > current API (I don't mean the documentation, I mean the actual code) is
> > not right.
>
> The behavior is fine in practice, it's just that the lockdep rules are
> violated (and that's indeed horribly wrong in the cases that matter, which
> are vanishingly rare in real-world practice).

In this case I confused again. Let's forget about get_user_pages() under
spin_lock(), pretend it works.

Two engines, E1 and E2, the tracee sleeps in utrace_resume()->utrace_stop().

E1 does utrace_control(UTRACE_RESUME), E2 does utrace_control(UTRACE_SINGLESTEP).
How this can work?

If E2 calls utrace_control() first, the subsequent UTRACE_RESUME does
user_disable_single_step(), and (in general) E2 has no chance to re-assert
SINGLESTEP.

> > I agree. But I can't explain why I agree ;) I mean, I feel this should be
> > more clean, but given that I do not understand the low level details even
> > remotely I can't judge.
>
> What level of details don't you understand?
> I hope you understand everything in utrace.c!

Yes, I hope I understand utrace.c ;) I meant user_enable_single_step().
I don't really understand it even in x86 case, to many low level magic.

> Speaking of which, it really looks like it's already the case now that
> ->stopped exactly matches task_is_traced().

Yes. I believe, currently ->stopped can go away.

> I know we discussed this
> before and put it off.

By the time we discussed it, ->stopped was a bit more complicated iirc,
it could be set in some subtle ->exit_state cases.

Afaics, we can kill ->stopped, but utrace_stop() and utrace_finish_jctl()
should do unconditial spin_unlock_wait(utrace->lock) after resume (or
lock/unlock).

Of course, unlike ->stopped, task_is_traced() == T is not stable under
under untrace->lock, but I think we don't care.

> > And yes, this is not even consistent across machines (as you explained
> > before), that is why I am very nervous when I think about the changes
> > in this area.
>
> Ok.  I think this is a corner where we will have some play in cleaning up
> and unifying the behavior upstream without much complaint.  In the status
> quo, x86 passes only 0 to tracehook_report_syscall_exit and will synthesize
> a SIGTRAP only if TIF_SINGLESTEP is set on its return (i.e. the last call
> user_enable_*_step was after the last call to user_disable_single_step).
> Other machines either pass step=1 when user_enable_*_step had been used to
> reach tracehook_report_syscall_exit and do nothing else, or pass step=0 and
> have no other related checks I can see.
>
> For us, step=1 doesn't really give any new information.  It means
> PTRACE_{SINGLE,BLOCK}STEP was used to hit the syscall insn (or used after
> PTRACE_SYSCALL stopped for syscall-entry).  But now ptrace_context.resume
> tells us that if we check it.
>
> So, if nothing else, we can do user_disable_single_step somewhere inside
> tracehook_report_syscall_exit to disarm the x86 code and then behave
> uniformly across machines inside there.  But, the x86 maintainers are
> generally friendly to our changes if we are clear about their purpose.

Yes, but sometimes ptrace (current implementation) has no chance to
notice this case and it relies on syscall_trace_leave()->send_sigtrap(),
we are going to return to user-mode without utrace->report/interrupt set.

> On all those other machines with unconditional step=0, we can assume that
> either they don't have single-step at all, PTRACE_SINGLESTEP over a syscall
> insn is already broken (actually stops after the following insn instead of
> after the syscall insn), or their hardware works differently so it doesn't
> have this issue, and regardless that they never much cared about the nit.

Horror. Horror, horror, horror.

I just realized that ptrace_resume()->send_sigtrap() can only work on x86.
I always knew this should be changed, but I hoped this more or less correct
for now. But send_sigtrap() doesn't even exist on !x86 machines!

> [...snip...]
>
> Then arch-independent code can fill siginfo_t just right for a synthetic
> single-step trap.

Yes, I think we should do something arch-independent.

I need to read the code and think, then I'll ask you stupid questions.

Oleg.