> So. We are going to make a separate, dynamically allocated structure
> for tracees. Say, we add "struct ptrace_child *ptrace_child" into
> task_struct.
Right.
> attach/attachme do kmalloc() and use task_lock() to avoid races.
> (with the current locking write_lock(tasklist) alone is enough).
Sure. Just task_lock() is preferable (no global contention), that's
what I'd figured it would be. I think having ptrace_exit() early in
the exit path now might already make it a little easier to get out
from under the tasklist_lock entanglements.
> Eventually, we should move all ptrace-related fields from task_struct
> to ptrace_child (ptrace_message/last_siginfo/etc), but let's forget
> about them for now.
Right.
> So, for the start we have (of course, I use the random naming).
Looks fine to start. (BTW, my preference in all the new code is to
stick to "task" or "tracee" and avoid "child", also "tracer" instead
of "parent" in ptrace contexts. I think it already tends to confuse
readers of the code about the nature of the arcanely overlapping--but
mostly unrelated--parent:child and tracer:tracee relationships.)
> The first question, should we free it after detach?
I don't think this needs to be a priority if it complicates everything.
The ultimate goal will be to hang this off utrace_engine.data and let
the utrace layer serialize our nasty races for us. For the intermediate
steps before using utrace, the only performance standard I think we want
to try real hard on is not to degrade the most common cases: a task that
has never used ptrace and a task that has never been traced by ptrace.
> for example, instead of "if (p->ptrace)" we should do
> "if (p->ptrace_child && p->ptrace_child->ptrace_flags)", not good.
I think it is just fine if ->ptrace_child != NULL means only "goes off
the fast path". e.g.
static inline int task_ptrace(struct task_struct *task)
{
int ptrace = 0;
if (unlikely(task->ptrace_child)) {
struct ptrace_child *p;
rcu_read_lock();
p = rcu_dereference(task->ptrace_child);
ptrace = p ? p->ptrace : 0;
rcu_read_lock();
}
return ptrace;
}
But even that is a lot of hair for the incremental patches in the first
several stages, I think. So just never deallocate it, and:
static inline int task_ptrace(struct task_struct *task)
{
return unlikely(task->ptrace_child) ? task->ptrace_child->flags
: 0;
}
> But. Until we rework the locking (afaics, ->ptrace_mutex should be held
> throughout the while sys_ptrace() call),
I'm not sure that will be necessary. I am not comfortable with holding
it around access_process_vm and user_regset calls (arch_ptrace) and all.
I think we'll find it OK just to take it (again) inside ptrace_resume()
et al, places where the ptrace bookkeeping is touched.
> even ptracer can't safely access
> child->ptrace_child->ptrace_flags. Once sys_ptrace() drops tasklist,
> child's sub-thread can do exec, and de_thread() can untrace the child
> and free ->ptrace_child.
This kind of trouble is why going to dynamic allocation should start
with never-deallocate (i.e. until release_task or something). We can
get a lot of clean-up and see how it looks before dealing with the
troubles that would cause. Perhaps we don't ever bother with them until
after figuring out utrace-based plans. (That's what I'd intended for
putting all the tracer-side stuff in one struct and switching that to
dynamic allocation too--without deallocation races, it's just a cheap
and easy way to save memory/cache footprint for the 99.44% of tasks that
never call ptrace.)
> And of course, we should also move task_struct->parent into ptrace_child,
>
> // was task_struct->parent
> struct task_struct *ptrace_parent;
struct ptrace_task.tracer, please. :-)
> And now we have the new minor problem. Say, tracehook_tracer_task().
> How can it safely read ->ptrace_child->ptrace_parent ? Looks like
> we should make ->ptrace_child rcu-safe. Actually, I'd prefer to rely
> on task_lock(), but again, afaics this means we should do the locking
> first.
tracehook_tracer_task() is one of the oddest angles all around. It's
most unlike every other path, because it can be by any third party. But
it is used very little and does not need to be fast. So there are many
options. But this too is trivially punted until later if we do not
deallocate--I guess with this use, it would have to be until
__put_task_struct to have no locking in tracehook_tracer_task().
Thanks,
Roland
