> 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