On 07/16, Roland McGrath wrote:
>
> > Looks like a horrible hack, imho. Utrace should know nothing about
> > ptrace.
>
> Agreed!
>
> > Can't we do something like
> [...]
> > + // SPECIAL!!! must not block/etc
> > + void (*notify_stopped)(...);
>
> Part of the point about utrace is to make it easier not to foul up other
> engines when your engine has a bug. Many of the rules about well-behaved
> callbacks have to be met by all engines or they will interfere; we can't
> really avoid that reality when we call engines' code.
>
> But part of the plan is not to make those rules too hard to meet.
> That's why the "don't block (for long)" rule is such a loose constraint
> in real kernel terms. Having engine-writers commonly need to write code
> that can't even call mutex_lock or kalloc, etc., is IMHO just too much
> to ask. It's far better if we don't have such tight constraints on any
> callback that engine-writers will really need to use, if even if just
> this special case. (Because it really is not a rare thing--this is the
> essential kind of synchronization that any engine-writer who handles
> stopping at all will want.)
Yes, I see. That is why I named it "notify_stopped", not report_xxx,
to emphasize it is really special and should be used with care and only
when really needed.
> So in today's utrace, the best you can do is:
>
> report_* does:
> wake_up(you);
> return UTRACE_STOP;
> you wake up and do:
> utrace_barrier
> => your callback is finished, its UTRACE_STOP is logged
> utrace_prepare_examine
> => if ok, it's really stopped now
> |> if -EAGAIN, you have to loop(?)
> user_regset->get, etc. (aka arch_ptrace)
> utrace_finish_examine
> => if ok, it really was really stopped when i said it was
> |> if -AGAIN, you have to loop(?)
>
> [... snip a lot of great explanations ...]
Thanks Roland. I even printed your message to re-read it carefully.
But there is another another reason why (I think) utrace_stop should
not play with ptrace/do_notify_parent_cldstop.
The usage of ->ptrace in utrace-ptrace is racy wrt attach/detach which
sets/clears ->ptrace. Only ptracer should change ->ptrace. Just for
example. If the tracee dequeues a signal right after PTRACE_ATTCH
does prepare_ptrace_attach(), then ptrace_set_action() can race with
ptrace_attach() doing "task->ptrace = PT_PTRACED".
I think we need another variable which should be used instead, and
the only "good" place where it can live is engine->data, it should
point to "struct ptrace_xxx".
So, if we do not want to add the special (and not-a-report) hooks
into engine->ops, then perhaps it makes sense to do
utrace_stop:
if (task_is_ptraced(task))
ptrace_utrace_stop(task);
void ptrace_utrace_stop(struct task_struct *task)
{
if (ptrace_resume_action(task) != UTRACE_STOP)
return;
read_lock(&tasklist_lock);
do_notify_parent_cldstop(task, CLD_TRAPPED);
read_unlock(&tasklist_lock);
}
But, even with the code above it is not easy to move the
"task->ptrace >> 16" info into engine->data, we have to find the engine
first which looks a bit ugly. And even utrace_stop()->task_is_ptraced()
does not look very nice.
So, perhaps we can just add engine->ops->notify_stopped() (or even
->ptrace_notify_stopped) without introducing UTRACE_STOP_NOTIFY, utrace_stop()
just calls this hook unconditionally if it != NULL.
Oleg.
