On 08/07, Roland McGrath wrote:
>
> > 1. Suddenly I realized, I do not really understand why ptrace_attach()
> > tries to reuse the "almost detached" engine. Can't attach just fail
> > in this case as if the task is still ptraced?
>
> No (unless it's actually a race with the first PTRACE_DETACH call).
> Consider:
>
> ptrace(PTRACE_DETACH, pid, 0, SIGUSR1);
> ptrace(PTRACE_ATTACH, pid);
Indeed, you are right. The caller of DETACH has all rights to assume
ATTACH must succeed. I missed this.
> However, the behavior requirement is that the tracee deliver (not queue)
> the SIGUSR1 before the tracer can see it do anything else after the second
> attach.
> ...
> The new engine should have no way to intercept the parting signal.
> i.e., it happens before the first possible ptrace stop (of the second
> attach). If you were to attach the new engine along with the lingering
> old engine, then it would get a report_signal callback as if the parting
> signal were a newly-dequeued signal. So you have to avoid that somehow.
Hmm. Another problem I didn't think of...
> > 2. Or. Perhaps we can add ptrace_utrace_detached_ops ? All methods should
> > return UTRACE_DETACH, except ptrace_utrace_detached_ops->report_signal()
> > fixups ->last_siginfo and returns UTRACE_SIGNAL_XXX | UTRACE_DETACH.
>
> I'm not entirely positive there aren't any cases where another callback
> would be made before the report_signal.
Yes. But, afaics, if another callback can be called before report_signal
then we don't have a valid ->last_siginfo.
IOW. ptrace_detach(signr) should have effect only if the tracee sleeps
in utrace_get_signal()->finish_resume_report() path, right ? (I am ignoring
report_syscall case for now).
This means, before utrace_get_signal() is called with ->interrupt == T,
the only possible callback is utrace_report_jctl()->ptrace_report_quiesce().
> > ptrace_detach_task() sets engine->ops = ptrace_utrace_detached_ops before
> > utrace_control(UTRACE_INTERRUPT). We don't even need utrace_barrier().
> >
> > This means that the new debugger can another engine.
> >
> > Do you think this can work?
>
> Perhaps. In the utrace API it is not really kosher ever to change the
> ->ops pointer. But this just means we would have to carefully examine
> the utrace code paths and say what the true rules are about when and how
> changing ->ops is safe.
I think this should be safe. The tracee should be stopped. We can only
race with SIGKILL, but given that old ->ops = ptrace_utrace_ops can't go
away I think nothing bad can happen. Or we can do
spin_lock(utrace->lock);
if (utrace->stopped)
engine->ops = new_ops;
/* else: it is killed, we don't care */
spin_unlock(utrace_lock);
Anyway. I am not proud of this idea, and it needs more thinking in any case.
Or. Perhaps. detach can just mark engine as detached, and ptrace_attach()
does utrace_attach_task() without UTRACE_ATTACH_EXCLUSIVE. This means
we should rely on PT_PTRACED, not good. And this means we should teach
ptrace_utrace_ops's methods to take this case into account. I don't think
this is good idea...
> > But, utrace-ptrace does this differently. report_syscall_xxx() do not
> > play with signals, instead when ptracer does PTRACE_CONT/etc we send
> > the signal to tracee before wakeup.
>
> But off hand I don't see any particular reason it matters which of these
> two places this send_sig() call goes.
Yes, ptrace_resume(PTRACE_EVENT_SYSCALL)->send_sig() looks good to me.
Except,
> > This means that with utrace-ptrace ptrace_detach(sig) does not imply
> > the signal if the tracee reported PTRACE_EVENT_SYSCALL.
> >
> > Should be fixed or I missed something?
>
> One of us is missing something, because I didn't expect that result.
> Since it's even in question, I think we should clearly have a case in
> the ptrace-tests suite that specifically tests this behavior.
OK, so this should be fixed somehow...
> I haven't looked closely at the old utrace-ptrace.patch code in a while.
> (I keep telling you, you're not fixing this old code, you're writing it
> correctly yourself!)
Roland, if only I knew how to improve the old utrace-ptrace.patch!
If only I knew how to write it correctly from scratch...
And giwen that it must be ready asap, my only hope is to make some fixes
against this old utrace-ptrace.patch. And I still have no idea how to
solve the problems with detach. Well, perhaps swithing engine->ops can
work...
Other problems with attach/detach which are more or less simple:
1. ptrace_check_attach() must check child->parent == current. We can
do this without tasklist.
2. ptrace_attach() should do prepare_ptrace_attach() after security
checks, under ->cred_guard_mutex.
3. ptrace_traceme() does prepare_ptrace_attach() before security
checks too... Not good, this can deny ptrace_attach() from another
tracer, but hopefully we can ignore this problem for now.
4. ptrace_utrace_exit() is called from tracehook_report_exit(). This
is not right, we can race with ptrace_traceme(). We should move
this code into exit_ptrace() which is called when we already have
PF_EXITING.
5. ptrace_report_clone() is racy. parent->parent can exit and untrace
child before utrace_attach_task(child, UTRACE_ATTACH_CREATE ...).
This _looks_ fixeable, we should check child->ptrace after
utrace_attach_task(), if the tracee is no longer traced we can
do utrace_control(UTRACE_DETACH). The child is not stopped from
utrace's pov, but at least we know utrace_get_signal() should
complete the detach later.
(CLONE_PTRACE is not right too, but this is minor).
Anything else I missed?
But again. I still have no idea how to fix the problems with
ptrace_detach(signr) which doesn't do UTRACE_DETACH.
I am stuck.
Oleg.
